Changes required by the ST low power ticker wrapper. #10536
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mprse, thank you for your changes. |
|
Added fix for STM low level lp ticker wrapper. |
adbridge
approved these changes
May 17, 2019
|
@ARMmbed/mbed-os-hal could we have a review please? |
fkjagodzinski
approved these changes
May 24, 2019
TESTS/mbed_hal/sleep/main.cpp
Outdated
| /* On some targets like STM family boards with LPTIM enabled there is a required delay (~100 us) before we are able to | ||
| reprogram LPTIM_COMPARE register back to back. This is handled by the low level lp ticker wrapper which uses LPTIM_CMPOK interrupt. | ||
| CMPOK fires when LPTIM_COMPARE register can be safely reprogrammed again. This means that on these platforms we have additional interrupt | ||
| (CMPOK) fired always ~100 us after programming lp ticker. Since this interrupt wake-ups the board from the sleep we need to go to sleep |
There was a problem hiding this comment.
Suggested change
| (CMPOK) fired always ~100 us after programming lp ticker. Since this interrupt wake-ups the board from the sleep we need to go to sleep | |
| (CMPOK) fired always ~100 us after programming lp ticker. Since this interrupt wakes up the board from the sleep we need to go to sleep |
|
@mprse I think you just have a typo to fix ... |
mprse
force-pushed
the
stm_lptim_issue
branch
2 times, most recently
from
81edde7 to
1da436d
Compare
mprse
changed the title
On some targets like STM family boards with LPTIM enabled an interrupt is triggered on counter rollover. We need special handling for cases when next_match_timestamp < start_timestamp (interrupt is to be fired after rollover). In such case after first wake-up we need to reset interrupt and go back to sleep waiting for the valid one. On some targets like STM family boards with LPTIM enabled there is a required delay (~100 us) before we are able to reprogram LPTIM_COMPARE register back to back. This is handled by the low level lp ticker wrapper which uses LPTIM_CMPOK interrupt. CMPOK fires when LPTIM_COMPARE register can be safely reprogrammed again. This means that on these platforms we have additional interrupt (CMPOK) fired always ~100 us after programming lp ticker. Since this interrupt wake-ups the board from the sleep we need to go to sleep after CMPOK is handled. Background: There is an errata in LPTIM specification that explains that CMP Flag condition is not an exact match (COUNTER = MATCH) but rather a comparison (COUNTER >= MATCH). As a consequence the interrupt is firing early than expected when programing a timestamp after the 0xFFFF wrap-around. In order to work-around this issue, we implement the below work-around. In case timestamp is after the work-around, let's decide to program the CMP value to 0xFFFF, which is the wrap-around value. There would anyway be a wake-up at the time of wrap-around to let the OS update the system time. When the wrap-around interrupt happen, OS will check the current time and program again the timestamp to the proper value.
Execute test case 100 times in loop since one run is not enough to catch possible failure.
|
Updated PR name and description. |
|
I started CI meanwhile, @ARMmbed/mbed-os-hal should review |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
maciejbocianski
approved these changes
Jun 3, 2019
|
@ARMmbed/team-st-mcd |
Please review. These fixes are not that trivial (at least for me after reviewing changes and their commit msg - very detailed 👍 ). |
LMESTM
approved these changes
Jun 3, 2019
0xc0170
reviewed
Jun 3, 2019
| CMPOK fires when LPTIM_COMPARE register can be safely reprogrammed again. During this period deep-sleep is locked. | ||
| This means that on these platforms we have additional interrupt (CMPOK) fired always ~100 us after programming lp ticker. | ||
| Since this interrupt wakes-up the board from the sleep we need to go to sleep after CMPOK is handled. */ | ||
| TEST_ASSERT_TRUE(sleep_manager_can_deep_sleep_test_check()); |
There was a problem hiding this comment.
how is this handled in the target code? Does not need to be there - scheduler schedule sleep, deep sleep would be locked, ~100us wake up, nothing to do, goes again to sleep - this time deep sleep.
Do I understand this correctly ?
Because seeing this in the common test file raises questions if a target does nto handle this internally and how to write generic sleep handling.
There was a problem hiding this comment.
Because this a HAL test, there is no scheduler involved here I think
Here is how I commented the same for sleep manager test
https://github.com/ARMmbed/mbed-os/pull/10700/files#diff-5d28bad27b5517915180d28407cb8c79R125
There was a problem hiding this comment.
So with scheduler anything above HAL, we are all good and this checks are not needed - will wake up - nothing to do, goes to sleep again (causing one false wake up only).
| We need special handling for cases when next_match_timestamp < start_timestamp (interrupt is to be fired after rollover). | ||
| In such case after first wake-up we need to reset interrupt and go back to sleep waiting for the valid one. | ||
| NOTE: Above comment (CMPOK) applies also here.*/ | ||
| #if MBED_CONF_TARGET_LPTICKER_LPTIM |
There was a problem hiding this comment.
Why this very target specific config (_LPTIM) is here in the test? Is this documented anywhere - this "unwanted" wake up from sleep
There was a problem hiding this comment.
So in the case of CMPOK I was able to use a fact that while waiting on CMPOK, deep-sleep is locked, so I used sleep_manager_can_deep_sleep_test_check(). This is a generic solution for all targets. STM targets which have lp tricker driven by LPTIM will wait on sleep_manager_can_deep_sleep_test_check() about ~100us and for other targets, this extra delay will not be performed.
The extra interrupt on 0xFFFF is the second special case for STM targets which have lp tricker driven by LPTIM, but I couldn't find the generic solution for this case in the test. More information about this workaround can be found in the description of PR #10701:
In particular, the LP TIMER ticker cannot be programmed for a timestamp in the past, so we need to program a wake-up interrupt at the wrap-around (0xFFFF), then the next interrupt will be programmed again.
mprse
commented
Jun 3, 2019
hal/mbed_ticker_api.c
Outdated
| @@ -375,7 +375,7 @@ void ticker_insert_event_us(const ticker_data_t *const ticker, ticker_event_t *o | |||
| obj->next = p; | |||
|
|
|||
| /* if prev is NULL we're at the head */ | |||
| if (prev == NULL) { | |||
| if (prev == NULL || timestamp <= ticker->queue->present_time) { | |||
There was a problem hiding this comment.
I just noticed that this implementation is wrong. Should be something like:
if (prev == NULL) {
ticker->queue->head = obj;
} else {
prev->next = obj;
}
if (prev == NULL || timestamp <= ticker->queue->present_time) {
schedule_interrupt(ticker);
}
I will test this tomorrow and provide the fix.
…as already expired. On some platforms, if low power ticker interrupt is set to very close value (e.g. timestamp < current tick + 3), then interrupt may not fire. This is one use case of lp ticker wrapper, but not all platforms use the wrapper. Some platforms cheat a bit and in this case, simply schedules interrupt a bit later. The problem has been found while working on the low-level lp ticker wrapper for ST boards which run lp ticker using LPTIM. These platforms have such limitation. Failing test: tests-mbed_drivers-lp_timeout (Test Case: Zero delay) In the test scenarion, the lp ticker callback is attached with 0.0 s delay in the loop. The new events are put in the front of the lp ticker event list and interrupt reschedule is performed. Usually, the new event is already expired, interrupt fires immediately and next event from the list is then scheduled (e.g. system tick). When the next event (e.g. system tick) is very close to the current time it might be scheduled a bit later (because of lp ticker limitation). Let's assume that system tick has been delayed by 3 ticks and while inserting new zero delay event, absolute system tick time on the event list has already expired. In this case, zero delay event may be added after the expired system tick event and no reschedule is performed (because the head of the list has not changed). Interrupt also didn't fire yet since it has been delayed, so after return from attach_callback(0) we are still waiting for the delayed interrupt and zero delay callback has not been called instantly. This may also affect other platforms which use such delays (Cypress, NORDIC, etc.). The proposition is to add extra condition while adding an event to the event list. If the inserted event is already expired, then perform reschedule immediately.
|
I have forced pushed a fix to ticker common layer change (#10536 (review)) and since some tests are skipped on CI I run a full test run for all related PRs combined together:
Results below ( BTW I guess that |
|
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR is related to PR #10701 (Stm lp ticker low-level wrapper).
While working on the low-level ticker wrapper the following issues have been found:
tests-mbed_hal-sleepWhen STM low-level lp ticker wrapper is in use an interrupt is triggered on counter rollover. We need special handling for cases when next_match_timestamp < start_timestamp (interrupt is to be fired after the rollover). In such case after first wake-up, we need to reset interrupt and go back to sleep waiting for the valid one.
On some targets like STM family boards with LPTIM enabled there is a required delay (~100 us) before we are able to reprogram LPTIM_COMPARE register back to back.
This is handled by the low-level lp ticker wrapper which uses LPTIM_CMPOK interrupt. CMPOK fires when LPTIM_COMPARE register can be safely reprogrammed again. This means that on these platforms we have additional interrupt (CMPOK) fired always ~100 us after programming lp ticker.
Since this interrupt wake-ups the board from the sleep we need to go to sleep after CMPOK is handled.
tests-mbed_drivers-lp_timeoutIt has been found that
Zero delaytest case sometimes fails. Test case createsLowPowerTimeoutobject and attaches callback with timeout0(no timeout). This test was failing occasionally against ST low-level lp ticker wrapper. To make the test more stable, the proposition is to execute it 100 times in a loop.Ticker common layer: run interrupt reschedule if the inserted event has already expired.On some platforms, if low power ticker interrupt is set to very close value (e.g. timestamp < current tick + 3), then interrupt may not fire. This is one use case of lp ticker wrapper, but not all platforms use the wrapper. Some platforms cheat a bit and in this case, simply schedules interrupt a bit later. The problem has been found while working on the low-level lp ticker wrapper for ST boards which run lp ticker using LPTIM. These platforms have such limitation.
Failing test:
tests-mbed_drivers-lp_timeout (Test Case: Zero delay)In the test scenario, the lp ticker callback is attached with 0.0 s delay in the loop. The new events are put in the front of the lp ticker event list and interrupt reschedule is performed. Usually, the new event is already expired, interrupt fires immediately and next event from the list is then scheduled (e.g. system tick). When the next event (e.g. system tick) is very close to the current time it might be scheduled a bit later (because of lp ticker limitation). Let's assume that system tick has been delayed by 3 ticks and while inserting new zero delay event, absolute system tick time on the event list has already expired. In this case, zero delay event may be added after the expired system tick event and no reschedule is performed (because the head of the list has not changed). Interrupt also didn't fire yet since it has been delayed, so after return from attach_callback(0) we are still waiting for the delayed interrupt and zero delay callback has not been called instantly.
This may also affect other platforms which use such delays (Cypress, NORDIC, etc.).
The proposition is to add extra condition while adding an event to the event list. If the inserted event is already expired, then perform reschedule immediately.
Debug log
tests-mbed_drivers-lp_timeout (Test Case: Zero delay)onNUCLEO_L073RZ:Pull request type
Reviewers
@LMESTM
@c1728p9