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

Add handling for synchronized low power tickers #6536

Merged
merged 1 commit into from Apr 18, 2018

Conversation

Projects
None yet
7 participants
@c1728p9
Contributor

c1728p9 commented Apr 3, 2018

Some low power tickers take multiple cycles of the low power clock to set a compare value. Because of this if the compare value is set twice back-to-back these implementations will block until that time has passed. This can cause system stability issues since interrupts are disabling for this time.

To gracefully support this kind of hardware this patch adds code to prevent back-to-back writes to the hardware. It does this by recording the low power clock cycle of the initial write. If any writes come in too soon after this initial write the microsecond ticker is used to schedule the new write in the future when the hardware is ready to accept a new value.

To enable this feature on a target the macro LOWPOWERTIMER_DELAY_TICKS must be set to the number of low power clock cycles that must elapse between writes to the low power timer.

[X] Feature

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Apr 3, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 5, 2018

Shouldn't LOWPOWERTIMER_DELAY_TICKS be part of ticker info? It would be better to have that member there than macro?
Minar had some similar requirement for tickers - the nearest delay that a ticker can schedule - if value is lower than that, it would be fired immediately. I think this is similar concept here, isn't it ?

#include "mbed_critical.h"
static void set_interrupt_wrapper(timestamp_t timestamp);
#else
#define set_interrupt_wrapper lp_ticker_set_interrupt

This comment has been minimized.

@0xc0170

0xc0170 Apr 5, 2018

Member

Referencing here a proposal for the change this macro : #6473 (comment) . would it be more readable?

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 5, 2018

@c1728p9 c1728p9 force-pushed the c1728p9:sync_lp_ticker_support branch from 31dfd7d to f2844db Apr 5, 2018

@c1728p9 c1728p9 requested a review from kjbracey-arm Apr 5, 2018

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Apr 5, 2018

Updated this PR by making the following changes:

  • Moved wrapper code to a new file
  • Changed mbed_lp_ticker_wrapper back to .c (from .cpp)
  • Removed macro for set_interrupt and instead using #if defined directly.
@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Apr 5, 2018

Shouldn't LOWPOWERTIMER_DELAY_TICKS be part of ticker info? It would be better to have that member there than macro?

I made this enabled by a macro so this code won't be present for devices which don't need it. As an alternative it could be a part of ticker info but in that case when you use the low power ticker, the microsecond ticker will also be included in your binary even for devices which don't need it.

if value is lower than that, it would be fired immediately

There is already the fire now ticker API which can be used to trigger interrupts immediately, so I'm not sure if there would be much value to this behavior. With the current implementation the code guarantees that the ticker irq will fire at or after the tick specified. Firing early would break this guarantee and make it harder to ensure correct handling in the common ticker layer.

Minar had some similar requirement for tickers - the nearest delay that a ticker can schedule - if value is lower than that, it would be fired immediately. I think this is similar concept here, isn't it ?

The primary purpose of this change is to prevent blocking when set_interrupt is called back-to-back, regardless of the value being set. Calling set interrupt to fire 1 second in the future and then calling it again immediately to fire 2 seconds in the future causes blocking without this change. Minar like requirements is also added as a side effect -

In addition to the code to prevent blocking, this patch also ensure that lp_ticker_set_interrupt isn't called with an alarm time sooner than LOWPOWERTIMER_DELAY_TICKS. This is done because the value LOWPOWERTIMER_DELAY_TICKS implies that it takes that amount of time for the value to take effect - If you set it to a value less than this it may cause the event to trigger on the next rollover rather than in a few clock cycles.

If these values aren't intrinsically coupled (which they may be), it may be best to separate out these two concepts so they aren't conflated:

  1. LOWPOWERTIMER_DELAY_TICKS - Number of clock cycles that must pass between calls to ticker_set_interrupt
  2. The minimum number of clock cycles in the future that it is safe to setup an alarm with ticker_set_interrupt

What do you think @0xc0170, @kjbracey-arm, @LMESTM, @jeromecoutant, @pan-?

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Apr 6, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 6, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 6, 2018

What do you think @0xc0170, @kjbracey-arm, @LMESTM, @jeromecoutant, @pan-?

LGTM, so delay low power ticks stays as it is in this PR

@0xc0170 0xc0170 dismissed their stale review Apr 6, 2018

updated

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 9, 2018

/morph test

@mbed-ci

This comment has been minimized.

@LMESTM LMESTM referenced this pull request Apr 9, 2018

Closed

Waking STM32L073 from sleep #6362

@0xc0170 0xc0170 added needs: review and removed needs: work labels Apr 9, 2018

@cmonr

LGTM, just a few clarification questions.

*/
#include "hal/lp_ticker_api.h"
#if DEVICE_LOWPOWERTIMER && defined(LOWPOWERTIMER_DELAY_TICKS) && (LOWPOWERTIMER_DELAY_TICKS > 0)

This comment has been minimized.

@cmonr

cmonr Apr 9, 2018

Contributor

Is there a reason why DEVICE_LOWPOWERTIMER isn't wrapped with a defined?

This comment has been minimized.

@0xc0170

0xc0170 Apr 10, 2018

Member

would not hurt but by default values not set defaults to 0.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 10, 2018

Contributor

Conversely there's no need for the defined(LOWPOWERTIME_DELAY_TICKS) - if it's not defined, the #if will treat it as 0.

next = timestamp;
last_request = current;
if (!timeout_pending) {
timeout->attach_us(set_interrupt_later, reschedule_us);

This comment has been minimized.

@cmonr

cmonr Apr 9, 2018

Contributor

I take it that this doesn't need to be unattached once the timeout elapses?

This comment has been minimized.

@0xc0170

0xc0170 Apr 10, 2018

Member

Not needed. Detach only if you want to remove it early

@cmonr

Holding off on merge until a second person OK's the PR.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2018

I did not get full details for the issue in #6522. But to me it seems related (please review). Realtek has a ticker (in this case high freq) also that takes some time to set interrupt.

cc @M-ichae-l

@cmonr cmonr added needs: review and removed needs: work labels Apr 16, 2018

Add handling for synchronized low power tickers
Some low power tickers take multiple cycles of the low power clock
to set a compare value. Because of this if the compare value is set
twice back-to-back these implementations will block until that time
has passed. This can cause system stability issues since interrupts
are disabling for this time.

To gracefully support this kind of hardware this patch adds code
to prevent back-to-back writes to the hardware. It does this by
recording the low power clock cycle of the initial write. If any
writes come in too soon after this initial write the microsecond
ticker is used to schedule the new write in the future when the
hardware is ready to accept a new value.

To enable this feature on a target the macro LOWPOWERTIMER_DELAY_TICKS
must be set to the number of low power clock cycles that must elapse
between writes to the low power timer.

@c1728p9 c1728p9 force-pushed the c1728p9:sync_lp_ticker_support branch from 933fd07 to 17892cb Apr 16, 2018

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Apr 16, 2018

I pulled the fix 436f1d1 into this PR.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 17, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

@studavekar Please review the java exception, the job took too long?

/morph export-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

Waiting for the @kjbracey-arm @bulislaw review

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

@bulislaw Any comments?

@0xc0170 0xc0170 merged commit 06eefcb into ARMmbed:master Apr 18, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10346 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B
Details
travis-ci/tools Local tools testing has passed
Details

jeromecoutant added a commit to jeromecoutant/mbed that referenced this pull request Apr 23, 2018

bulislaw added a commit to bulislaw/mbed-os that referenced this pull request May 15, 2018

bulislaw added a commit that referenced this pull request May 24, 2018

bulislaw added a commit to bulislaw/mbed-os that referenced this pull request May 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment