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

nrf: move set_waiting in button handling #319

Merged
merged 1 commit into from
Jul 19, 2023
Merged

nrf: move set_waiting in button handling #319

merged 1 commit into from
Jul 19, 2023

Conversation

daringer
Copy link
Collaborator

This correctly positions the set_waiting() calls as suggested inside the issue.

Tested with the google-ctap2-test-suite + nk3am. It now properly outputs:

Please touch your security key!

@daringer daringer linked an issue Jul 18, 2023 that may be closed by this pull request
@daringer daringer changed the title nrf: move set_waiting in button handling; fixes #318 nrf: move set_waiting in button handling Jul 18, 2023
@sosthene-nitrokey
Copy link
Collaborator

LGTM, though maybe it could have its place in set_status

Looking at it I also notice the call to self.set_status(ui::Status::WaitingForUserPresence); that is not present in the corresponding LPC55 implementation. Should it be there? The status is never restored by the firmware, and trussed already calls it before checking for user presence and then restores it.

@daringer
Copy link
Collaborator Author

daringer commented Jul 19, 2023

Generally it could also be that this fix could solves this issue, too: #96

Looking at it I also notice the call to self.set_status(ui::Status::WaitingForUserPresence); that is not present in the corresponding LPC55 implementation. Should it be there? The status is never restored by the firmware, and trussed already calls it before checking for user presence and then restores it.

Yes, that's right, seen that too, would suggest to make this a part of #93 , where we have to investigate the LED behavior anyways - will add a note there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nrf52: Fix upneeded status in CTAPHID keepalive
2 participants