zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.76k stars 6.56k forks source link

The STM32 standby_shutdown sample has a simple mistake #79451

Open MZachmann opened 3 weeks ago

MZachmann commented 3 weeks ago

In the sample application for the STM32 power management standby at samples/boards/stm32/power_mgmt/standby_shutdown

I'm pretty certain that the line in main.c line 49 that is currently:

LL_PWR_IsWakeUpPinPolarityLow(LL_PWR_WAKEUP_PIN2);

should be

LL_PWR_SetWakeUpPinPolarityLow(LL_PWR_WAKEUP_PIN2);

The first line requests the polarity and doesn't use the output which seems wrong. The second line (correctly) tells the os the pin polarity.

FRASTM commented 2 weeks ago

Thanks @MZachmann to point this. I do not see any requirement to read (access) the Power register CR4 (what is done by the LL_PWR_IsWakeUpPinPolarityLow function) before enabling the wakeUp pin, so yes the _LL_PWRIsWakeUpPinPolarityLow is useless

The test is to power off or standby (depending on the LED2) as long as the button is pressed, and wakes up when released --> rising edge since the button is active Low: so setting polarity High = the default register

However there is a "recommended" sequence, for the stm32L4 serie, for example :

/* The Following Wakeup sequence is highly recommended prior to each Standby mode entry
     mainly when using more than one wakeup source this is to not miss any wakeup event.
     - Disable all used wakeup sources,
     - Clear all related wakeup flags, 
     - Re-enable all used wakeup sources,
     - Enter the Standby mode.
  */

To me, this is better:

 void config_wakeup_features(void)
 {
-   /* Configure wake-up features */
-   /* WKUP2(PC13) only , - active low, pull-up */
-   /* Set pull-ups for standby modes */
-   LL_PWR_EnableGPIOPullUp(LL_PWR_GPIO_C, LL_PWR_GPIO_BIT_13);
-   LL_PWR_IsWakeUpPinPolarityLow(LL_PWR_WAKEUP_PIN2);
-   /* Enable pin pull up configurations and wakeup pins */
-   LL_PWR_EnablePUPDCfg();
+   /*
+    * Configure wake-up features : WKUP2(PC13) only
+    *
+    * The Following Wakeup sequence is highly recommended prior to each Standby mode entry
+    * mainly when using more than one wakeup source this is to not miss any wakeup event:
+    * - Disable all used wakeup sources,
+    * - Clear all related wakeup flags,
+    * - Re-enable all used wakeup sources,
+    * - Enter the Standby mode.
+    */
+   LL_PWR_DisableWakeUpPin(LL_PWR_WAKEUP_PIN2);
+   LL_PWR_ClearFlag_WU(); /* Clear wakeup flags */
    LL_PWR_EnableWakeUpPin(LL_PWR_WAKEUP_PIN2);
-   /* Clear wakeup flags */
-   LL_PWR_ClearFlag_WU();
 }
FRASTM commented 2 weeks ago

Second this sample should be upgraded thanks to the new PR "STM32: configure wake-up pins with GPIOs from devicetree for exiting Poweroff" and the new way of configuring wakeup pins introduced by this PR.