Skip to content

Update Ticker wrapper to handle early interrupts #8956

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

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Dec 4, 2018

Description

Update the LowPowerTickerWrapper class to handle rather than ignore early low power ticker interrupts. This ensures that devices don't get stuck in sleep due to a ignored early low power ticker interrupt.

Pull request type

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

Update the LowPowerTickerWrapper class to handle rather than ignore
early low power ticker interrupts. This ensures that devices don't get
stuck in sleep due to a ignored early low power ticker interrupt.
@c1728p9
Copy link
Contributor Author

c1728p9 commented Dec 4, 2018

This PR will cause more frequent low power ticker rescheduling and removes code which prevents violating this part of the HAL specification:

* * The ticker interrupt fires only when the ticker times increments to or past the value set by ticker_set_interrupt.
* Verified by ::ticker_interrupt_test and ::ticker_past_test

The benefit this brings is greatly increased stability since early low power ticker interrupts won't cause a lockup, as is the case in #8906.

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

LGTM

@cmonr
Copy link
Contributor

cmonr commented Dec 4, 2018

Change looks good to me, but @c1728p9 I'm wondering if this also needs input from Partners (ping @ashok-rao @MarceloSalazar @screamerbg) or @ARMmbed/mbed-os-hal

@cmonr
Copy link
Contributor

cmonr commented Dec 4, 2018

Marking for 11.1 since this feels like it's a fix, but could also be seen as a breaking change, pushing it out to 12.0-rc1

@0xc0170 0xc0170 requested a review from a team December 4, 2018 12:05
@klaas019
Copy link

klaas019 commented Dec 4, 2018

@cmonr
This fix along with #8777 appears to fix #8906. Would be nice to get this in 11.1 instead of 12.0-rc1 as #8777 is scheduled for 11.1, since lowpower ticker/timeout is broken as of 5.10.3 for st devices.

@cmonr
Copy link
Contributor

cmonr commented Dec 4, 2018

@klaas019 Thanks for the context. Will keep at 11.1 for now.

@cmonr
Copy link
Contributor

cmonr commented Dec 4, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 5, 2018

Test run: SUCCESS

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

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.

8 participants