Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Multi-touch touchscreen support (#123) #297
base: master
Are you sure you want to change the base?
Multi-touch touchscreen support (#123) #297
Changes from 2 commits
abadb2b
1f00971
ba4f2d9
dfe04b8
e41e732
87558be
7b4e634
827bf37
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said the library supports 10 fingers. Why are you only demonstrating 8 here? Also a few comments about the fact that 10 fingers are supported would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Hybrid Mode" issue below.
2 fingers will fully exercise the report. Anything more than 2 exercises the Hybrid Mode reporting. I went over an issue with reporting not-multiple-of-2 fingers when unused fingers are not zeroed, so I decided to report 7 here. This will have fully populated reports, multiple reports for hybrid mode and partially populated reports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why do you define 10 fingers then? I think I dont understand that correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current state, 2 is the number of fingers in the report, defined by the descriptor; 10 is the theoretical maximum number of fingers, defined by the feature report:
I'm setting 2/10 because I think that represents the most common scenarios. In fact I'm trying to do something that sends ~8 touches regularly up to max ~36 concurrent touches. I was expecting the user to configure these parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about those? What is the difference between those states and are they used by the windows client? If yes, why dont we expose these?
Nevertheless, please put them in an enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they are just for internal tracking. 0 and 1 just says if an entry is in use or not. 2 helps to identify a "release" action because windows expects that a released touch is reported once with tip-switch set off off after releasing, instead of just being silently excluded from the report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my suggest API implementation will work, I think this tracking is no longer required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are moved to hpp as an enum, but I think the current internal tracking cannot be further simplified. See below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if those API names can't be improved. Checkout the other APIs:
https://github.com/NicoHood/HID/blob/master/src/HID-APIs/KeyboardAPI.h#L48-L56
https://github.com/NicoHood/HID/blob/master/src/HID-APIs/AbsoluteMouseAPI.h#L69-L75
What about having:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not yet very about having logic to perform sequences of operations to implement gestures. I'm sticking with the simplest operations for now. I'll come back to this when I find a chance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you declare this x1 and x0 variable as int16, there is no need to to the MSB LSB shifting.
Like here:
https://github.com/NicoHood/HID/blob/master/src/HID-APIs/AbsoluteMouseAPI.h#L47-L48
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done