timer: handle timer overflow #395
Conversation
5693cc7 to
f0b6210
Compare
|
Following on Tyler's comment, it would be nice to capture the more complete testing program in the |
71cb726 to
9ca0b9c
Compare
examples/tests/timer_overflow/main.c
Outdated
There was a problem hiding this comment.
I don't understand what this test does. Setting a timer for 2 seconds will work just fine.
I think having to modify the libtock library is a high burden for running this test. I think it would be better to calculate how long until a overflow will happen, and set a timer for longer than that.
We then only run this test on a kernel with a very fast clock.
There was a problem hiding this comment.
That sounds reasonable.
I have 2 questions:
- How do we know the number of milliseconds that it will take to overflow? We know that overflows happen after
$2^{32}$ ticks, however the number of milliseconds that takes depends on the frequency of the clock. We can query this value for the purpose of the test usingalarm_internal_frequencyfunction, but we were told not to use thealarm_internal_*functions in our testing code. We could hard code this value for the NRF52840dk board or just set a timer for an absurdly large number (like$2^{32}$ ms) likely overflow. - How can a user customize their kernel to run with a fast clock? Does the kernel already allow users to customize the clock speed?
There was a problem hiding this comment.
-
Once the left aligned frequency changes are merged (capsules/alarm: left-justify 32 bit ticks, re-architect alarm, add unit tests tock#3975), we will be able to determine the wrap point. I don't think we should block on waiting for that PR. For now, I think it's reasonable to just set this to some "large value" i.e. 10 minutes and place a comment saying this time value is board specific.
-
Yes, this can be configured within the
main.rsinitialization.
6277a89 to
568c4e2
Compare
examples/tests/timer_overflow/main.c
Outdated
There was a problem hiding this comment.
-
Once the left aligned frequency changes are merged (capsules/alarm: left-justify 32 bit ticks, re-architect alarm, add unit tests tock#3975), we will be able to determine the wrap point. I don't think we should block on waiting for that PR. For now, I think it's reasonable to just set this to some "large value" i.e. 10 minutes and place a comment saying this time value is board specific.
-
Yes, this can be configured within the
main.rsinitialization.
libtock/alarm_timer.c
Outdated
| // (`frequency` (ticks per second) / `1000` milliseconds per second) | ||
| // The division is done this way because of the same argument in | ||
| // `ms_to_ticks`. | ||
| milliseconds += (leftover_ticks * milliseconds_per_second) / frequency; |
There was a problem hiding this comment.
leftover_ticks is at most equal to frequency. For frequency values larger than 4MHz, I believe this calculation will overflow. I do not think there is a way around having a uint64_t to handle the intermediate calculations.
Using a uint64_t would allow us to support up to and beyond GHz frequencies.
There was a problem hiding this comment.
I'm just browsing through all this, and I think I agree with the logic here. Using uint64_t would also allow us to simply the logic here a bunch, and remove the split between seconds and fractional seconds, effectively making this equivalent to: ticks_to_ms = (ticks * 1000) / freq.
I'm wondering how much the split into seconds and fractional seconds buys us here in terms of performance in code size, vs. 64-bit arithmetic on a 32-bit platform.
There was a problem hiding this comment.
Nevermind, I just saw that the new, merged PR has been changed to 64-bit, my bad. Part of my comment still applies, I suppose: https://github.com/tock/libtock-c/pull/434/files#r1642913413
libtock/alarm_timer.c
Outdated
| // WARNING: This function can silently overflow if the output | ||
| // number of ticks is larger than `UINT32_MAX`. |
There was a problem hiding this comment.
This does not seem ideal considering that libtock-c assumes:
* The client should not assume anything about the underlying clock used by an
* implementation other than that it is running at sufficient frequency to
* deliver at least millisecond granularity and that it is a 32-bit clock (i.e.
* it will wrap at 2^32 clock ticks).
Perhaps it would be better to pass a uint32_t reference that this function updates and instead returns a returncode?
|
Considering this PR has stalled for a bit, I propose that we split this into two PRs:
We can keep this PR as (2) blocking on the new PR (1). |
This helper function has been thoroughly debugged in tock#395. Floating point imprecision was causing timer overflows to work incorrectly. `ms_to_ticks` now better corresponds with the behavior of the libtock-c 32-bit tick rollover and is precise to 1ms of error.
This helper function has been thoroughly debugged in tock#395. Floating point imprecision was causing timer overflows to work incorrectly. `ms_to_ticks` now better corresponds with the behavior of the libtock-c 32-bit tick rollover and is precise to 1ms of error.
This helper function has been thoroughly debugged in tock#395. Floating point imprecision was causing timer overflows to work incorrectly. `ms_to_ticks` now better corresponds with the behavior of the libtock-c 32-bit tick rollover and is precise to 1ms of error.
libtock/services/alarm.h
Outdated
| /** \brief Opaque handle to a repeating alarm. | ||
| * | ||
| * An opaque handle to a repeating alarm created by `libtock_alarm_repeating_every`. | ||
| * An opaque handle to an alarm created by `libtock_alarm_repeating_every_ms` | ||
| * and `libtock_alarm_in_ms`. | ||
| */ | ||
| typedef struct alarm_repeating { | ||
| uint32_t interval; | ||
| libtock_alarm_callback cb; | ||
| void* ud; | ||
| typedef struct alarm_data { | ||
| // Length of timer in milliseconds. | ||
| uint32_t interval_ms; | ||
| // Number of times the underlying counter will overflow | ||
| // before hitting the target interval_ms. | ||
| uint32_t overflows_left; | ||
| // Number of ticks remaining after the last time the | ||
| // counter overflows. | ||
| uint32_t remaining_ticks; | ||
| libtock_alarm_callback callback; | ||
| void* user_data; | ||
| libtock_alarm_t alarm; | ||
| } libtock_alarm_repeating_t; | ||
| } libtock_alarm_data_t; |
There was a problem hiding this comment.
Why does this modify libtock_alarm_repeating_t and not libtock_alarm_t? Couldn't a single alarm also have an overflow?
There was a problem hiding this comment.
libtock_alarm_at sets a single alarm and takes in a 32 bit argument (dt) for the length of the alarm in ticks. This cannot overflow as the kernel guarantees that the alarm buffer will not overflow sooner than 32 bits of ticks. Only alarms longer than libtock_alarm_in_ms.
There was a problem hiding this comment.
Ok that makes sense.
What about renaming libtock_alarm_t to something like libtock_alarm_dt_t, creating a new libtock_alarm_t that supports overflows, and leaving libtock_alarm_repeating_t as it is? It would be nice to not have to change all of the examples and to keep the more descriptive "repeating" name. Changing the type for libtock_alarm_at() seems the least invasive since it isn't really used.
There was a problem hiding this comment.
I have restored the libtock_alarm_t name to make the renaming less invasive. We cannot keep the libtock_alarm_repeating_t unchanged for a few reasons:
libtock_alarm_repeating_tsets repeating alarms in milliseconds, so it must support overflows. Thus we must add extra fields to support the new overflow logic.libtock_alarm_repeating_every_mscallslibtock_alarm_in_mswith the alarm struct it was given. To support the less intrusive naming, both must be the same type namelibtock_alarm_t. As the structlibtock_alarm_repeating_tis not relevant to the user calling these functions, I do not think it is more helpful to include the "repeating" moniker. This is why I believe there should only be two structs and why the name of therepeatingstruct must be changed.
cf70666 to
e294786
Compare
|
@hudson-ayers Do you think there is any reason to keep the |
|
I think it is fine to get rid of |
e294786 to
189416d
Compare
Tock does not support setting alarms more than 2^32 ticks in the future as the `alarm_at` syscall takes in ticks. For the default nrf52840 tock configuration at 32KHz, the greatest possible value for the syscall alarm is 37 hours. `alarm_in` takes in milliseconds, but is flawed in that it is just a wrapper around `alarm_at` which can only accept a max value of 2^32 ticks. Thus, calling `alarm_in` longer than 2^32 ticks in milliseconds will not have the expected behavior. `delay_ms` calls `alarm_in`, so it is flawed for the same reason. We fixed this by modifying `alarm_in`. If the timer length is longer than the time that `alarm_at` can count up to, then `alarm_in` will schedule multiple alarms to reach the full length. We calculate the number of full overflows and the remainder ticks to reach the target length of time. The overflows use the `overflow_callback` for each intermediate overflow. Co-Authored-By: Anthony Tarbinian <atar13@users.noreply.github.com>
This tests the libtock alarm implementation's ability to handle situations where setting a alarm would overflow the underlying hardware counter. Co-Authored-By: Anthony Tarbinian <atar13@users.noreply.github.com>
Renamed libtock_alarm_repeating_t to libtock_alarm_t since the previous name suggested it was only used by libtock_alarm_repeating_ms when it was also used by libtock_alarm_in_ms. This new name is more general and can apply to either function. This must be changed to support the new fields for overflow. Co-authored-by: Samir Rashid <Samir-Rashid@godsped.com>
189416d to
b9cdf9e
Compare
PR fixes tock/tock#3879
Tock does not support setting alarms more than 2^32 ticks in the future as the
alarm_atsyscall takes in ticks. For the default nrf52840 tock configuration at 32KHz, the greatest possible value for the syscall alarm is 37 hours.timer_intakes in milliseconds, but is flawed in that it is just a wrapper aroundalarm_atwhich can only accept a max value of 2^32 ticks. Thus, callingtimer_inlonger than 2^32 ticks in milliseconds will not have the expected behavior.delay_mscallstimer_in, so it is flawed for the same reason.We fixed this by modifying
timer_in. If the timer length is longer than the time thatalarm_atcan count up to, thentimer_inwill schedule multiple alarms to reach the full length. We calculate the number of full overflows and the remainder ticks to reach the target length of time. The overflows use theoverflow_callbackfor each intermediate overflow.Testing
We verified this code works by creating a test program
timer_overflowthat simulates setting a timer longer than the threshold of what would overflow and verifies the total timer length was correct. To test overflow you need to edit#define MAX_TICKSinalarm_timer.cto overflow in a reasonable amount of time. This tests the overflow logic, which works correctly.