Add wifi.radio.listen_interval to ESP32-family chips. #9476
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The main risk of |
|
Yeah, I'm not sure if it also affects multicast, but broadcast isn't super commonly used so it might be fine. The advantage of capping at 1 is that it could then be completely safe to use higher values on all boards, and there could just be one parameter for DTIM beacon sleep and target wake time. But I'm not actually sure higher DTIM values are actually doing anything, ping times and power look the same as when set to 1. |
tannewt
requested changes
Jul 31, 2024
|
Sounds good, I'll make these changes tonight after I figure out this
pre-commit issue... I thought it looked a bit odd, but pre-commit didn't
show any errors, maybe I just need to manually run it once or something.
In further testing I'm not actually quite sure values above 1 are doing
anything, ping times look the same as they do for 1.
The advantage to capping at 1 would be that using a higher value would
always be totally safe, so you could use the same code for WiFi 6, and it
could just automatically figure out the best possible power savings without
risk of losing broadcasts. But maybe broadcast packets aren't that
important these days?
…On Wed, Jul 31, 2024, 12:21 PM Scott Shawcroft ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks for the addition! Please add it to all wifi ports even if it raises
an exception from the port code.
It also looks like you have a code formatter that disagrees with ours. I
think setting up pre-commit will format it back for you.
------------------------------
In shared-bindings/wifi/Radio.c
<#9476 (comment)>
:
> @@ -186,6 +185,29 @@ MP_PROPERTY_GETSET(wifi_radio_tx_power_obj,
(mp_obj_t)&wifi_radio_get_tx_power_obj,
(mp_obj_t)&wifi_radio_set_tx_power_obj);
+#if CIRCUITPY_WIFI_RADIO_SETTABLE_LISTEN_INTERVAL
Don't conditionally enable this here. We want it on all ports. If
unsupported, then assigning to it can raise an error using
mp_raise_NotImplementedError().
------------------------------
In shared-bindings/wifi/Radio.c
<#9476 (comment)>
:
> @@ -186,6 +185,29 @@ MP_PROPERTY_GETSET(wifi_radio_tx_power_obj,
(mp_obj_t)&wifi_radio_get_tx_power_obj,
(mp_obj_t)&wifi_radio_set_tx_power_obj);
+#if CIRCUITPY_WIFI_RADIO_SETTABLE_LISTEN_INTERVAL
+//| listen_interval: int
+//| """Wifi power save listen interval power, in DTIM periods, or 100ms intervals if TWT is supported."""
⬇️ Suggested change
-//| """Wifi power save listen interval power, in DTIM periods, or 100ms intervals if TWT is supported."""
+//| """Wifi power save listen interval, in DTIM periods, or 100ms intervals if TWT is supported."""
—
Reply to this email directly, view it on GitHub
<#9476 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFZCH2UCCD5AWK6OX3KVR3ZPETK5AVCNFSM6AAAAABLXW62PKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMJQHA2DOOJVGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I think I've partially figured out the issue! It was running the normal system uncrustify instead of the micropython-uncrustify version. Not sure what to do about the error string translation stuff for the notimplemented error, looks like there's an existing string "can't set attribute" that would fit? |
tannewt
requested changes
Aug 5, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not heavily tested, and is posted for review and discussion as part of work on #9463.
It allows you to set the listen interval of the WiFi hardware to reduce power consumption.
When listen interval is 1, WIFI_PS_MIN_MODEM is used, when it is more than 1, WIFI_PS_MAX_MODEM is used.
The listen interval is set in units of whatever the AP's beacon interval is, most likely 100ms.
Without it, and running at 80MHz, power consumption hovers around 110mA with 140mA spikes. When set to 1, power is ~60mA with much less frequent spikes.
Setting it to more than 1 makes no difference that I can see on the power meter, it seems to be a sub-milliamp difference, so I almost wonder if it might be better to just cap it at 1, and use higher values only if there's more advanced features like target wake time available on your chip.