Skip to content

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

Merged
fjmolinas merged 14 commits intoRIOT-OS:masterfrom
kfessel:p-evtimer32
Dec 15, 2021
Merged

sys/evtimer,ztimer: do not depend on ztimer_now64#17357
fjmolinas merged 14 commits intoRIOT-OS:masterfrom
kfessel:p-evtimer32

Conversation

@kfessel
Copy link
Copy Markdown
Contributor

@kfessel 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

@github-actions github-actions bot added Area: sys Area: System Area: timers Area: timer subsystems labels Dec 8, 2021
@kfessel
Copy link
Copy Markdown
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

@kfessel
Copy link
Copy Markdown
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
fjmolinas reviewed Dec 8, 2021
View reviewed changes
fjmolinas
fjmolinas reviewed Dec 8, 2021
View reviewed changes
fjmolinas
fjmolinas reviewed Dec 8, 2021
View reviewed changes
fjmolinas
fjmolinas reviewed Dec 8, 2021
View reviewed changes
@kfessel
Copy link
Copy Markdown
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)

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 8, 2021
fjmolinas
fjmolinas reviewed Dec 8, 2021
View reviewed changes
@fjmolinas
Copy link
Copy Markdown
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 fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR 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 Dec 10, 2021
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Dec 10, 2021

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

@fjmolinas fjmolinas 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 Dec 13, 2021
@kfessel
Copy link
Copy Markdown
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

@fjmolinas
Copy link
Copy Markdown
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"

@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Dec 13, 2021 •
edited
Loading

@fjmolinas : do you mind testing with master and evtimer_on_ztimer

@kfessel kfessel requested a review from cgundogan as a code owner December 13, 2021 16:13
@kfessel kfessel force-pushed the p-evtimer32 branch 2 times, most recently from dc4f160 to f3e95f9 Compare December 14, 2021 13:12
fjmolinas
fjmolinas approved these changes Dec 14, 2021
View reviewed changes
Copy link
Copy Markdown
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, please squash and remove the forced dependency, the test failure was expected.

@fjmolinas fjmolinas removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Dec 14, 2021
@kfessel
Copy link
Copy Markdown
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 fjmolinas enabled auto-merge December 14, 2021 17:22
fjmolinas
fjmolinas reviewed Dec 14, 2021
View reviewed changes
tests/evtimer_underflow/main.c Outdated

void sleep_msec(uint16_t t)
{
ztimer_sleep(ZTIMER_MSEC, t);
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Dec 14, 2021

Choose a reason for hiding this comment

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

Should still be xtimer

Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Dec 14, 2021

Choose a reason for hiding this comment

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

Squash right away and re-trigger

Copy link
Copy Markdown
Contributor Author

@kfessel kfessel Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

i did miss the disabled ci-flag :(

shall the runtest flag be added again?

Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Dec 15, 2021

Choose a reason for hiding this comment

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

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

@fjmolinas fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 14, 2021
kfessel added 13 commits December 14, 2021 22:53
@kfessel
sys/evtimer: deprecate evtimer_now_min
95767ec
@kfessel
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
@kfessel
net/gnrc_sock: do not include xtimer.h without xtimer module
0a3dd44
@kfessel
sys/benchmark_upd: depends on xtimer
1e31818
@kfessel
net/uhcpc: depends on xtimer
0a25554
@kfessel
suit: example doesn't need xtimer suit_coap needs xtimer
427df6d
@kfessel
pkg/tinydtls: depends on xtimer
af4381d
@kfessel
gnrc/ipv6_auto_subnets: depend on xtimer
af13447
@kfessel
test/gnrc_udp: depends on xtimer
9f285f3
@kfessel
example/nanocoap_server: depends on xtimer
53387d8
@kfessel
test/gnrc_sock_ip: depends on xtimer
6473c5c
@kfessel
test/pkg_microcoap: depends on xtimer
5dfe59b
@kfessel
tests: adds some INSUFFICIENT_MEMORY boards
f30433c
@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 15, 2021
@fjmolinas fjmolinas merged commit fe850e8 into RIOT-OS:master Dec 15, 2021
@kfessel kfessel mentioned this pull request Dec 15, 2021
Merged
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
@kfessel kfessel mentioned this pull request Feb 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: examples Area: Example Applications Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework 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

Projects

None yet

Milestone

Release 2022.01

Development

Successfully merging this pull request may close these issues.

2 participants

@kfessel @fjmolinas