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

boards/waspmote: Fix timer config #14799

Merged
merged 3 commits into from
Sep 24, 2020
Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Aug 19, 2020

Contribution description

  • Set XTIMER_HZ to something that is actually possible to generate with one of the available clock dividers from the core frequency
  • Use xtimer_on_ztimer if xtimer is used and not ztimer_xtimer_compat is used
    • This is needed because xtimer is simply not compatible with any of the possible clock frequencies of this board

Testing procedure

The tests for periph_timer and xtimer in tests/ should now pass on the waspmote-pro

Issues/PRs references

Fixes #14750

Includes #14807

@maribu maribu added Area: boards Area: Board ports Area: timers Area: timer subsystems Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 19, 2020
@maribu
Copy link
Member Author

maribu commented Aug 19, 2020

@jdavid: Could you test this PR and report back here? Thanks :-)

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 19, 2020
@jdavid
Copy link
Contributor

jdavid commented Aug 20, 2020

tests/periph_timer fails with the same output:

$ git rev-parse --verify HEAD
10c6bccf6ccac6cd4800f04256e28bdf6395bcc1
$ make BOARD=waspmote-pro PORT=/dev/ttyUSB0 FFLAGS_EXTRA="-F" all flash -C tests/periph_timer
[...]
$ make BOARD=waspmote-pro BOOTLOADER_BAUD=115200 PORT=/dev/ttyUSB0 term -C tests/periph_timer
make: Entering directory '/home/jdavid/sandboxes/UiO/wsn_riot/RIOT/tests/periph_timer'
/home/jdavid/sandboxes/UiO/wsn_riot/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyUSB0" -b "9600"  
2020-08-20 12:45:39,616 # Connect to serial port /dev/ttyUSB0
Welcome to pyterm!
Type '/exit' to exit.
2020-08-20 12:45:41,007 # 

2020-08-20 12:45:41,055 # Help: Press s to start test, r to print it is ready
r
2020-08-20 12:45:42,542 # READY
s
2020-08-20 12:45:43,406 # START
2020-08-20 12:45:43,470 # main(): This is RIOT! (Version: 2020.10-devel-861-g10c6b-HEAD)
2020-08-20 12:45:43,470 # 
2020-08-20 12:45:43,502 # Test for peripheral TIMERs
2020-08-20 12:45:43,502 # 
2020-08-20 12:45:43,518 # Available timers: 2
2020-08-20 12:45:43,518 # 
2020-08-20 12:45:43,549 # Testing TIMER_0:
2020-08-20 12:45:43,582 # TIMER_0: ERROR on initialization - skipping
2020-08-20 12:45:43,583 # 
2020-08-20 12:45:43,597 # 
2020-08-20 12:45:43,613 # Testing TIMER_1:
2020-08-20 12:45:43,661 # TIMER_1: ERROR on initialization - skipping
2020-08-20 12:45:43,661 # 
2020-08-20 12:45:43,662 # 
2020-08-20 12:45:43,662 # TEST FAILED

USEMODULE=xtimer_on_ztimer doesn't make a difference, though I understand this does not affect the periph_timer test.

@jdavid
Copy link
Contributor

jdavid commented Aug 20, 2020

but tests/xtimer_usleep works!!

2020-08-20 13:01:16,980 # START
2020-08-20 13:01:17,044 # main(): This is RIOT! (Version: 2020.10-devel-861-g10c6b-HEAD)
2020-08-20 13:01:17,092 # Running test 5 times with 7 distinct sleep times
2020-08-20 13:01:17,156 # Slept for 11011 us (expected: 10000 us) Offset: 1011 us
2020-08-20 13:01:17,268 # Slept for 50998 us (expected: 50000 us) Offset: 998 us
2020-08-20 13:01:17,332 # Slept for 11242 us (expected: 10234 us) Offset: 1008 us
2020-08-20 13:01:17,444 # Slept for 57778 us (expected: 56780 us) Offset: 998 us
2020-08-20 13:01:17,508 # Slept for 13130 us (expected: 12122 us) Offset: 1008 us
2020-08-20 13:01:17,668 # Slept for 99765 us (expected: 98765 us) Offset: 1000 us
[...]

@maribu
Copy link
Member Author

maribu commented Aug 20, 2020

tests/periph_timer fails with the same output:

Sorry, my bad. The test manually configured the timer frequency via the Makefile. I updated it to also abuse XTIMER_HZ for that, as all other timer tests do. Now it should work. Could you test again?

@jdavid
Copy link
Contributor

jdavid commented Aug 20, 2020

tests/periph_timer still doesn't work, with the same output

@aabadie
Copy link
Contributor

aabadie commented Aug 20, 2020

tests/periph_timer still doesn't work, with the same output

It's because this test expect the timer to run at 250kHZ and now it's set to 230400LU:

BOARDS_TIMER_25kHz := \
arduino-duemilanove \
arduino-leonardo \
arduino-mega2560 \
arduino-uno \
atmega256rfr2-xpro \
atmega328p \
waspmote-pro \
#

Can you try TIMER_SPEED=230400 make BOARD=waspmote-pro -C tests/periph_timer flash test ?

@maribu
Copy link
Member Author

maribu commented Aug 20, 2020

It's because this test expect the timer to run at 250kHZ and now it's set to 230400LU:

This should be changed by this commit: 9575ffd

@maribu
Copy link
Member Author

maribu commented Aug 20, 2020

Good that I took a second look. I sneaked in the drop of the volatile by accident. (That should have happend one commit later by using atomics.)

@maribu
Copy link
Member Author

maribu commented Aug 20, 2020

Arghh, and no I accidentially added a space :-/

@aabadie
Copy link
Contributor

aabadie commented Aug 20, 2020

This PR seems to go a bit out of scope: the changes in test application affect several boards. Maybe that would deserve a separate PR ?

@maribu maribu added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 20, 2020
@maribu maribu added the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 16, 2020
@maribu maribu removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 17, 2020
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 17, 2020
maribu added a commit to maribu/RIOT that referenced this pull request Sep 18, 2020
xtimer.h must not be included, when the xtimer module is not use. Otherwise
compilation on the waspmote-pro with RIOT-OS#14799
will not longer work. gnrc_netif_pktq includes xtimer.h and uses xtimer, but
gnrc_netif includes gnrc_netif_pktq.h regardless of whether gnrc_netif_pktq
is used. This makes sure that gnrc_netif_pktq.h is only included when actually
used.
maribu added a commit to maribu/RIOT that referenced this pull request Sep 19, 2020
xtimer.h must not be included, when the xtimer module is not use. Otherwise
compilation on the waspmote-pro with RIOT-OS#14799
will not longer work. gnrc_netif_pktq includes xtimer.h and uses xtimer, but
gnrc_netif includes gnrc_netif_pktq.h regardless of whether gnrc_netif_pktq
is used. This makes sure that gnrc_netif_pktq.h is only included when actually
used.
maribu added a commit to maribu/RIOT that referenced this pull request Sep 22, 2020
xtimer.h must not be included, when the xtimer module is not use. Otherwise
compilation on the waspmote-pro with RIOT-OS#14799
will not longer work. gnrc_netif_pktq includes xtimer.h and uses xtimer, but
gnrc_netif includes gnrc_netif_pktq.h regardless of whether gnrc_netif_pktq
is used. This makes sure that gnrc_netif_pktq.h is only included when actually
used.
maribu added a commit to maribu/RIOT that referenced this pull request Sep 22, 2020
xtimer.h must not be included, when the xtimer module is not use. Otherwise
compilation on the waspmote-pro with RIOT-OS#14799
will not longer work. gnrc_netif_pktq includes xtimer.h and uses xtimer, but
gnrc_netif includes gnrc_netif_pktq.h regardless of whether gnrc_netif_pktq
is used. This makes sure that gnrc_netif_pktq.h is only included when actually
used.
@maribu maribu added this to the Release 2020.10 milestone Sep 23, 2020
@maribu
Copy link
Member Author

maribu commented Sep 23, 2020

It would be nice to get this bugfix into the next release. I just rebased on top of the now merged compilation fix.

@jdavid provided testing results, so that only needs a code review, a green Murdock and an ACK 😄 (And I have good reason to believe that the recently merged compilation fixes results in a green Murdock.)

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, changes make sense and test output was provided.

- Set XTIMER_HZ to something that is actually possible to generate with one
  of the available clock dividers from the core frequency
- Use xtimer_on_ztimer if xtimer is used and not ztimer_xtimer_compat is used
    - This is needed because xtimer is simply not compatible with any of the
      possible clock frequencies of this board
The waspmote-pro cannot run at 250 kHz, as no available divider can divide
its core frequency down to that. Instead, the board can be tested at its
core clock frequency.
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 23, 2020
@maribu
Copy link
Member Author

maribu commented Sep 23, 2020

All green. make BOARD=waspmote-pro info-modules still should xtimer_on_ztimer getting pulled in when xtimer is used, so splitting the handling out introduced no issues.

@fjmolinas
Copy link
Contributor

Still good IMO

@fjmolinas fjmolinas merged commit 2cfb4f4 into RIOT-OS:master Sep 24, 2020
@maribu
Copy link
Member Author

maribu commented Sep 24, 2020

Thanks everyone! Especially thanks to @jdavid for reporting the issue and testing the fix!

@maribu maribu deleted the waspmote-pro branch September 24, 2020 07:38
@jdavid
Copy link
Contributor

jdavid commented Sep 28, 2020

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

periph_timer test doesn't work with the waspmote-pro board
6 participants