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

Fix NimBLEHIDDevice output report returning incorrect characteristic. #805

Merged
merged 1 commit into from
Dec 15, 2024

Conversation

h2zero
Copy link
Owner

@h2zero h2zero commented Dec 15, 2024

The input and output report characteristics share the same UUID and the getOutputReport was returning the input report characteristic. This change will return the correct characteristic by finding it with the index parameter.

Fixes #804

@h2zero h2zero force-pushed the bugfix/hid-output-report branch 2 times, most recently from eb00d7f to 1c393d8 Compare December 15, 2024 16:55
The input and output report characteristics share the same UUID and the `getOutputReport` was returning the input report characteristic.
This change will return the correct characteristic by finding it with the index parameter.
@h2zero h2zero force-pushed the bugfix/hid-output-report branch from 1c393d8 to 20f10c5 Compare December 15, 2024 16:57
@h2zero h2zero merged commit 8c30638 into master Dec 15, 2024
31 checks passed
@h2zero h2zero deleted the bugfix/hid-output-report branch December 15, 2024 17:01
h2zero added a commit that referenced this pull request Dec 15, 2024
…#805)

The input and output report characteristics share the same UUID and the `getOutputReport` was returning the input report characteristic.
This change will return the correct characteristic by finding it with the index parameter.
@afpineda
Copy link
Contributor

afpineda commented Dec 17, 2024

Hi. I know v2.1.1 has been released, but I think this bug is not fixed at all.
You can have more than one output report. The same applies to getFeatureReport() which was not fixed.
The implementation in v1.4.x was OK.

Note: The output report characteristic is optional and should only be created after the input report characteristic.

This creates temporal coupling which is bad both for the user and the maintainer. There should be another fix.

Another related problem:

If you return the same characteristic for any reportID, you have no way to install a different characteristic callback for each of them.

@h2zero
Copy link
Owner Author

h2zero commented Dec 17, 2024

Hi @afpineda, thanks for the feedback.

You can have more than one output report.

I did not know this.

The PR was only to address the issue brought up in #804, where it was confirmed to resolve the issue.

I'm starting to think that the best fix is to move this class into it's own repo/library as I know nothing of HID specs and have no use or interest for this myself. The only reason this is here is because it was in the original library so I converted it to use NimBLE.
If you would like to open a repo and host this I would be happy to provide a link to it from here instead of hosting the code.

@afpineda
Copy link
Contributor

afpineda commented Dec 17, 2024

You can have more than one output report.

I did not know this.

In fact, you can have more than one report of any kind (up to 255), including input reports.

I'm starting to think that the best fix is to move this class into it's own repo/library as I know nothing of HID specs and have no use or interest for this myself. The only reason this is here is because it was in the original library so I converted it to use NimBLE. If you would like to open a repo and host this I would be happy to provide a link to it from here instead of hosting the code.

Don't get discouraged by a few bugs. You are doing a good job!
Believe me, I make extensive use of the NimBLEHIDDevice class and I can say that it works well.
Moving it to another repo is a bad idea. You force your users into another software dependency, and dependency management is a nightmare. But don't worry, I will help with this via pull request.

@h2zero
Copy link
Owner Author

h2zero commented Dec 18, 2024

Not discouraged, just a bit weary. I get in trouble any time I touch this class lol. Like I said this is not my area of knowledge, just thought it would be better if someone that knows more about it maintained it.

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.

Output Report Function Not Working Properly in Release 2.1.0
2 participants