Add Low Power driver #183

ghost · 2017-12-20T12:52:27Z

Allow to use some low power mode:

Sleep
Stop
Standby
Shutdown

Wake up sources (depends on LP mode):

All interrupts
Wake up pins
Reset
UART
I2C (not implemented yet)
RTC

The driver works with NUCLEO-L476RG & NUCLEO-L053R8.
This driver is used by the STM32LowPower library.

fpistm

One major things to review: PinMap_WKUP

fpistm · 2017-12-21T09:04:33Z

cores/arduino/stm32/PeripheralPins.h

@@ -71,5 +71,7 @@ extern const PinMap PinMap_Ethernet[];
 //*** QUADSPI ***
 extern const PinMap PinMap_QUADSPI[];

+//*** WKUP ***


This should not be here. Only one linked to Peripheral <--> pins should be here.

fpistm · 2017-12-21T09:04:47Z

cores/arduino/stm32/uart.c

@@ -112,6 +112,7 @@ void uart_init(serial_t *obj)
  USART_TypeDef *uart_tx = pinmap_peripheral(obj->pin_tx, PinMap_UART_TX);
  USART_TypeDef *uart_rx = pinmap_peripheral(obj->pin_rx, PinMap_UART_RX);

+


Remove NL please

fpistm · 2017-12-21T09:06:04Z

cores/arduino/stm32/uart.c

@@ -126,6 +127,19 @@ void uart_init(serial_t *obj)
    return;
  }

+#ifdef STM32L4xx
+  // Select HSI as source clock 


Please, Could you explain why only L4 and why only USART 1-3 ? Waht about UART4-5?

fpistm · 2017-12-21T09:08:12Z

variants/NUCLEO_L053R8/PeripheralPins.c

@@ -188,3 +188,11 @@ const PinMap PinMap_SPI_SSEL[] = {

 //*** CAN ***
 // NO CAN
+


PeripheralPins should only contains array For Pin <--> Peripheral.
How this array is defined? Is it linked to boards? MCU ? Is it exhaustive?

fpistm · 2017-12-21T09:08:22Z

variants/NUCLEO_L476RG/PeripheralPins.c

+
+//*** WAKE UP ***
+
+const PinMap PinMap_WKUP[] = {


PeripheralPins should only contains array For Pin <--> Peripheral.
How this array is defined? Is it linked to boards? MCU ? Is it exhaustive?

Agree with @fpistm. Please add comment ...
From what I've read in MCU datasheet that is related to a given range of MCUs (e.g. L4x6), so this can be defined for all boards using the same MCU. Whether each pin is connected or not should not change the table.

fpistm · 2017-12-21T09:08:46Z

variants/NUCLEO_L476RG/variant.cpp

@@ -123,11 +123,11 @@ WEAK void SystemClock_Config(void)
 {
  RCC_OscInitTypeDef RCC_OscInitStruct = {};
  RCC_ClkInitTypeDef RCC_ClkInitStruct = {};
+  RCC_OscInitTypeDef RCC_OscInitStructHSI = {};


It could be mixed with RCC_OscInitStruct

fpistm · 2017-12-21T09:09:16Z

variants/NUCLEO_L476RG/variant.cpp

@@ -123,11 +123,11 @@ WEAK void SystemClock_Config(void)
 {
  RCC_OscInitTypeDef RCC_OscInitStruct = {};
  RCC_ClkInitTypeDef RCC_ClkInitStruct = {};
+  RCC_OscInitTypeDef RCC_OscInitStructHSI = {};

  /* MSI is enabled after System reset, activate PLL with MSI as source */
  RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_MSI;


ex: RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_MSI | RCC_OSCILLATORTYPE_HSI
....

fpistm · 2017-12-21T09:09:47Z

variants/NUCLEO_L476RG/variant.cpp

@@ -156,6 +155,18 @@ WEAK void SystemClock_Config(void)
    /* Initialization Error */
    while(1);
  }
+
+  /*## Enable the HSI clock  #*/


See previous comment, to avoid call twice HAL_RCC_OscConfig()

fpistm · 2017-12-21T09:11:17Z

cores/arduino/stm32/LowPower.c

+  * @param  mode: pin mode (edge or state). The configuration have to be compatible.
+  * @retval None
+  */
+void LowPower_EnableWakeUpPin(uint32_t pin, uint32_t mode) {


Is the STM32L1xx supported ? (default ?)

L1 not supported.

fpistm · 2017-12-21T09:13:59Z

cores/arduino/stm32/LowPower.c

+  UNUSED(mode);
+#endif
+  PinName p = digitalPinToPinName(pin);
+  uint32_t wupPin = pinmap_find_function(p, PinMap_WKUP);


PinMap_WKUP is the config? Maybe it should be at sketch level ? And/Or adding a method to set wakeup pins?

PinMap_WKUP is the list of wake up pins available on the board.
I don't think it should be at sketch level.
I suggest to move PinMap_WKUP in variant.cpp. What do you think about it?

We need to clarify a few points I think concerning this table:
which low power modes are we talking about ?
where are the pins defined for a given MCU ?

Wake up pins touch the standby mode.

If you confirm us that pins are the same across a MCU family, we will able to look for another logic and remove the array from variant.cpp.

Please add it in the comments or above table declaration that PinMap_WKUP only applies to standby/shutdown. I am not sure whether pins are always the same accross MCUs from the same family ...

LMESTM

thanks for the PR. Few comments and questions shared

LMESTM · 2018-01-09T15:44:28Z

cores/arduino/stm32/LowPower.c

+  UNUSED(mode);
+#endif
+  PinName p = digitalPinToPinName(pin);
+  uint32_t wupPin = pinmap_find_function(p, PinMap_WKUP);


We need to clarify a few points I think concerning this table:
which low power modes are we talking about ?
where are the pins defined for a given MCU ?

LMESTM · 2018-01-09T16:12:29Z

cores/arduino/stm32/LowPower.c

+void LowPower_sleep(){
+  /*Suspend Tick increment to prevent wakeup by Systick interrupt.
+    Otherwise the Systick interrupt will wake up the device within 1ms (HAL time base)*/
+  HAL_SuspendTick();


Shouldn't we disable IRQs and re-enable them after
using __disable_irq(); and __enable_irq(); ?
Otherwise the IRQ handler can be called after HAL_SuspendTick(); and this could have side effects in case the tick is being used by the interrupt handler.

same applies to all modes ...

If I understood, you suggest something like this:

__disable_irq(); HAL_SuspendTick(); __enable_irq();

?

I was advising __disable_irq before suspending tick and __enable_irq after HAL_ResumeTick

Unfortunately you can't because interrupts allow to wake up the MCU. HAL_SuspendTick() disables only Tick IRQ as explained in the comment.

Have you tried it?
I think it should block the SW handling of interrupts (disabled at NVIC level), but it should not block the peripherals interrupts to wake-up the MCU. and yes we still need to stop the TICK interrupt otherwise it would wake-up the MCU.

Hope I'm clear enough ... the point is to enter a critical section, not to disable interrupts at peripheral level.

LMESTM · 2018-01-09T16:13:15Z

cores/arduino/stm32/LowPower.c

+ extern "C" {
+#endif
+
+static UART_HandleTypeDef* WakeUpUart = NULL;


what are WakeUpUart and WakeUpUartCb ? please comment what this is intended for

It is use to save the wake up callback provides by user from enableWakeupFrom().

please comment in the code ...
enableWakeupFrom does not seem to be related to UART, but callback seems to cover UART only .. this could be misleading ...

LMESTM · 2018-01-09T16:16:05Z

cores/arduino/stm32/LowPower.c

+/**
+  * @brief  Configure the UART as a wakeup source. A callback can be called when
+  *         the chip leaves the low power mode. See board datasheet to check
+  *         with which low power mode the UART is compatible.


from the above implementation we can state that this is not supported in standby nor shutdown modes

LMESTM · 2018-01-09T16:20:02Z

cores/arduino/stm32/LowPower.c

+  HAL_ResumeTick();
+
+  if (WakeUpUartCb != NULL) {
+    WakeUpUartCb();


this is not clear why UART callback is always called, even if wake-up source is not UART ?

At first sight, we aren't able to know which source wake up the MCU. Maybe we have missed something.

LMESTM · 2018-01-09T16:24:34Z

variants/NUCLEO_L476RG/variant.cpp

@@ -125,9 +125,10 @@ WEAK void SystemClock_Config(void)
  RCC_ClkInitTypeDef RCC_ClkInitStruct = {};

  /* MSI is enabled after System reset, activate PLL with MSI as source */
-  RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_MSI;
+  RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_MSI|RCC_OSCILLATORTYPE_HSI;


so that means we will always have HSI and MSI running ... can't we keep HSI only as the purpose of this PR is to save power ?

We'll look the impact of keep only HSI. But I think we could.

LMESTM · 2018-01-09T16:29:17Z

variants/NUCLEO_L476RG/PeripheralPins.c

+const PinMap PinMap_WKUP[] = {
+    {PA_0,  NP,  1},  //WKUP1
+    {PC_13, NP,  2},  //WKUP2
+//  {PE_6,  NP,  3},  //WKUP3 - Not connected


I would keep it anyway and leave it up to the user to select a pin that is configured.

LMESTM · 2018-01-09T16:29:20Z

variants/NUCLEO_L476RG/PeripheralPins.c

+
+//*** WAKE UP ***
+
+const PinMap PinMap_WKUP[] = {


Agree with @fpistm. Please add comment ...
From what I've read in MCU datasheet that is related to a given range of MCUs (e.g. L4x6), so this can be defined for all boards using the same MCU. Whether each pin is connected or not should not change the table.

… into quakesense Add LowPower driver and fix some conflicts in cores/arduino/stm32/uart.c

fpistm · 2018-03-06T09:51:39Z

Hi @fprwi6labs
any news on this PR?

…ource in stop mode Signed-off-by: fpr <[email protected]>

ghost · 2018-03-15T15:48:23Z

Hi @fpistm
I think this PR is up to date.

PinMap_WKUP has been moved from PeripheralPins.c to LowPower.c. For the moment we keep it until you can confirm those pins are common to all MCU familly.

Critical section added.

Wake up from I2C is not possible with the current I2C implementation. It is not possible to use the listen interrupt as wake up source (see datasheet). We have no solution to both keep the I2C functionality and add the wake up from I2C.

biagiom · 2018-04-03T22:31:05Z

Hi @fpistm,
I have added some improvements to LowPower driver that could be added to the great work made by @fprwi6labs:

I merged the Add RTC driver #177 pull request which regards the RTC driver used by the LowPower driver, then I also merged Add Low Power driver #183 pull request removing some conflicts
PinMap_WKUP list has been updated using some datasheets and documentation I found about the STM32 MCU families. I also updated some #define and #include directives according to the MCU family.

You can find all the changes in the quakesense branch of my fork of Arduino_Core_STM32 repo (the branch is called quakesense because it includes the improvements I made to the LowPower library in order to use it in my QuakeSense project which I also used for testing the LowPower driver and STM32LowPower library).

I have added some updates to my fork of STM32LowPower library - branch generic-config and then I made some tests on my STM32 Nucleo F401RE: LowPower driver seemed to work quite well using the examples provided by the STM32LowPower library and I was able to put the STM32F401RE in idle, sleep, deepSleep and shutdown mode.
I also performed some tests on my B-L475E-IOT01A Discovery Kit but I didn't manage to get the Serial communication working when sending some data to the Serial Monitor of Arduino IDE (I think the issue depends on initialization of LPUART in cores/arduino/stm32/LowPower.c and/or cores/arduino/stm32/uart.c).

@fpistm and @fprwi6labs, what do you think ?

fpistm · 2018-04-04T04:54:38Z

Hi @biagiom ,
thanks for your work I will check that in the coming days.

fpistm · 2018-04-05T12:13:21Z

cores/arduino/stm32/LowPower.c

+ extern "C" {
+#endif
+
+// NOTE: is these pins are common to a MCU family?


Hereafter the list of the 21 possibles wake up pins across all STM32 MCU (Fx/H7/Lx)
#define SYS_WKUP0 PA_0
#define SYS_WKUP1 PA_0
#define SYS_WKUP1 PA_2
#define SYS_WKUP1 PC_13
#define SYS_WKUP2 PA_2
#define SYS_WKUP2 PC_0
#define SYS_WKUP2 PC_13
#define SYS_WKUP3 PA_2
#define SYS_WKUP3 PC_1
#define SYS_WKUP3 PE_6
#define SYS_WKUP3 PI_8
#define SYS_WKUP4 PA_2
#define SYS_WKUP4 PC_13
#define SYS_WKUP4 PI_11
#define SYS_WKUP5 PC_1
#define SYS_WKUP5 PC_5
#define SYS_WKUP5 PI_8
#define SYS_WKUP6 PB_5
#define SYS_WKUP6 PI_11
#define SYS_WKUP7 PB_15
#define SYS_WKUP8 PF_2
I will add defines linked to the targeted MCU in the variant to avoid defining a pinmap array and use those define here.

It's OK for me

fpistm · 2018-04-25T07:20:27Z

Driver build against all series has not been tested. Several series failed due to unknown macros or parameters.
@fprwi6labs why not test build for all series before submit ?

Signed-off-by: fpr <[email protected]>

ghost · 2018-04-26T13:57:55Z

Hi @fpistm

Fixed! Test build passed for all series.
But low power validated only for L0 & L4 series.

fpistm · 2018-05-23T07:50:20Z

Fully replaced by #255

ghost requested review from LMESTM, fpistm and VVESTM December 20, 2017 12:52

ghost mentioned this pull request Dec 20, 2017

STM32 Low Power library stm32duino/STM32LowPower#1

Closed

fpistm requested changes Dec 21, 2017

View reviewed changes

LMESTM reviewed Jan 9, 2018

View reviewed changes

fpistm added the new feature label Jan 11, 2018

ghost requested review from LMESTM and fpistm January 17, 2018 13:39

fpistm self-assigned this Jan 29, 2018

fpistm added this to the Next release milestone Jan 29, 2018

fpistm added the review on going label Jan 29, 2018

biagiom added a commit to biagiom/Arduino_Core_STM32 that referenced this pull request Feb 12, 2018

Merge pull request stm32duino#183 from fprwi6labs/add_lowPower_driver…

22c8dc5

… into quakesense Add LowPower driver and fix some conflicts in cores/arduino/stm32/uart.c

fpistm added the waiting feedback Further information is required label Mar 7, 2018

Update NUCLEO-L476 clock config: enable HSI to allow uart as wakeup s…

39db47b

…ource in stop mode Signed-off-by: fpr <[email protected]>

fpistm modified the milestones: Next release, 1.2.1 Mar 23, 2018

fpistm removed the waiting feedback Further information is required label Apr 5, 2018

fpistm reviewed Apr 5, 2018

View reviewed changes

fpr added 2 commits April 26, 2018 14:23

Update uart driver for low power mode

eb61637

Signed-off-by: fpr <[email protected]>

Add low power driver

05b8a54

Signed-off-by: fpr <[email protected]>

fpistm force-pushed the master branch from 5f641c5 to cf54e13 Compare May 12, 2018 16:06

fpistm added invalid This doesn't seem right abandoned No more work on this labels May 23, 2018

fpistm removed the review on going label May 23, 2018

fpistm closed this May 25, 2018

fpistm removed this from the 1.2.1/1.3.0 milestone May 28, 2018

fpistm mentioned this pull request Jun 6, 2023

gh pr list do not show some PR cli/cli#7538

Closed

fpistm added enhancement New feature or request and removed New feature labels Jul 16, 2024

		@@ -112,6 +112,7 @@ void uart_init(serial_t *obj)
		USART_TypeDef *uart_tx = pinmap_peripheral(obj->pin_tx, PinMap_UART_TX);
		USART_TypeDef *uart_rx = pinmap_peripheral(obj->pin_rx, PinMap_UART_RX);

		@@ -188,3 +188,11 @@ const PinMap PinMap_SPI_SSEL[] = {

		//* CAN *
		// NO CAN

Add Low Power driver #183

Add Low Power driver #183

Conversation

ghost commented Dec 20, 2017

fpistm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LMESTM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 15, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpistm commented Mar 6, 2018

ghost commented Mar 15, 2018

biagiom commented Apr 3, 2018

fpistm commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpistm commented Apr 25, 2018

ghost commented Apr 26, 2018

fpistm commented May 23, 2018

ghost Jan 15, 2018 •

edited by ghost

Loading