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

Don't assume that all notifications are NotifySlotChange #154

Merged

Conversation

bluetech
Copy link
Contributor

CCID defines another type of message type on the interrupt pipe,
HardwareError, and others may also be conceivably sent. So check the
message type instead of assuming it's a NotifySlotChange.

While at it, I added debug logging of HardwareError, it might be useful
for someone.

Note 1: I recommend viewing the second commit ignoring whitespace, it makes the diff clearer.

Note 2: I don't have a multi-slot reader, so I wasn't able to test the Multi_ code paths.

@bluetech bluetech force-pushed the check-notification-type branch from 3dd4ffc to 1394d11 Compare December 12, 2024 14:16
src/ccid.h Show resolved Hide resolved
src/ccid.h Outdated Show resolved Hide resolved
@LudovicRousseau
Copy link
Owner

I like your changes. It is good work.

@bluetech bluetech force-pushed the check-notification-type branch from 1394d11 to 982b68f Compare December 13, 2024 07:43
@bluetech
Copy link
Contributor Author

Thanks for the review, I addressed your comments.

This is somewhat nice for readability, but will be useful for the next
commit.
CCID defines another type of message type on the interrupt pipe,
HardwareError, and others may also be conceivably sent. So check the
message type instead of assuming it's a NotifySlotChange.

While at it, I added debug logging of HardwareError, it might be useful
for someone.
@bluetech bluetech force-pushed the check-notification-type branch from 982b68f to 7adefa1 Compare December 13, 2024 13:34
@bluetech
Copy link
Contributor Author

I added the default cases. I used the message "Unrecognized notification", since "Unknown bMessageType" is a bit ambiguous.

@LudovicRousseau LudovicRousseau merged commit 8bd1acf into LudovicRousseau:master Dec 13, 2024
10 checks passed
@LudovicRousseau
Copy link
Owner

Thanks

@bluetech bluetech deleted the check-notification-type branch December 13, 2024 20:17
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.

2 participants