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

STM32 LPTICKER with RTC : better sleep time #8777

Merged
merged 2 commits into from Dec 20, 2018

Conversation

jeromecoutant
Copy link
Collaborator

Description

With current implementation, as the WakeUp timer is a 16 bits counter, the maximum sleep duration is about 2s.

Idea is to decrease WakeUp clock source frequency for "long time".

@c1728p9 @LMESTM

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@mbed-ci
Copy link

mbed-ci commented Nov 18, 2018

Test run: FAILED

Summary: 4 of 7 test jobs failed
Build number : 2
Build artifacts
Build logs

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 19, 2018

@jeromecoutant The above is a test run (don't mind it at the moment). I'll restart jenkins CI now

@mbed-ci
Copy link

mbed-ci commented Nov 19, 2018

Test run: FAILED

Summary: 4 of 7 test jobs failed
Build number : 3
Build artifacts
Build logs

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM

@cmonr
Copy link
Contributor

cmonr commented Nov 22, 2018

Restarting CI

@mbed-ci
Copy link

mbed-ci commented Nov 22, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 4
Build artifacts
Build logs

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 22, 2018

@OPpuolitaival Please review test failures (test run success ?)

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 22, 2018

We will restart CI but after reviews are done and having some 5.11 items in.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2018

reviewers please review this PR

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.

only one question - otherwise looks good

WakeUpCounter = WakeUpCounter/2;

if (WakeUpCounter > 0xFFFF) {
WakeUpCounter = 0xFFFF;
Copy link
Contributor

Choose a reason for hiding this comment

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

In case where wake-up is pushed to the upper limit, it means that we cannot sleep as much as was requested right ?
Is it then expected that higher layers (mbed-os core) takes care of synchronizing os-time again ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wake up can always be early, due to other wakeup sources than time. As long as the HAL reports how long it actually slept, that's fine, from the OS's point of view.

Copy link
Contributor

Choose a reason for hiding this comment

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

The common ticker layer can safely handle early wakeups, but the LowPowerTickerWrapper does not. It treats early interrupts as spurious interrupts and ignores them.

To prevent this I recommend adjusting the supported bits from lp_ticker_get_info to match what you can safely set. This way the common ticker layer will always schedule a wakup time the hardware can achieve. In the case of maximum clock division (1/4th normal speed) with a 16 bit counter that would be .bits=18. This value is currently set to 32 bits which can cause failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't .bits=18 still a little off? Can't actually schedule 0x3FFFF, only 0x3FFFC. Is that significant enough to matter?

@OPpuolitaival
Copy link
Contributor

Test failed because of broken hardware. The board is taken off from CI and now restarted the test

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 28, 2018

@LMESTM @c1728p9 Please finalize your reviews

@jeromecoutant
Copy link
Collaborator Author

Hi
This PR needs work.
Either:

  • LowPowerTickerWrapper mbed-os update to safely handle early wakeups
  • or/and update to declare 18 bits as indicated by Russ
  • or update in the set interrupt function to support 32 bits

@kjbracey
Copy link
Contributor

One question about performance here - how significant is the power saving of 32s versus 8s versus 2s sleep time? (And as part of that calculation, how long do we spend awake each time?). What would the saving be if we achieved much higher or unlimited time?

@mattbrown015
Copy link
Contributor

mattbrown015 commented Nov 29, 2018

how significant is the power saving of 32s versus 8s versus 2s sleep time?

Whether or not something is significant is difficult to quantify, justify and/or state categorically!

But, I looked at this for myself the other day and can give you some figures.

I'm using a STM32L4 and measured the wake up time as a little over 2 ms.

The sweet spot for our application is waking up and taking a measurement every 10 minutes. To take and transmit the measurement we spend less than 100 ms in Run mode.

So for 8s we spend ((10 * 60) / 8) * 2 = 150 ms in Run mode only to decide to go back to sleep. For 32s it becomes ((10 * 60) / 32) * 2 = 37.5 ms. Over a 100 ms not in Run mode feels worth it to me.

Obviously hours, or even days, would be much better for the situations where the sample interval is reduced but I can see why this would be difficult to achieve while maintaining accuracy.

@kjbracey
Copy link
Contributor

I am kind of asking because I'm trying to justify (or not) some sort of API extension which would permit 1 second granularity, which would then presumably open the door to much longer sleeps here.

In STM, can we sleep with 1s wakeup precision to get a longer range (RTC alarm), but still get a <1ms precision measurement of sleep time? Or would we lose sleep time measurement precision too?

@LMESTM
Copy link
Contributor

LMESTM commented Nov 29, 2018

Last time I had this discussion, users were asking for an RTC Alarm API.
I thnk that the RTC Alarm API would allow long period of sleep without wake-ups and is pretty intuitive to understand by users. Several users have asked for it in past years.
This may need mbed-OS core to be able to not schedule any wake-up timer at all ?
Whether power saving would be significant is hard to tell, but this is a recurring request ...

@kjbracey
Copy link
Contributor

kjbracey commented Nov 29, 2018

The two possibilities I'm looking at initially are

 suspend(seconds)

so potential for rtc alarm to be built-in to the call, or maybe

rtcalarm->set(seconds)
suspend()

So no built-in suspend wakeups, just whatever wakeup sources you have.

I'm inclined to go for the first, as it can be implemented using existing LPTimer. Use of RTC alarm could be added under the surface.

Getting the automatic idle to also have that RTC alarm potential under the surface would also be possible, but would probably require the ability to provide an accurate measurement of how long the sleep period actually was, even if the wakeup had 1-second granularity. Whereas for an explicit suspend we could just document the potential of 1 second system clock drift.

@LMESTM
Copy link
Contributor

LMESTM commented Nov 29, 2018

I like the RTC alarm idea because application may rely on a RTC alarm handler to be called,
whether the application is just back from suspend or was already awake when the alarm expires ...
So my 2 cents: I like your 2nd proposal better :-)

@kjbracey
Copy link
Contributor

I feel you, but one of my design goals is getting suspend working without the requirement for HAL changes... Leveraging that to overcome the 16-bit * 10ms limitation on STM is a side bonus.

As for the alarm handler, Mbed OS already has a lot of ways to get a callback at a specific time - I'm not yet convinced there's a separate need for an API requesting a callback specifically directly from the RTC handler peripheral. Something more under-the-surface could potentially cope better with latency, such as with the latency stuff.

ThisThread::wait(1000) can pre-fire the LPTicker early to ensure an accurate wakeup, whereas requesting 1000 directly from the LPTicker will get you there late if coming from deep sleep. Same logic could apply for using the RTC alarm to wake up within a second of the correct time, then a lpticker to hit more precisely.

@jeromecoutant
Copy link
Collaborator Author

Can we start CI ?
Thx

@cmonr
Copy link
Contributor

cmonr commented Dec 11, 2018

Marked as needing CI.

Waiting for RC3 PRs to wrap up before starting CI for 5.11.1 PRs.

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 12, 2018

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 12, 2018

Pipe closed in CI, restarting exporters

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 12, 2018

This looks ready from CI point of view but not reviewers.

Reviewers, please review

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

@kjbracey-arm @c1728p9 Mind doing a quick re-review? This appears to be ready aside for final checks.

@cmonr
Copy link
Contributor

cmonr commented Dec 19, 2018

Rerunning CI since it's been more than a week.

@cmonr
Copy link
Contributor

cmonr commented Dec 19, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 20, 2018

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 6
Build artifacts

@kjbracey
Copy link
Contributor

This introduces issue #9176, fixed by #9180.

@mattbrown015
Copy link
Contributor

Hi @jeromecoutant ,

I've found a problem with this which really deserves a new issue. I'm busy doing other things so I'll just mention the problem with the expectation that you're not actually going to do anything!

I've found this change affects our event scheduling accuracy. When scheduling events for only a minute or 2 in the future it sometimes starts selecting the least accurate wake-up clock.

It is repeatable but I haven't had time to investigate. I've stopped it selecting RTC_WAKEUPCLOCK_CK_SPRE_16BITS in order to keep our scheduling working how we want it.

My feeling is that the problem isn't with what you've done. I suspect that the RTOS starts misjudging the sleep time and calls rtc_set_wake_up_timer with a number that is too big.

@cmonr
Copy link
Contributor

cmonr commented Jan 17, 2019

with the expectation that you're not actually going to do anything!
Well that seems a bit rude...

Wouldn't fault him considering comments in a closed PR aren't tracked. In the future, please open issues instead of commenting in closed/merged PRs.

@mattbrown015
Copy link
Contributor

Well that seems a bit rude

Sorry, I didn't mean to be rude. I certainly didn't intend to imply any criticism of the ST or Mbed teams.

Rather I meant, I wasn't expecting @jeromecoutant to start an investigation based on my vague and incoherent ramblings.

If this was more important to me I'd do some more investigation, gather some evidence and try and write a coherent issue. I haven't got time to do that now and I'm not expecting @jeromecoutant to do it for me.

But, I mentioned it in case @jeromecoutant sees, or hears about, something funny and perhaps things will start adding up into something a bit more tangible!

I was actually trying to avoid making a fuss and work for everyone. I've completely failed in that regard!

@cmonr
Copy link
Contributor

cmonr commented Jan 17, 2019

Thanks for the clarification.

Sarcasm over a text-only medium is hard 😄

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.

None yet

9 participants