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.

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/fido2: use ztimer instead of xtimer #17753

Merged
merged 1 commit into from
Mar 10, 2022
Merged

sys/fido2: use ztimer instead of xtimer #17753

merged 1 commit into from
Mar 10, 2022

Conversation

fjmolinas
Copy link
Contributor

Contribution description

Before fido2 was using xtimer and ztimer while not making the xtimer dependency explicit, this PR uses ztimer or ztimer64 everywhere where xtimer was used.

  • for ctap hid timeouts xtimer was used, use ztimer64_msec instead
    since the code is using absolute times, an already using ztimer_msec
  • use event_timeout_ztimer instead of event_timeout to not pull in
    xtimer

The code could probably be refactored to not require 64bit, but the GOAL of this PR is just to not require the 64BITS xtimer API.

Testing procedure

Green murdock should be enough.

@fjmolinas
sys/fido2: use ztimer instead of xtimer
8e34227 
- for ctap hid timeouts xtimer was used, use ztimer64_msec instead
  since the code is using absolute times, an already using ztimer_msec
- use event_timeout_ztimer instead of event_timeout to not pull in
  xtimer
@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 Mar 4, 2022
@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: sys Area: System labels Mar 4, 2022
@fjmolinas
Copy link
Contributor Author

Will de-schedule, re-schedule this one in favor of #17754 once PR #17746 finished to be sure there are no more overlfows pending.

@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 Mar 4, 2022
@kfessel kfessel added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 4, 2022
@Ollrogge
Copy link
Contributor

Ollrogge commented Mar 6, 2022

Changes look good. ACK from me

@fjmolinas fjmolinas requested a review from benpicco March 7, 2022 07:14
@fjmolinas
Copy link
Contributor Author

Thanks for the review @Ollrogge, could a maintainer take a look and eventually ACK, I think it should be good since @Ollrogge is the contributor here. Maybe @kfessel or @kaspar030?

benpicco
benpicco approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

I'm a bit surprised ztimer64_msec is needed though

@fjmolinas
Copy link
Contributor Author

I'm a bit surprised ztimer64_msec is needed though

I don't think it is, but this was the easiest approach for me that does not understand the fido2 code in detail, it limits this PR to a timer backend change and not the ctap timeout logic, @Ollrogge can probably state better but the timeout code could be reworked to rely on 32bit timeouts?

@Ollrogge
Copy link
Contributor

Ollrogge commented Mar 9, 2022

I'm a bit surprised ztimer64_msec is needed though

I don't think it is, but this was the easiest approach for me that does not understand the fido2 code in detail, it limits this PR to a timer backend change and not the ctap timeout logic, @Ollrogge can probably state better but the timeout code could be reworked to rely on 32bit timeouts?

Yes it can be reworked. Changing it from 64 bit to 32 bit should have no implications

@fjmolinas fjmolinas merged commit 8fa2687 into RIOT-OS:master Mar 10, 2022
@fjmolinas fjmolinas deleted the pr_fido2_all_ztimer branch March 10, 2022 07:08
@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

@benpicco benpicco benpicco approved these changes

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

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@Ollrogge Ollrogge Awaiting requested review from Ollrogge

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

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees
No one assigned
Labels
Area: Kconfig Area: Kconfig integration Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Milestone
Release 2022.04
Development

Successfully merging this pull request may close these issues.
5 participants
@fjmolinas @Ollrogge @benpicco @kfessel @OlegHahm