Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alarm overflow - gettimeasticks #366

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Conversation

tyler-potyondy
Copy link
Contributor

@tyler-potyondy tyler-potyondy commented Jan 31, 2024

While working on the OpenThread libtock-c port, @atar13 and @Samir-Rashid observed a bug on the nrf52840dk board attempting to use the gettimeasticks functionality. When tested with the following code:

    // code below abbreviated for demonstration
    for (int i = 0; i < NUM_SAMPLES; i++) {
        uint32_t cur_time_ms = gettimeasticks(&tv, NULL);
        clock_return[i] = cur_time_ms;
        delay_ms(1);
    }

    // print results
    for (int i = 0; i < NUM_SAMPLES; i++) {
        uint32_t item = clock_return[i];
        printf("item %d:\t %lu\n", i, item);  // output wraps around at about 128
    }

they observed that the the time in milliseconds wrapped around to zero approximately every 128 ms (see below)

item 26:         103
item 27:         107
item 28:         111
item 29:         115
item 30:         119
item 31:         123
item 32:         127
item 33:         0
item 34:         3
item 35:         7

Upon looking into this, it appears this is caused by a buffer overflow when performing the microsecond calculation in libtock/alarm_timer.c:

tv->tv_usec = (remainder * 1000 * 1000) / frequency;

The nrf52840DK board is configured to frequency = 32768Hz. The greatest possible remainder value is $remainder = frequency - 1$. Depending on __USE_TIME_BITS64, tv_usec is either uint32_t or uint64_t. In the case of tv_usec being of type unint32_t, the intermediate calculation $(remainder * 1000 * 1000)$ will overflow and result in the timer microsecond value appearing to rollover and reset.

A proposed fix to this (at the loss of ~1us of precision for the frequency = 32768Hz case) is to adjust the microsecond calculation. I detailed this precision concern and overflow concerns for varied frequencies in a comment within the function. This loss of precision only occurs for the frequency value of 32768Hz. All other frequencies defined in the kernel at kernel/src/hil/time.rs (e.g. 1KHz, 16MHz, etc) will not experience this loss of precision as the 3 least significant digits do not encode data.

@alistair23 your input would be appreciated on this since I believe you were using the gettimeasticks function. Please let me know if I overlooked some detail related to other board platforms timer frequency configuration.

Testing this on the nrf52840dk, this implementation now returns the expected behavior and no longer exhibits the "rollover" at 128 ms.

bradjc
bradjc previously approved these changes Feb 1, 2024
@alistair23
Copy link
Contributor

This looks reasonable to me. The CI is failing though

@tyler-potyondy
Copy link
Contributor Author

Formatting CI is passing now, but the build CI is still failing. Looking into the logs it appears to be an issue with ./cxx_hello. Testing locally, cxx_hello is building without issue. Any ideas as to what might be causing this?

@lschuermann
Copy link
Member

Seems that we have a combination of a missing dependency specification (newlib for CXX targets), as well as a race condition in the CI:

libtock-libc++-10.5.0.zip: OK
Unpacking libtock-libc++-10.5.0.zip...
libtock-newlib-4.2.0.20211231.zip: OK
Unpacking libtock-newlib-4.2.0.20211231.zip...
Done upacking libtock-libc++-10.5.0.zip...
 CXX        main.cc
 CXX        main.cc
 CXX        main.cc
 CXX        main.cc
 CXX        main.cc
 CXX        main.cc
main.cc:1:10: fatal error: stdio.h: No such file or directory
    1 | #include <stdio.h>
      |          ^~~~~~~~~
compilation terminated.
make: *** [../../AppMakefile.mk:311: build/rv32imac/main.o] Error 1
make: *** Waiting for unfinished jobs....
main.cc:1:10: fatal error: stdio.h: No such file or directory
    1 | #include <stdio.h>
      |          ^~~~~~~~~
compilation terminated.
make: *** [../../AppMakefile.mk:311: build/rv32imc/main.o] Error 1
Done upacking libtock-newlib-4.2.0.20211231.zip...
 ⤤ Failure building ./cxx_hello
Rebuilding Verbose: ./cxx_hello

When I try to reproduce this locally make (presumably) orders the build steps differently and proceeds to build a C file first, which will cause newlib to be fetched. Then the C++ source is built, which fetches libc++. If the C++ object is built first and it depends on newlib, the build fails.

I'll attempt to fix and open a PR.

@lschuermann
Copy link
Member

lschuermann commented Feb 3, 2024

@tyler-potyondy Try rebasing on #367 and see if that helps.

@tyler-potyondy
Copy link
Contributor Author

I rebased on #367 and that seems to have fixed the CI issue. @lschuermann thanks for resolving the issue!

@lschuermann lschuermann added the blocked Blocked on promised changes or other PRs label Feb 5, 2024
@lschuermann
Copy link
Member

Marking as blocked on #367 then. Nice to see that that fixed it!

@bradjc bradjc removed the blocked Blocked on promised changes or other PRs label Feb 6, 2024
@bradjc
Copy link
Contributor

bradjc commented Feb 6, 2024

Need rebase

Previous implementation overflowed an intermediate calculation when
computing the time in microseconds. This implementation resolves the
overflow and also provides comments and assertion statements to guard
against future overflow bugs.
@tyler-potyondy
Copy link
Contributor Author

Should be rebased and good to merge now.

@bradjc bradjc added this pull request to the merge queue Feb 7, 2024
Merged via the queue into tock:master with commit 8e52148 Feb 7, 2024
2 checks passed
@tyler-potyondy tyler-potyondy deleted the alarm-overflow branch April 5, 2024 23:35
Samir-Rashid added a commit to tyler-potyondy/libtock-c that referenced this pull request May 1, 2024
This change improves the fix done by tock#366 to make the arithemetic
calculation uphold `gettimeasticks` promise of less than "1us of error".
Samir-Rashid added a commit to tyler-potyondy/libtock-c that referenced this pull request May 1, 2024
This change improves the fix done by tock#366 to make the arithmetic
calculation uphold `gettimeasticks` promise of less than "1us of error".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants