[HAL] Add support for 64 bit us timestamp #4094

pan- · 2017-03-31T14:28:08Z

Description

This patch add support of 64 bit us timestamp in the HAL ticker API.
As a result it is possible to schedule events in more than 2147 seconds as it was previously the case.
The class Ticker, Timer and Timeout takes advantage of this new possibility.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

pan- · 2017-03-31T14:29:19Z

@0xc0170 @geky @c1728p9 Could you review this PR.
It includes tests describing in detail the behavior expected by the ticker.

geky · 2017-03-31T16:17:51Z

drivers/Timer.cpp

@@ -46,32 +46,39 @@ void Timer::stop() {

 int Timer::read_us() {


Do you think we can get away with updating this return type?

That's an excellent question. I don't have the answer, I choose a conservative approach for this PR even if it hurts to see this return type be int.

geky · 2017-03-31T16:21:27Z

drivers/Timer.cpp

+    us_timestamp_t time = _time + slicetime();
+    core_util_critical_section_exit();
+    time = time / 1000;
+    return (float)read_ms() / 1000.0f;


It looks like time does not get used. Also is this able to return us precision? or is that out of the range of 32-bit floats?

@pan- : time not used?

geky · 2017-03-31T16:31:18Z

drivers/Timer.cpp

 }

 int Timer::read_ms() {
-    return read_us() / 1000;
+    core_util_critical_section_enter();
+    us_timestamp_t time = _time + slicetime();


tangent: The expression _time + slicetime() is repeated quite a bit. Should we actually be adding _time in the slicetime function?

geky · 2017-03-31T16:34:17Z

drivers/TimerEvent.cpp

@@ -44,6 +44,10 @@ void TimerEvent::insert(timestamp_t timestamp) {
    ticker_insert_event(_ticker_data, &event, timestamp, (uint32_t)this);


Is TimerEvent::insert used at all now?

It is a public interface, so it might be used by user code.

geky · 2017-03-31T16:38:24Z

hal/mbed_ticker_api.c

 #include <stddef.h>
 #include "hal/ticker_api.h"
 #include "platform/mbed_critical.h"

-void ticker_set_handler(const ticker_data_t *const data, ticker_event_handler handler) {
-    data->interface->init();
+#define MBED_MIN(x,y) (((x)<(y))?(x):(y))


thought: Should this be static inline to avoid side-effects becoming duplicated?

geky · 2017-03-31T18:50:21Z

This is looking great 👍, Left some review comments.

Another thought (perhaps this should be a separate pr), some devices (NCS36510 for example) only provide a 16-bit timer. Do you think this could be extended to support timers with any bit-width?

pan- · 2017-03-31T19:54:18Z

@geky Yes it would be really easy to support timer with any bit width if the interface provide this information.

To support a 16 bit timer with the current code. The interface just have to keep state of its counter as a 32 bit counter. The interface can trigger interrupt whenever it wants (at every overflow for instance) without tracking the time requested because the ticker code supports spurious interrupts.

geky · 2017-03-31T21:14:20Z

Just tried this pr with a 16-bit timer and it worked perfectly! This is a great update to the api.

Here's what the NCS36510 16b timer code looks like after this pr: geky@492b558

0xc0170

+1 for the tests, extensive, have to have another look at them :-)

Just general comments.

0xc0170 · 2017-04-07T12:13:14Z

hal/mbed_ticker_api.c

+ * 
+ * It is important to note that the result will **never** be in the past. If the 
+ * value of the low res timetamp is less than the low res part of the reference 
+ * timestamp then an overflow is 


then this is overflow or how a user should read this (looks like unfinished sentence).

0xc0170 · 2017-04-07T12:15:38Z

hal/mbed_ticker_api.c

+ * @param ref: The timestamp of reference. 
+ * @param relative_timestamp: The timestamp to convert. 
+ */
+static us_timestamp_t convert_relative_timestamp(us_timestamp_t ref, timestamp_t relative_timestamp) { 


style: fix the body { (newline) in some functions in this file (at least new functions, we should fix this file later). plus bool is misaligned below?

0xc0170 · 2017-04-07T12:21:03Z

drivers/Timer.cpp

+    us_timestamp_t time = _time + slicetime();
+    core_util_critical_section_exit();
+    time = time / 1000;
+    return (float)read_ms() / 1000.0f;


@pan- : time not used?

0xc0170 · 2017-04-07T12:34:29Z

hal/ticker_api.h

+ * @note prefer the use of ticker_insert_event_us which allows registration of
+ * absolute timestamp.
+ *
+ * @param data      The ticker's data


data = ticker, plus update the description

0xc0170 · 2017-04-07T12:34:52Z

hal/ticker_api.h

+ *
+ * @param data      The ticker's data
+ * @param obj       The event object to be inserted to the queue
+ * @param timestamp The event's timestamp


relative timestamp in this case

0xc0170 · 2017-04-07T13:10:41Z

hal/mbed_ticker_api.c


    /* Go through all the pending TimerEvents */
    while (1) {
-        if (data->queue->head == NULL) {
-            // There are no more TimerEvents left, so disable matches.
-            data->interface->disable_interrupt();


are we disabling interrupts anywhere? there are 3 removals for disable_interrupt() call. Is this function becoming redundant ? How us ticker/lp ticker achieve the same result (no events - nothing happening) - this question is regarding their implementations, if they do something similar (they do not assume upper layer disables interrupts) ?

Interrupts cannot be disabled because the ticker interface doesn't expose anything to monitor overflow of the counter.
The workaround is to track overflow at this layer but it requires to have interrupts always on.

The workaround is to track overflow at this layer but it requires to have interrupts always on.

as in - if none, schedule max delta as it is in update_interrupts, and count overflows here in higher layer. I got it now

I will add a block of documentation detailing the algorithm and how we keeps track of overflow.

0xc0170 · 2017-04-07T13:11:26Z

hal/mbed_ticker_api.c

+}
+
+/**
+ * update the current timestamp value of a ticker.


when we are supposed to call this ? Whats the requirement?

User is not supposed to call this function, it is private (static). It can be called whenever the us_timestamp_t held by a ticker_data_t has to be updated.

My question was from the developer view, once I am about to edit this file, I was not clear when this function is supposed to be called.

0xc0170 · 2017-04-07T13:13:35Z

hal/mbed_ticker_api.c

+ * interrupt will be set to MBED_TICKER_INTERRUPT_TIMESTAMP_MAX_DELTA us from now. 
+ * Otherwise the interrupt will be set to head->timestamp - queue->timestamp us.
+ */
+static void update_interrupt(const ticker_data_t *const ticker) { 


update_interrupt vs set_interrupt? Might find a better name for this function?

I believe this is the most important function in this changeset. Where is it then handled that MAX_DELTA is still less than 64bit (if there's event lets say 46bit timestamp set). I assume in the irq handler. I yet have not seen this documented, might be worth adding this here? Something to extend if set_interrupt(MAX_DELTA) then the remaining part is handled ... somewhere... somehow...

update_interrupt and set_interrupt are really different and doesn't operate on the same object

update_interrupt(const ticker_data_t *const ticker) update the interrupt driving a ticker_data_t object. The update depends on the state of the object and is encapsulated in the function which is not part of the public interface.

ticker_interface_t::set_interrupt(timestamp_t) set a specific, given timestamp in a ticker_interface.

Do you think of a better name ?

Where is it then handled that MAX_DELTA is still less than 64bit (if there's event lets say 46bit timestamp set). I assume in the irq handler. I yet have not seen this documented, might be worth adding this here? Something to extend if set_interrupt(MAX_DELTA) then the remaining part is handled ... somewhere... somehow...

I'm not sure of fully understand what you want. Can you explain a bit more what you want ?

Do you think of a better name ?

Set new match interrupt ? It might not be match in some sense... or just leave it, I was just not clear what that function does from the name

I'm not sure of fully understand what you want. Can you explain a bit more what you want ?

I'll get back to this, have to reread the implementation again

0xc0170 · 2017-04-07T13:16:26Z

hal/mbed_ticker_api.c

+    // update the current timestamp
+    update_current_timestamp(ticker);
+
+    // filter out timestamp in the past 


what does this condition handle - if timestamp to be set is less than the time "now" (thats how we cal this in the comments above) ? might be worth adding a comment or its just me

I'm not sure of the desirable behavior here: the event can either be dropped and forget or added to the queue and the queue scheduled immediately.

@0xc0170 @geky Thoughts ?

added to the queue and the queue scheduled immediately

I would vote for this, no events are forgotten

+1 for not forgetting events

0xc0170 · 2017-04-07T13:18:05Z

hal/mbed_ticker_api.c

+}
+
+void ticker_insert_event_us(const ticker_data_t *const ticker, ticker_event_t *obj, us_timestamp_t timestamp, uint32_t id) { 
+    /* disable interrupts for the duration of the function */


prefer to have a reason why you are disabling interrupts, its clear we enter/exit critical section. same for the comment above (update the current timestamp - function name is the same to a reader). A reader is interested why there's a need for critical section - what we are protecting here and why

Ticker API is thread safe and interrupt safe therefore access to the ticker has to be protected from external modifications.

I was just referring there that the comment either can be removed (obvious that theres critical enter/exit) or we specify a reason why its under critical section. same for the below (update the current time stamp - function name says it).

0xc0170 · 2017-04-07T13:28:07Z

One question - with this work, if we ever get HAL tickers to count overflows, wont be much work to add it in (hypothetically speaking) ? now that we have 64bit timestamp

pan- · 2017-04-07T14:07:42Z

If the ticker interface provides a function returning the number of time the counter has overflowed then it may simplify the code in mbed_ticker_api.c and won't be hard to integrate.

theotherjimmy · 2017-04-24T15:17:56Z

bump @pan- Status?

theotherjimmy · 2017-05-08T16:02:39Z

@pan- Could you rebase? and maybe tell us what's up with this PR (status).

- A new 64 timestamp type has been added: us_timestamp_t. - Changed type of timestamp in ticker_events iinto us_timestamp_t. - Event queue now have a to store a current, absolute timestamp. - Add alternative versions of ticker_insert_event and ticker_read which accept in input us_timestamp_t. - Add documentation explaining the limitation and behavior of ticker_read and ticker_insert_event.

The Timer implementation has been refactored to take advantage of this implementation as the base for all functions reading the time elapsed.

* update_interrupt renamed into schedule_interrupt * update_current_timestamp renamed into update_present_time * ticker_event_queue_t::timestamp renamed into * ticker_event_queue_t::present_time * Fix doxygen comments in ticker_api.h * Update comments internal comments in mbed_ticker_api.c

pan- · 2017-05-16T09:45:44Z

@0xc0170 @geky I think I've addressed your comments, please review my changes. (1, 2, 3 and 4).
@theotherjimmy PR rebased against master.

sg- · 2017-05-26T22:31:25Z

Lets throw a nightly on this to pickup and run the newly added tests against alllll the boards.

/morph test-nightly

mbed-bot · 2017-05-27T04:29:20Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 363

Test failed!

0xc0170 · 2017-05-30T08:14:18Z

@pan- Can you have a look a the failures? NCS fails ticker tests with this changeset?

0xc0170 · 2017-05-30T15:34:59Z

-+--------+--------+--------------------+
| NCS36510-GCC_ARM | NCS36510      | tests-mbed_hal-lp_ticker | 1ms lp_ticker          | 1
 | 0      | OK     | 1.05               |
| NCS36510-GCC_ARM | NCS36510      | tests-mbed_hal-lp_ticker | 1s lp_ticker           | 1
 | 0      | OK     | 1.04               |
| NCS36510-GCC_ARM | NCS36510      | tests-mbed_hal-lp_ticker | 1s lp_ticker deepsleep | 1
 | 0      | OK     | 1.07               |
| NCS36510-GCC_ARM | NCS36510      | tests-mbed_hal-lp_ticker | 1s lp_ticker sleep     | 1
 | 0      | OK     | 1.04               |
| NCS36510-GCC_ARM | NCS36510      | tests-mbed_hal-lp_ticker | 500us lp_ticker        | 0
 | 1      | FAIL   | 0.15               |
| NCS36510-GCC_ARM | NCS36510      | tests-mbed_hal-lp_ticker | 5s lp_ticker           | 1
 | 0      | OK     | 5.04               |
+------------------+---------------+--------------------------+------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 1 FAIL / 5 OK
```

All OK on latest master. same results for IAR, thus al 3 toolchains have the same problem. investigating with Vincent

insert_absolute. With this patch, event insertion should be more precise.

sg- · 2017-06-01T03:33:12Z

any updates? @pan- @0xc0170

* Initialization clear interrupt status * Remove state in management of interrupt * Handle timestamp in the past * Handle current seconds, even if out of the relative timestamp. * Simplify interrupt handling logic.

pan- · 2017-06-01T15:45:28Z

@sg- @geky @0xc0170 The fault was on the NCS36510 low power ticker driver, the lp ticker logic was flawed, didn't handle many cases (immediate event, conversion timestamp into seconds counter, ...) and didn't prevented invalid write of the peripheral registers.
I've rewritten a good chunk of it. All tests should be green now on NCS36510.

studavekar · 2017-06-01T19:17:05Z

/morph test-nightly

mbed-bot · 2017-06-01T22:34:35Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 406

All builds and test passed!

When initializing the RTC on Kinetis devices, handle the case where the time overflow interrupt is pending and the case where the time alarm flag is pending. These flags persist across reset and if not handled will cause a crash when powering up the low power ticker. This problem manifested as a lp_ticker test failure on the K22F and K64F on CI only when running a nightly. This problem has been present but was made obvious by PR ARMmbed#4094 which configures all tickers to interrupt at least every MBED_TICKER_INTERRUPT_TIMESTAMP_MAX_DELTA (~31 minutes). This caused the RTC alarm to fire 31 minutes after the lp_ticker or lp_timeout test and caused the next run of the lp_ticker test to crash on boot.

When initializing the RTC on Kinetis devices, handle the case where the time overflow interrupt is pending and the case where the time alarm flag is pending. These flags persist across reset and if not handled will cause a crash when powering up the low power ticker. This problem manifested as a lp_ticker test failure on the K22F and K64F on CI only when running a nightly. This problem has been present but was made obvious by PR #4094 which configures all tickers to interrupt at least every MBED_TICKER_INTERRUPT_TIMESTAMP_MAX_DELTA (~31 minutes). This caused the RTC alarm to fire 31 minutes after the lp_ticker or lp_timeout test and caused the next run of the lp_ticker test to crash on boot.

LDong-Arm · 2020-12-08T10:27:47Z

drivers/TimerEvent.h

@@ -44,9 +44,12 @@ class TimerEvent {
    // The handler called to service the timer event of the derived class
    virtual void handler() = 0;

-    // insert in to linked list
+    // insert relative timestamp in to linked list


I guess this was meant to be "relative to the 32-bit cycle" rather than "relative current time". But later contributors assumed the later, and it's only addressed by #14005.

pan- changed the title ~~Add support for 64 bit us timestamp~~ [HAL] Add support for 64 bit us timestamp Mar 31, 2017

pan- requested review from geky, c1728p9 and 0xc0170 March 31, 2017 14:44

geky reviewed Mar 31, 2017

View reviewed changes

geky approved these changes Mar 31, 2017

View reviewed changes

pan- mentioned this pull request Apr 5, 2017

nRF52 timer problem #3857

Closed

0xc0170 reviewed Apr 7, 2017

View reviewed changes

0xc0170 added the needs: work label Apr 10, 2017

pan- added 11 commits May 16, 2017 10:35

[HAL] Add Tests for the hal ticker API.

b1f3aa7

[HAL] Add support of 64 bits timestamp in ticker API implementation.

1057720

[HAL] Ensure us_ticker and lp_ticker queue are correctly initialized.

e119357

[Driver] Add API to support insertion of us_timestamp_t in TimerEvent.

c90a3a0

[Drivers] Let ticker support long us timestamp.

abb0a65

[Driver] Improve timer precision for ms and seconds.

15327ca

Timer: add a function to read the timer in a 64bit variable.

3b6c00b

The Timer implementation has been refactored to take advantage of this implementation as the base for all functions reading the time elapsed.

ticker api: format code to meet mbed standards.

c20a1cc

ticker api: Schedule immediately event in the past.

ab09a17

pan- force-pushed the fix_hal_ticker branch from 251627a to ab09a17 Compare May 16, 2017 09:43

0xc0170 added needs: work and removed ready for merge labels May 30, 2017

adbridge added the release-version: 5.5.0-rc1 label May 30, 2017

pan- added 3 commits May 31, 2017 19:13

lp_ticker test: Capture completion time at completion point.

3e24e8f

TimerEvent: Use ticker_insert_event_us instead of the old API in

f52da48

insert_absolute. With this patch, event insertion should be more precise.

Ticker NCS36510: Fix tests of the status register.

6fe07c6

pan- added 2 commits June 1, 2017 16:31

lp ticker test: Fix timestamp update during deepsleep test.

c6433b0

NCS36510 RTC driver: Fix driver.

045b026

* Initialization clear interrupt status * Remove state in management of interrupt * Handle timestamp in the past * Handle current seconds, even if out of the relative timestamp. * Simplify interrupt handling logic.

sg- added needs: CI and removed needs: work labels Jun 1, 2017

sg- added ready for merge and removed needs: CI labels Jun 2, 2017

sg- merged commit fa0cd20 into ARMmbed:master Jun 2, 2017

sg- removed the ready for merge label Jun 2, 2017

0xc0170 mentioned this pull request Jun 5, 2017

Fix corner cases in STM32 16bit tickers #4424

Merged

c1728p9 mentioned this pull request Jun 5, 2017

Safely initialize RTC on kinetis devices #4453

Merged

geky mentioned this pull request Jun 30, 2017

ncs36510: us ticker improvements #4679

Merged

pan- deleted the fix_hal_ticker branch November 14, 2018 10:50

LDong-Arm reviewed Dec 8, 2020

View reviewed changes

		@@ -46,32 +46,39 @@ void Timer::stop() {

		int Timer::read_us() {

		@@ -44,6 +44,10 @@ void TimerEvent::insert(timestamp_t timestamp) {
		ticker_insert_event(_ticker_data, &event, timestamp, (uint32_t)this);

[HAL] Add support for 64 bit us timestamp #4094

[HAL] Add support for 64 bit us timestamp #4094

Uh oh!

Conversation

pan- commented Mar 31, 2017

Description

Status

Migrations

Uh oh!

pan- commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Mar 31, 2017

Uh oh!

pan- commented Mar 31, 2017

Uh oh!

geky commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- commented Mar 31, 2017 •

edited

Loading

pan- Mar 31, 2017 •

edited

Loading

geky commented Mar 31, 2017 •

edited

Loading

pan- commented Jun 1, 2017 •

edited

Loading