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.

Sign up for GitHub

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

Jump to bottom

Fix LowPowerTickerWrapper operation when suspended #8279

Merged
merged 2 commits into from
Oct 26, 2018
Merged

Fix LowPowerTickerWrapper operation when suspended #8279

merged 2 commits into from
Oct 26, 2018

Conversation

fkjagodzinski
Copy link
Member

Description

Update the LowPowerTickerWrapper class logic to stop using the internal
Timeout object for scheduling LP ticker interrupts after the wrapper has
been suspended.

Fixes #8278

Pull request type

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

CC @c1728p9 @mprse @jamesbeyond

@fkjagodzinski fkjagodzinski mentioned this pull request Sep 28, 2018
Merged
@jeromecoutant jeromecoutant mentioned this pull request Oct 2, 2018
Closed
jamesbeyond
jamesbeyond approved these changes Oct 2, 2018
View reviewed changes
Copy link
Contributor

@jamesbeyond jamesbeyond left a comment

Choose a reason for hiding this comment

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

Looks good to me. but still need @c1728p9 to review and confirm.

c1728p9
c1728p9 suggested changes Oct 2, 2018
View reviewed changes
hal/LowPowerTickerWrapper.cpp
@@ -32,15 +32,7 @@ void LowPowerTickerWrapper::irq_handler(ticker_irq_handler_type handler)
{
core_util_critical_section_enter();

if (_suspended) {
if (handler) {
handler(&data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even when suspended interrupts need to be passed through so higher layers can run tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please note the interrupts are passed as before. I moved _suspended to the next if statement to have the flags cleared (i.e. _pending_match).

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I didn't realize the interrupt was still getting through.

@jeromecoutant jeromecoutant mentioned this pull request Oct 4, 2018
Closed
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 4, 2018

As there are other 2 dependencies on this fix, please review the latest review

@fkjagodzinski fkjagodzinski mentioned this pull request Oct 8, 2018
Closed
@fkjagodzinski
Copy link
Member Author

@c1728p9, could you take another look please? All this patch does, is to actually disable the use of us ticker when the wrapper is suspended, as the docs say:

mbed-os/hal/mbed_lp_ticker_wrapper.h

Lines 52 to 57 in 19190d6

* Suspend the wrapper layer code
*
* Pass through all interrupts to the low power ticker and stop using
* the microsecond ticker.
*/
void lp_ticker_wrapper_suspend(void);

c1728p9
c1728p9 reviewed Oct 16, 2018
View reviewed changes
hal/LowPowerTickerWrapper.cpp Outdated
@@ -271,13 +263,15 @@ void LowPowerTickerWrapper::_schedule_match(timestamp_t current)
return;
}

if (!too_close) {
if (!too_close || _suspended) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than checking for _suspended here I recommend updating LowPowerTickerWrapper::read and LowPowerTickerWrapper::set_interrupt to check for _suspended can call straight into the real interface. That way it is clear in the code that when this is suspended everything is passed through directly.

If you make this change you'll also want to add a delay in LowPowerTickerWrapper::resume similar to the one in LowPowerTickerWrapper::suspend to ensure any ongoing hardware writes have taken effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@fkjagodzinski
Fix LowPowerTickerWrapper operation when suspended
023603f 
Update the LowPowerTickerWrapper class logic to stop using its internal
Timeout object for scheduling LP ticker interrupts after the wrapper has
been suspended.

Fixes ARMmbed#8278
@fkjagodzinski
lp ticker wrapper: Add undefined behavior warning
effabc5
@fkjagodzinski
Copy link
Member Author

Rebased and added a warning about undefined behavior.

@c1728p9 , I updated the code as you suggested. Is that ok?

@cmonr
Copy link
Contributor

cmonr commented Oct 25, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 25, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Oct 26, 2018

Exporter Build : SUCCESS

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

@mbed-ci
Copy link

mbed-ci commented Oct 26, 2018

Test : SUCCESS

Build number : 3258
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8279/3258

@cmonr cmonr merged commit 3f5eeac into ARMmbed:master Oct 26, 2018
@fkjagodzinski fkjagodzinski deleted the fix-lp_ticker_wrapper-suspend branch October 29, 2018 08:36
@jeromecoutant jeromecoutant mentioned this pull request Nov 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c1728p9 c1728p9 c1728p9 approved these changes

@jamesbeyond jamesbeyond jamesbeyond approved these changes

@mprse mprse Awaiting requested review from mprse

Assignees
No one assigned
Labels
release-version: 5.10.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fkjagodzinski @0xc0170 @cmonr @mbed-ci @c1728p9 @jamesbeyond