Skip to content

drivers: migrate xtimer64 and xtimer/ticks users to ztimer#17367

Merged
fjmolinas merged 7 commits intoRIOT-OS:masterfrom
fjmolinas:pr_driver_ztimer_corner_cases
Jan 24, 2022
Merged

drivers: migrate xtimer64 and xtimer/ticks users to ztimer#17367
fjmolinas merged 7 commits intoRIOT-OS:masterfrom
fjmolinas:pr_driver_ztimer_corner_cases

Conversation

@fjmolinas
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas commented Dec 9, 2021 •
edited
Loading

Contribution description

This PR takes care of the user's of xtimer which can't be directly changed to ztimer with the coccinelle script.

Testing procedure

I don't have any of the hardware sadly... but I tagged people that are susceptible to having the hardware, it will at least document what was and not tested.

Issues/PRs references

Part of #13667 and #17111

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 9, 2021
@github-actions github-actions bot added Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework labels Dec 9, 2021
@fjmolinas fjmolinas changed the title drivers: migrate 64bits and ticks users to ztimer drivers: migrate xtimer64 and xtimer/ticks users to ztimer Dec 9, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Dec 9, 2021

Regarding the ltc coulomb counter: I think it makes sense to use ztimer (not 64) with milliseconds precision instead. Chances are, that the coulomb counter is used to monitor the power consumption of the board attached to it, so one might be interested in using low power. Also, microsecond resolution is likely not needed, as with typical current consumption there is at least roughly a second between to pulses.

I think the ltc driver is one of my first drivers, so likely it is shitty noob code. Maybe I should take a look at it again, clean it up, and port it to ztimer?

@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Dec 9, 2021

I think the ltc driver is one of my first drivers, so likely it is shitty noob code. Maybe I should take a look at it again, clean it up, and port it to ztimer?

As you prefer!

@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Dec 13, 2021

I think the ltc driver is one of my first drivers, so likely it is shitty noob code. Maybe I should take a look at it again, clean it up, and port it to ztimer?

As you prefer!

@maribu should I split that one out and you open a separate PR for it?

@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Dec 15, 2021

I think the ltc driver is one of my first drivers, so likely it is shitty noob code. Maybe I should take a look at it again, clean it up, and port it to ztimer?

As you prefer!

@maribu should I split that one out and you open a separate PR for it?

Otherwise we get this one in (quickly get rid of xtimer) and then you can do the re-work?

@fjmolinas fjmolinas requested a review from kaspar030 December 15, 2021 09:50
@fjmolinas fjmolinas force-pushed the pr_driver_ztimer_corner_cases branch from 6411240 to c15ed7b Compare December 15, 2021 10:01
@github-actions github-actions bot added Area: boards Area: Board ports Area: doc Area: Documentation labels Dec 15, 2021
@fjmolinas fjmolinas mentioned this pull request Dec 15, 2021
Closed
@fjmolinas fjmolinas force-pushed the pr_driver_ztimer_corner_cases branch from c15ed7b to 0cbae06 Compare December 15, 2021 13:06
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Dec 15, 2021

Rebased to fix conflict

@fjmolinas fjmolinas force-pushed the pr_driver_ztimer_corner_cases branch 2 times, most recently from 5eb11e8 to df23ba0 Compare December 16, 2021 11:28
@github-actions github-actions bot added the Area: sys Area: System label Dec 16, 2021
@fjmolinas fjmolinas mentioned this pull request Dec 16, 2021
Merged
@fjmolinas fjmolinas force-pushed the pr_driver_ztimer_corner_cases branch from df23ba0 to fc7c991 Compare December 16, 2021 14:12
@github-actions github-actions bot removed the Area: sys Area: System label Dec 16, 2021
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Dec 16, 2021

All green @maribu if you would rather still split out the ltc, please feel free to force push to the branch, I'll be AFK ideally for the last month.

@maribu
Copy link
Copy Markdown
Member

maribu commented Dec 16, 2021

lets get it in as is, using ztimer_msec instead of ztimer_usec can be a follow-up

kfessel
kfessel reviewed Dec 20, 2021
View reviewed changes
@fjmolinas fjmolinas force-pushed the pr_driver_ztimer_corner_cases branch from 565d22e to 8fdb92d Compare January 17, 2022 11:04
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Jan 18, 2022

@maribu can we get this in?

maribu
maribu reviewed Jan 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I don't have time for testing, but I also don't see anything that would break due to the switch, unless this would trigger an unknown bug in ztimer.

@github-actions github-actions bot removed the Area: doc Area: Documentation label Jan 18, 2022
@fjmolinas fjmolinas force-pushed the pr_driver_ztimer_corner_cases branch from 5140148 to c19a787 Compare January 18, 2022 15:27
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Jan 19, 2022

Code looks good to me. I don't have time for testing, but I also don't see anything that would break due to the switch, unless this would trigger an unknown bug in ztimer.

Yes, at least its explicit that not all was tested, and its pretty isolated code. Only isolated driver code.

maribu
maribu approved these changes Jan 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

The code looks good an unscary. If anyone insist on testing every driver, speak up now ;)

@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Jan 24, 2022

The code looks good an unscary. If anyone insist on testing every driver, speak up now ;)

Lets go then!

@fjmolinas fjmolinas merged commit b985a74 into RIOT-OS:master Jan 24, 2022
@fjmolinas fjmolinas deleted the pr_driver_ztimer_corner_cases branch January 24, 2022 09:41
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Jan 24, 2022

Thanks for the review!

@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfessel kfessel kfessel left review comments

@maribu maribu maribu approved these changes

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

@cgundogan cgundogan Awaiting requested review from cgundogan

@jia200x jia200x Awaiting requested review from jia200x jia200x 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

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

No one assigned

Labels

Area: boards Area: Board ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework 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.04

Development

Successfully merging this pull request may close these issues.

4 participants

@fjmolinas @maribu @kfessel @OlegHahm