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.

Sign up for GitHub

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

Jump to bottom

sys/evtimer,ztimer: do not depend on ztimer_now64 #17357

Merged

fjmolinas merged 14 commits into RIOT-OS:master from kfessel:p-evtimer32

Dec 15, 2021

Contributor

kfessel commented Dec 8, 2021

Contribution description

evtimer uses 32 bits of msec -> there is no reason to pull ztimer_now64

Testing procedure

run evtimer tests evtimer_mbox, evtimer_msg, evtimer_underflow with USEMODULE=evtimer_on_ztimer

Issues/PRs references

kfessel requested review from bergzand and kaspar030 as code owners

December 8, 2021 13:24

github-actions bot added Area: sys Area: timers labels

Contributor Author

kfessel commented Dec 8, 2021

evtimer_underflow is failing with USEMODULE=evtimer_on_ztimer before and after this commit so there is a bug that this does not yet fix

Contributor Author

kfessel commented Dec 8, 2021

i just saw there is a convenience function hidden in evtimer.h: evtimer_now_min i found one user of it who need 16bit (nib tracking the validit of not offline nibs (offline nibs are valid in milliseconds :wat: )

fjmolinas reviewed

View reviewed changes

sys/evtimer/evtimer.c Outdated Show resolved Hide resolved

fjmolinas reviewed

View reviewed changes

sys/evtimer/evtimer.c Outdated Show resolved Hide resolved

fjmolinas reviewed

View reviewed changes

sys/evtimer/evtimer.c Outdated Show resolved Hide resolved

fjmolinas reviewed

View reviewed changes

sys/include/evtimer.h Show resolved Hide resolved

Contributor Author

kfessel commented Dec 8, 2021

the loop in evtimer_underflow : main is blocking the timer that should be setup from triggering (in case of xtimer this is solved by xtimer_spin (which blocks the thread until the spin is over)

kfessel force-pushed the p-evtimer32 branch from 31a08a1 to c0b895b Compare

December 8, 2021 18:31

kfessel requested review from aabadie, leandrolanzieri, MichelRottleuthner and smlng as code owners

December 8, 2021 18:31

github-actions bot added the Area: tests label

fjmolinas reviewed

View reviewed changes

tests/evtimer_underflow/main.c Outdated Show resolved Hide resolved

Contributor

fjmolinas commented Dec 10, 2021

I can't really see why ztimer_now64 is needed either... lets give this one a spin... with tests ran, will do the same on my side.

fjmolinas added CI: run tests CI: ready for build and removed CI: ready for build labels

Contributor

fjmolinas commented Dec 10, 2021

@kfessel can you put a removeme commit to force evtimer_on_ztimer to be used?

fjmolinas added CI: ready for build and removed CI: ready for build labels

Contributor Author

kfessel commented Dec 13, 2021 •

edited

Loading

every time i remove xtimer dependancy at one place i expose how xtimer depenacy is not expressed for other places

Contributor

fjmolinas commented Dec 13, 2021

I had the test fail on samr21-xpro and hifive-1b with something like:

main(): This is RIOT! (Version: riot/pr/17357)
Testing generic evtimer
This should list 2 items
ev #1 offset=997
ev #2 offset=501
This should list 4 items
ev #1 offset=658
ev #2 offset=339
ev #3 offset=501
ev #4 offset=2456
Are the reception times of all 4 msgs close to the supposed values?
At   4327 ms received msg 0: "#2 supposed to be 4324"

Contributor Author

kfessel commented Dec 13, 2021 •

edited

Loading

@fjmolinas : do you mind testing with master and evtimer_on_ztimer

kfessel requested a review from cgundogan as a code owner

December 13, 2021 16:13

kfessel force-pushed the p-evtimer32 branch 2 times, most recently from dc4f160 to f3e95f9 Compare

December 14, 2021 13:12


          sys/evtimer,ztimer: do not depend on ztimer_now64

4d70d93

kfessel force-pushed the p-evtimer32 branch from f3e95f9 to 086b63d Compare

December 14, 2021 13:41

fjmolinas approved these changes

View reviewed changes

Contributor

fjmolinas left a comment

ACK, please squash and remove the forced dependency, the test failure was expected.

fjmolinas removed the CI: run tests label

kfessel force-pushed the p-evtimer32 branch from 086b63d to e281d92 Compare

December 14, 2021 17:17

Contributor Author

kfessel commented Dec 14, 2021

the failing test: see #17357 (comment)

https://ci.riot-os.org/RIOT-OS/RIOT/17357/086b63d320011e5bd9f085e590ef3cdd574939f2/output/run_test/tests/evtimer_msg/samr21-xpro:gnu.txt

is addressed in #17395

fjmolinas enabled auto-merge

December 14, 2021 17:22

fjmolinas reviewed

View reviewed changes

tests/evtimer_underflow/main.c Outdated

@@ @@ -58,6 +58,11 @@ void *worker_thread(void *arg) @@
                   }
               }
+              void sleep_msec(uint16_t t)
+              {
+                  ztimer_sleep(ZTIMER_MSEC, t);

Contributor

fjmolinas Dec 14, 2021

Should still be xtimer

Contributor

fjmolinas Dec 14, 2021

Squash right away and re-trigger

Contributor Author

kfessel Dec 15, 2021 •

edited

Loading

i did miss the disabled ci-flag :(

shall the runtest flag be added again?

Contributor

fjmolinas Dec 15, 2021

No the failures was unrelated, only ci-build is fine

fjmolinas removed the CI: ready for build label

kfessel added 13 commits

December 14, 2021 22:53


          sys/evtimer: deprecate evtimer_now_min

95767ec


          test/evtimer_underflow: test shall not need a blocking timer set

937efda

xtimer for short timouts is blocking (spin) this was needed for this test to pass
by sleeping in main we no longer need the timer set to wait


          net/gnrc_sock: do not include xtimer.h without xtimer module

0a3dd44


          sys/benchmark_upd: depends on xtimer

1e31818


          net/uhcpc: depends on xtimer

0a25554


          suit: example doesn't need xtimer suit_coap needs xtimer

427df6d


          pkg/tinydtls: depends on xtimer

af4381d


          gnrc/ipv6_auto_subnets: depend on xtimer

af13447


          test/gnrc_udp: depends on xtimer

9f285f3


          example/nanocoap_server: depends on xtimer

53387d8


          test/gnrc_sock_ip: depends on xtimer

6473c5c


          test/pkg_microcoap: depends on xtimer

5dfe59b


          tests: adds some INSUFFICIENT_MEMORY boards

f30433c

kfessel force-pushed the p-evtimer32 branch from e281d92 to f30433c Compare

December 14, 2021 21:56

kfessel added the CI: ready for build label

fjmolinas merged commit fe850e8 into RIOT-OS:master

kfessel mentioned this pull request

gnrc/ipv6nib: remove the need for evtimer-minutes #17411

Merged

fjmolinas added this to the Release 2022.01 milestone

kfessel mentioned this pull request

sys/evtimer: remove deprecated evtimer_now_min #17655

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

fjmolinas fjmolinas approved these changes

bergzand Awaiting requested review from bergzand bergzand is a code owner

kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

aabadie Awaiting requested review from aabadie aabadie is a code owner

leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

smlng Awaiting requested review from smlng

cgundogan Awaiting requested review from cgundogan

haukepetersen Awaiting requested review from haukepetersen

miri64 Awaiting requested review from miri64 miri64 is a code owner

PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

Labels

Area: examples Area: network Area: pkg Area: sys Area: tests Area: timers CI: ready for build