Merge feature-hal-spec-ticker to master #6995
Closed
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
kjbracey
previously approved these changes
May 24, 2018
There was a problem hiding this comment.
Looks good.
My only reservation is that this is great as HAL-level improvement, but leaves an existing problem at app layer.
We've been busy moving code away from using Ticker or LowPowerTicker because it's just a pain to do "if some config use LPTicker else Ticker". And due to uncertainty of LP ticker quality the "config" has always been manual, not ifdef DEVICE_LOWPOWERTIMER. If the quality issue has been solved, there should really be an alias hook somewhere for LowResTimer that provides one or the other depending on HAL availability. That's what most people need, not Timer, but they tend to use Timer and block deep sleep because it's the easy obvious function. (See also wait and its microsecond precision).
Even then, people in the RTOS don't usually need either - they can use the RTOS's tick count. So that's what we've been moving things like the EventQueue to using.
| * \defgroup hal_lp_ticker Low Power Ticker | ||
| * Low level interface to the low power ticker of a target | ||
| * | ||
| * # Defined behavior |
There was a problem hiding this comment.
These specifications are great, but the rough outline should be repeated in the C++ classes so people know what performance to expect.
jeromecoutant
previously approved these changes
May 24, 2018
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: #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.
This will free up the LPTMR to be used as lpticker Signed-off-by: Mahesh Mahadevan <[email protected]>
This will free up the LPTMR to be used as lpticker Signed-off-by: Mahesh Mahadevan <[email protected]>
This will free up the LPTMR to be used as lpticker Signed-off-by: Mahesh Mahadevan <[email protected]>
Signed-off-by: Mahesh Mahadevan <[email protected]>
Use only the GPT module and avoid using RTC. Signed-off-by: Mahesh Mahadevan <[email protected]>
It is possible that the difference between base and next tick count on some platforms is greater than 1, in this case we need to repeat counting with the reduced number of cycles (for slower boards). If number of cycles has been reduced than base tick count needs to be redefined. This operation is missing and is added by this patch.
Test inserts event into the TimerEvent object which is scheduled 50 ms in the future. Then test thread hangs on the semaphore for 51 ms and after that time we expect that event handler has been executed. This test fails sometimes on NRF51_DK, NRF52_DK since different clocks are used for event handling and delay. TimerEvent class uses fast us ticker and semaphores are based on lp ticker (NRF51_DK) or System Tick Timer (NRF52_DK). We assume that ticker measurement error is 10% and our 1 [ms] extra corresponds to 2% error. I suggest to increase delay to 2 [ms] (4%). This should be enough for all boards at this moment.
Represent tolerance value as it is done in `tests-mbed_drivers_timer`. Use 5% tolerance for lp ticker.
NRF52_DK is now based on fast and accurate 1MHz counter.
This function should perform instruction cycles count in critical section. Additionally remove redundant code.
This functionality is required by new sleep standards.
bulislaw
force-pushed
the
feature-hal-spec-ticker
branch
from
8bbe654 to
9fdb5c2
Compare
|
Removed labeling to make 5.9 tags easier to sort through. Will be brought in with #7009. |
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
Bring new USTICKER and LPTICKER HAL APIs to master.
Pull request type
@cmonr