Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32L4: Fix sleep implementation #7813

Merged
merged 2 commits into from
Oct 1, 2018

Conversation

MSiglreithmaierRB
Copy link
Contributor

Description

Correctly detect and handle the low power run mode when entering and exiting sleep mode.

The generic hal_sleep implementation tries to exit LPR mode always, resulting in a spin-loop during a critical section (disabled IRQ).
The new approach returns from LPR to Run mode if enabled (LPR bit set), enters sleep, and resets to the original state on wakeup (WFI). This follows the specification for the STM32L4.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

Correctly detect and handle the low power run mode when entering and exiting sleep mode.

The generic `hal_sleep` implementation tries to exit LPR mode always, resulting in a spin-loop during a critical section (disabled IRQ).
The new approach returns from LPR to Run mode if enabled (LPR bit set), enters sleep, and resets to the original state on wakeup (WFI).
Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your proposal of this improvement.
I have asked 1 question. Also we will run tests before we can merge this patch.

targets/TARGET_STM/sleep.c Outdated Show resolved Hide resolved
@cmonr cmonr requested a review from a team August 17, 2018 15:55
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 3, 2018

@ARMmbed/team-st-mcd Please review

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 21, 2018

@ARMmbed/team-st-mcd Please review

Is this patch still up to date, does it need any further edits or approved?

@LMESTM
Copy link
Contributor

LMESTM commented Sep 21, 2018

@MSiglreithmaierRB hi - is there any chance you could update your PR and use HAL interface instead of directly accessing registers ?
Also can you confirm how you actually test this ?

@MSiglreithmaierRB
Copy link
Contributor Author

MSiglreithmaierRB commented Sep 26, 2018

@LMESTM Sry, I didn't see the notification.
Our logic is basically like below- It's part of our larger framework but I cut it down to:

// power on device ..
UARTSerial uart (TX, RX);
// ..
uart.baud(115200)
uart.write("Hello");
wait_ms(8000); // could be also 1ms, we receive some data over during this call. Spin looping with wait_ms(0) would work in contrast as we don't hit the 'sleep' path there.
uart.read(..); // read out uart, we see that we are missing bytes from the response
// The UART buffer overflowed at higher baud rates. For lower rates we saw less drops

I don't have a simple example file to reproduce it atm, unfortunately.

is there any chance you could update your PR and use HAL interface instead of directly accessing registers ?

sure, I will update it!

@MSiglreithmaierRB
Copy link
Contributor Author

Pushed the requested change. Please let me know if further changes are required and/or if I'm supposed to squash the commits.

Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !

@LMESTM
Copy link
Contributor

LMESTM commented Sep 26, 2018

@MSiglreithmaierRB
Thanks for the change.
About the comment in your code sample that UART data may be missed at higher rates, deep sleep exit takes about 2ms, so the sequence deep sleep entry then exit will most probably take between 2 and 3ms which can explain the limit of achievable baudrates.

@MSiglreithmaierRB
Copy link
Contributor Author

@LMESTM Thanks for reviewing.
I checked the UART overflow flag before the LPR mode exit attempt and afterwards, and saw that during this time frame the data gets dropped as we are in a critical section here disabling the interrupt handlers. AFAIK we only enter sleep, not deep sleep. With higher baudrates I meant 115200, which we use in our code, compared to 19200 were it worked okish (still dropping data but a bit less..). With the change it works fine for both on our system.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 1, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 1, 2018

Build : SUCCESS

Build number : 3202
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7813/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 1, 2018

/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Oct 1, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 1, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 1, 2018

The test results are all passed, there is one character in the logs that caused CI to report unstable.
We are investigating, should not block this PR.

@0xc0170 0xc0170 merged commit 232543a into ARMmbed:master Oct 1, 2018
@MSiglreithmaierRB MSiglreithmaierRB deleted the os_sleep_stm32l4 branch October 2, 2018 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants