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

tests/periph_rtc: test sub-second precision #10763

Closed
wants to merge 1 commit into from

Conversation

benemorius
Copy link
Member

@benemorius benemorius commented Jan 14, 2019

Contribution description

This expands tests/periph_rtc to verify the microsecond precision of RTC timers. This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second. It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.

Testing procedure

Run tests/periph_rtc and observe new output (error -177429 microseconds).

Currently it fails for kinetis/kw41z (perhaps all kinetis) and that looks like this:

main(): This is RIOT! (Version: 2018.10-RC1-679-ga296b-telnet-shell)

RIOT RTC low-level driver test
This test will display 'Alarm!' every 2 seconds for 4 times
  Setting clock to   2011-12-13 14:15:15
Clock value is now   2011-12-13 14:15:15
  Setting alarm to   2011-12-13 14:15:17
   Alarm is set to   2011-12-13 14:15:17

[30303] First alarm expected at 2018615 microseconds
[1841186] Alarm! after 1822571 microseconds (error -177429 microseconds)
[3841186] Alarm! after 3822571 microseconds (error -177429 microseconds)
[5841186] Alarm! after 5822571 microseconds (error -177429 microseconds)
[7841186] Alarm! after 7822571 microseconds (error -177429 microseconds)

In the case of kw41z the error is always a random value (abs(error) < 1 second) depending on what value is randomly in the RTC subsecond register which is currently ignored by kinetis/periph/rtt_set_counter().

A successful test should have output similar to:

main(): This is RIOT! (Version: 2018.10-RC1-680-g83348-telnet-shell)

RIOT RTC low-level driver test
This test will display 'Alarm!' every 2 seconds for 4 times
  Setting clock to   2011-12-13 14:15:15
Clock value is now   2011-12-13 14:15:15
  Setting alarm to   2011-12-13 14:15:17
   Alarm is set to   2011-12-13 14:15:17

[30303] First alarm expected at 2018585 microseconds
[2018615] Alarm! after 2000030 microseconds (error 30 microseconds)
[4018615] Alarm! after 4000030 microseconds (error 30 microseconds)
[6018615] Alarm! after 6000030 microseconds (error 30 microseconds)
[8018615] Alarm! after 8000030 microseconds (error 30 microseconds)

The error precision is limited by the frequency of xtimer's timer, hence the 30us error for kw41z with a 16bit 32kHz xtimer where 30us is only 1 clock tick. Platforms with a faster xtimer should have a smaller error. Perhaps no platform should have an error over 100us.

Comments

Must test other platforms.
Perhaps I should have updated the readme.
Why doesn't newlib declare settimeofday()?

Issues/PRs references

#10764 fixes kinetis/periph_rtt to give the successful test output shown above.

@jcarrano
Copy link
Contributor

I think this test is useful and necessary, although I'm concerned about the case where the RTC and Xtimer are powered by the same clock. Wouldn't the test stop working in that case?

@benemorius
Copy link
Member Author

I think this test is useful and necessary, although I'm concerned about the case where the RTC and Xtimer are powered by the same clock. Wouldn't the test stop working in that case?

It works for me on kw41z where both xtimer and periph_rtc use the same low power clock source. I think most low power platforms will be this way. I can't think of a reason it should be a problem.

@PeterKietzmann PeterKietzmann added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: timers Area: timer subsystems labels Jan 15, 2019
@PeterKietzmann
Copy link
Member

I think this change is useful. It already helped in #10764.

I'm concerned about the case where the RTC and Xtimer are powered by the same clock.

@jcarrano I'm no convinced about that but also lacking knowledge about xtimers internals so I'd rather run some tests on exemplary devices. Do you know of any where your concerns apply?

@aabadie
Copy link
Contributor

aabadie commented Jan 23, 2019

I think this change is useful.

I fully agree.
The Python test automation script should be updated, otherwise it's broken in the current state.

@benemorius
Copy link
Member Author

The Python test automation script should be updated, otherwise it's broken in the current state.

I didn't see those scripts until now. Wow that's a very nice system.

Should we check that the error is less than some predetermined value? I didn't expect to see any platform with an error over 100us but @PeterKietzmann found in #10764 that it can be higher. Although it's possible that that's due to another problem.

@PeterKietzmann
Copy link
Member

Should we check that the error is less than some predetermined value?

I think this would be a nice improvement. However, this requires certain board specific assumptions about maximum deviations. @MrKevinWeiss came to the great idea to look up worst-case parameters of clock sources at DigiKey which lead to a total number around 500ppm. With that we would at least find misconfigured timers

@PeterKietzmann
Copy link
Member

BTW: @MrKevinWeiss would you like to take this PR over?

@MrKevinWeiss
Copy link
Contributor

Only for +-250ppm for frequency stability and +-250ppm for frequency tolerance (not necessarily compounded but we can say that as a dumb worst case) for crystals... Not to mention internal RC oscillators.
@PeterKietzmann Like is a strong word, but if we need it to be finished and added in to be runnable with the compile_and_test_for_board.py tool, then I will, it will only be after the uart test PR is merged though, FIFO style.

@MrKevinWeiss MrKevinWeiss self-assigned this Jan 24, 2019
@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot closed this Sep 10, 2019
@MrKevinWeiss MrKevinWeiss reopened this Sep 10, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Sep 10, 2019
@MrKevinWeiss
Copy link
Contributor

Oops. I need to find time to take this over!

@benpicco
Copy link
Contributor

That's pretty useful (also for RTT) - want to give it a rebase?

MrKevinWeiss added a commit to MrKevinWeiss/RIOT that referenced this pull request Feb 26, 2020
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 us  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
@MrKevinWeiss
Copy link
Contributor

I have #13483 to replace this. It takes care of the conflict and adapts the python test.

@benemorius benemorius closed this Feb 28, 2020
benpicco pushed a commit to benpicco/RIOT that referenced this pull request Feb 8, 2021
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 ms  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
benpicco pushed a commit to benpicco/RIOT that referenced this pull request Feb 8, 2021
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 ms  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
benpicco pushed a commit to benpicco/RIOT that referenced this pull request Feb 8, 2021
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 ms  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
benpicco pushed a commit to benpicco/RIOT that referenced this pull request Feb 8, 2021
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 ms  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
benpicco pushed a commit to benpicco/RIOT that referenced this pull request Feb 23, 2021
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 ms  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
benpicco pushed a commit to benpicco/RIOT that referenced this pull request Feb 23, 2021
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 ms  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
benpicco pushed a commit to benpicco/RIOT that referenced this pull request May 6, 2021
Adapted from @benemorius PR RIOT-OS#10763.
Add mircosecond precision test to RTC.
Adapt automated python test to match test API changes.
Assert max of 100 ms  error.

This is needed to test that an RTC alarm fires at the desired time and not with an arbitrary offset up to +/-1 second.
It also tests that rtc_set_time() behaves equivalently correctly, which is what I needed to test when I wrote this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: tests Area: tests and testing framework Area: timers Area: timer subsystems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants