Proof of concept lp ticker fix #6473
Conversation
c1728p9
force-pushed
the
lp_ticker_blocking_fix
branch
from
5cb904f to
91a4070
Compare
c1728p9
force-pushed
the
lp_ticker_blocking_fix
branch
from
91a4070 to
b5a30d4
Compare
hal/mbed_lp_ticker_api.cpp
Outdated
| */ | ||
| #include "hal/lp_ticker_api.h" | ||
|
|
||
| #if DEVICE_LPTICKER |
There was a problem hiding this comment.
this should be part of ticker_info_t or should be defined in targets.json rather than here.
c1728p9
force-pushed
the
lp_ticker_blocking_fix
branch
2 times, most recently
from
22ed36e to
07fabfc
Compare
Add documentation and test header for the HAL Ticker API.
…header files. Add one potential bug to the test description. Fix test function names to be consistant with desctiption and test header. Fix typos.
…pecific drivers. Since target specific ticker drivers are not ready also features which uses ticker in upper layers may not work correctly and tests for these features. We need to disable also failing higher level ticker related tests (by adding check if DEVICE_USTICKER symbol is available and raise #error [NOT_SUPPORTED] if not).
Current version: The function ticker_init resets the internal count and disables the ticker interrupt. Proposed version: The function ticker_init allows the ticker to keep counting and disables the ticker interrupt. This is a result of the following discussion: ARMmbed#5233 (review)
The intention was to use ticker_overflow_delta equal to 0 for low power ticker tests and ticker_overflow_delta equal to 50 for high frequency ticker tests. Current implementation is invalid since for devices which provide LOW_POWER_TIMER feature delta is equal to 0 and for devices without this feature delta is equal 50.
- provide ticker configuration: 1MHz/32 bit counter. - us_ticker_init() routine disables interrupts. - adapt comments.
Low power ticker time counter is created based on RTC which is driven by 32KHz clock. Additional low power timer is used to generate interrupts. We need to adapt driver to operate on ticks instead of us. Perform the following updates: - provide lp ticker configuration: 32KHz/32 bit counter. - lp_ticker_init() routine disables lp ticker interrupts . - adapt the driver functions to operate on ticks instead us. - adapt comments. - add us_ticker_free() routine.
Since according to the ticker requirements min acceptable counter size is 12 bits (low power timer) for which max count is 4095, then all test cases must be executed in this time window. HAL ticker layer handles overflow and it is not handled in the target ticker drivers.
On some platforms (e.g. K64F) different counters are used for time measurement and interrupt generation. Because of that we should relax interrupt test case and give additional time before checking if interrupt handler has been executed.
…lts on all compilers count_ticks() function counts ticker ticks elapsed during execution of N cycles of empty while loop. In current version N value (cycles) is given as volatile paramater in order to disable possible compiler optimalisation. There was a problem with measured time on different compilers and additionally results on ARM compiler were unexpected (the difference beetween measured elapsed ticks for the same number of cycles was to large). This might be caused by the memory access in order to store updated variable in memory. To fix this issue given numer of cycles has been stored into register and register is decremented (no memory access). With this fix count_ticks(NUM_OF_CYCLES, 1) call returns 2500 +/-1 for us ticker ticks using each compiler (K64F).
…pecific drivers. Since target specific ticker drivers are not ready also features which uses ticker in upper layers may not work correctly and tests for these features. We need to disable also failing higher level ticker related tests (by adding check if DEVICE_USTICKER symbol is available and raise #error [NOT_SUPPORTED] if not). Note: tests-mbed_drivers-rtc is new and uses us ticker to perform a delay.
Test that lp ticker does not glitch backwards due to an incorrectly implemented ripple counter driver.
That's to match DEVICE_USTICKER.
|
@c1728p9 Sorry, but you're going to have to rebase this one. |
The new HAL allows to share the timer bit width and frequency, the actual handling of mapping 16 bits counter up to 32 bits or 64 bits is now managed by mbed common layer. This makes this ticker layer very similar to 32bits one and much easier than before.
hal/mbed_lp_ticker_api.cpp
Outdated
|
|
||
| #if DEVICE_LOWPOWERTIMER | ||
|
|
||
| #define DELAY_TICKS 3 |
There was a problem hiding this comment.
should this be covered #ifndef ? Or why is this define here only - expected to be overwritten in this file? not outside , in the app or similar?
looking at lines below, it actually can be undefined or set to 0 ?
hal/mbed_lp_ticker_api.cpp
Outdated
| #include "mbed_critical.h" | ||
| static void set_interrupt_wrapper(timestamp_t timestamp); | ||
| #else | ||
| #define set_interrupt_wrapper lp_ticker_set_interrupt |
There was a problem hiding this comment.
Was about to disagree, but actually, I do think this is a bit too sneaky - too easy quickly scanning and searching to assume looking at the ticker_interface_t that the set_interface_wrapper() function visible in this file is always used as .set_interrupt.
Suggest having an obvious .set_interrupt = SET_INTERRUPT_FN, and then that is obviously defined as either set_interrupt_wrapper or lp_ticker_set_interrupt. Or just put an ifdef/else for the two alternatives in the structure definition itself, skipping the define.
|
I split this into 3 separate PRs and moved it to master: If these are merged to master then the |
|
Hi Maybe we can merge this one into feature branch in order to test our own implementation ? @c1728p9 it seems there is some conflict Thx |
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.
Change tickless handling to use public RTX calls and to prevent unnecessary calls to suspend/resume by looping until the kernel needs to be resumed.
Detach in the Timeout::handler so deep sleep is properly unlocked.
c1728p9
force-pushed
the
lp_ticker_blocking_fix
branch
from
512dbb1 to
81591b0
Compare
|
Hi @jeromecoutant I updated this PR to pull in the 3 updated fixes. Is this enough to allow you to test? Ideally I would like this to be used for testing only, and have the final changes come in through master. |
|
I am closing this one. If any update (those 3 PR are not integrated, we can reopen this one) |
|
@jeromecoutant Yup, that can be done as soon as those three PRs get in. We'd rather have to rebase the feature PR instead of managing two sets of fixes. Makes bringing the feature into master much easier. |
|
Reopening for further testing |
When the Timeout object has been setup to perform the next low power ticker write don't write to the low power ticker from lp_ticker_set_interrupt_wrapper. Instead write to it only from set_interrupt_later. This prevents a race condition where the low power ticker is written to in two places.
bulislaw
force-pushed
the
feature-hal-spec-ticker
branch
from
155a9a0 to
61c0007
Compare
|
Closing this PR since all relevant changes have been merged into master. |
Update the lp ticker code to prevent lp_ticker_set_interrupt from being called back-to-back and blocking.