georgerobotics / cyw43-driver

Other
83 stars 44 forks source link

Power saving labels are reversed and have propagated issues to other projects #122

Closed Darkhand81 closed 1 month ago

Darkhand81 commented 4 months ago

Hello!

The labels for power saving modes in /src/cyw43.h seem to be reversed:

https://github.com/georgerobotics/cyw43-driver/blob/1d2227bca200e13c1d6a630032d5f6d7fca69fef/src/cyw43.h#L638-L646

CYW43_AGGRESSIVE_PM states that it provides "optimial power usage at the cost of performance", but running the chip for 2000ms uses more power and keeps the chip running longer, providing better performance compared to CYW43_PERFORMANCE_PM, which states that it uses "more power ... to increase performance". That mode goes to low power in only 20ms, using less power and hurting performance.

This seems to have caused issues with regard to using power saving modes with the chip in general: https://github.com/raspberrypi/pico-sdk/issues/912

It does appear to have been noticed: https://github.com/raspberrypi/pico-sdk/issues/912#issuecomment-1180318506

However more broadly, it's also leaked into several other major project's documentation and is being used as gospel: https://www.raspberrypi.com/documentation/pico-sdk/group__cyw43__driver.html https://docs.circuitpython.org/en/latest/shared-bindings/cyw43/index.html

As well as in postings and tutorials: https://adafruit-playground.com/u/blakebr/pages/wifi-power-management-for-the-raspberry-pi-pico-w https://forums.raspberrypi.com/viewtopic.php?t=340050#p2037055

I believe many of the issues folks are having across the internet with regard to this chip and power saving modes are related to this mixup!

peterharperuk commented 4 months ago

Yes, it does appear to be the wrong way around. Larger values of pm2_sleep_ret_ms would imply an increase in power usage However larger values for the listen interval imply a decrease in power usage.

dpgeorge commented 4 months ago

Thanks for the report. I think you are right that the constants are not correct.

Doing some testing:

For all three of the pre-defined constants CYW43_DEFAULT_PM, CYW43_AGGRESSIVE_PM and CYW43_PERFORMANCE_PM, doing a fast ping -i 0.1 <ip> to the cyw43, there's pretty much no difference (they all have low latency). That's because they all use CYW43_PM2_POWERSAVE_MODE and the fast ping is supplying constant traffic over the air and keeping the cyw43 awake.

Doing a standard 1-second interval ping you see the difference between these three pre-defined constants: CYW43_AGGRESSIVE_PM still has low latency, but the other two have very varied ping latency, up to about 200ms (because the cyw43 has gone to sleep and takes time to wake up).

So I see two issues with these three pre-defined constants:

In the end the cyw43_pm_value() function is there for users to define their own power-management settings which suit their application. But definitely we should be providing a good set of well-named constants.


MicroPython has its own set of constants, which seem to be correctly labelled and give a more varied set of behaviours:

#define PM_NONE         (CYW43_PM_VALUE(CYW43_NO_POWERSAVE_MODE, 10, 0, 0, 0))
#define PM_PERFORMANCE  (CYW43_PM_VALUE(CYW43_PM2_POWERSAVE_MODE, 200, 1, 1, 10))
#define PM_POWERSAVE    (CYW43_PM_VALUE(CYW43_PM1_POWERSAVE_MODE, 10, 0, 0, 0))

Maybe we can use those? The default would be PM_PERFORMANCE.

dpgeorge commented 3 months ago

See #126 for a fix based on the above proposal.

dpgeorge commented 1 month ago

Fixed by 6a05a0d55e489f7e60c0b9542d7704a59a82afb9