Multi-touch touchscreen support (#123) #297
Conversation
There was a problem hiding this comment.
Thanks a lot for submitting this very awesome patch!
I've taken a look at it and added lots of comments. I hope that helps in improving the code. Please keep in mind that I have not looked at my own code for years, so I might be wrong or missunderstand. You are the expert, as you've worked on that for the last weeks or even month.
A few major things after looking at this:
- What about renaming
MultiTouchtoTouchscreen? I guess that describes it better, right? - Can you please tell me what this feature report is used for? Is it really required? I do not yet see the benefit for that, but I can imagine that Windows might need it?
- I've lot looked that close at the API implementation, as I'd suggest an API change that is more clear (see my comment). I don't know if my suggestion makes sense, but if, I will give it another review.
So lots of comments were made, I hope it was not too much. I am looking forward to get this integrated soon and as clean as possible. You made a really great job, USB is super tricky and overall this patch looks super good!
I am on vacation for multiple (2-4) weeks now, so I am sorry if I do not respond in time. Just wanted to let you know, as I really would like to continue on this after my vacation! If I miss the post, please ping me again.
Thanks a lot!
examples/MultiTouch/MultiTouch.ino
Outdated
| int16_t x = 2000, y = 2000; | ||
|
|
||
| for (; x <= 8000; x+=10) { | ||
| for (int i = 0; i < 7; i++) { |
There was a problem hiding this comment.
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.
"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.
And why do you define 10 fingers then? I think I dont understand that correct.
There was a problem hiding this comment.
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:
- Each report has 2 fingers
- If more than 2 fingers is present, we send more reports. Hybrid mode defines how to do it.
- We can send as many reports as desired, but Windows won't process more than 10 (software restriction I guess)
- Windows will say 10-finger support in System Properties
- We don't have a 10-finger report because it will be unnecessarily large for the common case where only 2 fingers are expected. Maybe there are also size limits so we can't pack like 100 touches into a single report, but still able to report them through 20 reports with 5 touches in each.
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.
|
Thanks for the comments. I admit that the PR code is some quick implementation and not really polished. I'll get to the details soon. I'm not in any rush to get them merged but will do them sooner than later. Enjoy your vacation! If you haven't worked with touch digitizers before I can imagine some of the details are not very straightforward compared to other HID devices, especially when Microsoft outlines additional requirements. The main two issues are "Feature Report" and "Hybrid Mode", and I'll summarize them here. Feature Report. Microsoft requires it. If you recall in the discussion of #123 Eric posted a working gist of 10 points for Android. It's my starting point, but Windows see it as a single touch device. I think the feature report is what Windows uses to decide a touchscreen is more than single-touch. In order to support feature reports, I made this "single-report". It's not really for omitting report-id, but for PluggableUSB because PluggableHID does not seem to give me the opportunity to respond to a feature report request. I still included report-ids in the descriptor because that page also lists report-id as mandatory, and I'm not sure if feature reports would work without report-id. Honestly I was trying to implement feature reports by retrying random stuff relevant until my System Properties says 10-point support rather than single-touch support, and I'm not sure if I'm doing it actually correctly as the USB docs are too intimidating to navigate through. Hybrid Mode. In my submitted code by default, each report only contains 2 touches, but a maximum 10 touches can be reported. When a single report does not contain all touches, hybrid mode allows sending multiple small reports in series to report all touches. Of course one could create a descriptor to contain all possible touches, but I was looking at a case of max ~40 touches but only ~5 is expected normally. Having a descriptor that big seems unnecessary and I'm not sure if there is some size limit on it. Details of how it works can be found here. With this reporting mode, one report does not contain all information. So I have a different data structure that tracks all contacts. The actual report(s) is generated from it during send. I think we can eliminate the report member storage from the class, but a H2D control transfer may set the input report and referenced it. It doesn't make sense to me why the host OS ever want to set a input message, and if we are sure a set input report won't happen we can remove it. Let me know you have further issues, and I look forward to making it part of this awesome library! Thanks. |
|
You can use a feature report within every hid device. For example I am using it optional in the keyboard: Now the question is, how to create a valid report, that contains the feature description in a single report. I think this was the tool, that can be used to build reports: Nevertheless I think your solution could just work be removing the report IDs. But if not that should be also fine, as its an interface on its own. If we change the API to my suggested solution (assuming my idea would work), we do not need to keep track of the pointer states, right? This way we can remove all structs to track the data. I'd leave that tracking up to the usercode, if possible. This saves us lots of ram. If that is our decision, we must of course remove the input callback. It see that you've copied it from the keyboard, I also dont know why it was there. I think for bootmode support, but that is not important here. I think it would make most sense if you could cleanup this code first and then continue discussing :-) |
|
I've pushed my cleanups. I'm not sure if I understood you correctly, but feature report is a concept parallel to input reports. A feature is not part of an input report. Feature reports travel through the control interface while input reports travel through the input interrupt interface (except when being polled which we don't handle and the OS normally don't do). You can see that it's part of setup and sent with It works with all report-ids removed, although MSDN specifies them as mandatory... For the API I'm afraid it doesn't work nicely. An individual report contains less information than the fingers array due to hybrid mode. The send procedure is more like encoding the current state into a series of reports in a particular format and pattern. Although the reports can be parse back to touches, but it requires encoding and decoding and is not so straightforward like other HID descriptors. The input required to generate the reports is all active touches and just-released touches. Given the Arduino environment, first I don't think the user would be happy to provide a linked list or condensed array of active fingers. So an array of both active and inactive touches seems required. In that case, in terms of memory usage I don't think the user can possibly use less memory. (If they support less fingers, they should change the parameter) I believe offloading the tracking task to the user creates confusion, and I don't really see further simplifications to the current API. Regarding higher-levels APIs to make gestures in a single call, I think it's definitely doable and can be relatively easily implemented with current APIs, but I'm unsure what would be a good collection of gestures. If the user needed most simple single-point gestures I think an AbsoluteMouse may be better. Multi-point gestures seems too diverse to cover in a few functions, especially that blocks when the gesture is being performed. If you have ideas feel free to just add them. |
|
|
||
| class TouchscreenAPI | ||
| { | ||
| public: |
There was a problem hiding this comment.
I've not written c++ for a while, but why dont you have a constructor? I can see, that I am clearing the data in the other constructors, why dont we do that here as well?
https://github.com/NicoHood/HID/blob/master/src/HID-APIs/MouseAPI.hpp#L27
There was a problem hiding this comment.
Implicit default constructor is specified to zero everything in C++. But for consistency I added one anyway.
There was a problem hiding this comment.
I remember that this enlarges the sketches flash, is that still correct? I am wondering why I've added it in the first place then...
There was a problem hiding this comment.
I shouldn't as it's the exact same behavior as the default constructor. I believe the compiler can figure out that static linking is all required for the global instances and no extra assign-to-zero code would be generated.
There was a problem hiding this comment.
Can you test that real quick? It it adds additional overhead we should remove it from all other classes as well, I'd say.
There was a problem hiding this comment.
The explicit initializer list in the constructor (not the constructor itself) made the example program text 32 bytes larger. In disassembly gcc inserted runtime code to zero memory. If the commonsense in x86 that global data are zeroed during exe image loading also applies to AVR (in my test instance it does), then it is preferable to not include an explicit initializer list.
I'm afraid I do not have the free time to perform project-wide polishing for you. I've removed the initializer list from this case. If there are no other touchscreen-specific issues, could you merge and publish this and then perform any other optimizations you'd like? Thanks.
Good work!
Correct!
USB is a mystery! If it works, that is great. (Still confirms I have not forgotten everything about USB XD )
Now I am very new to this, and I really need to do deeper into this. If I could, I would really like to improve the API. But I am not sure if I am really right with that idea, it could be really that I dont understand the concept. The reason why I'd like to discuss this, is that I do not want to add a breaking change to the API later on, so we should design a good API now! The rest of the PR should be fine now, I think. Just correct the few more annotations and we are mostly good to go. |
|
Please make sure to also add the touchscreen to the list of supported devices in the readme. |
|
All fixes has been pushed. I don't feel like designing a optimal high-level API at this moment because I don't really understand the high-level demands either. My use case has to directly use the setFinger and releaseFinger and they appear low-level enough that almost any higher-level APIs would call them. So I think we are good to publish with only these two essential APIs, and leave higher-level gestures open until there's a clearer pattern of uses or someone come up with a better design. |
|
Could you please also integrate this patch: |
Merged. |
|
What I meant was that you should also apply the fixes made to the other device, to your touchscreen device. |
Done |
|
@NicoHood is anything blocking this PR to get merged? |
|
Good question. Will place it on my TODO! Give me some more time, please. |
|
@NicoHood what about now? 😂 |
|
@NicoHood ping again :) |
|
I'm trying the Touchscreen object but am getting some fundamental compile errors. Do I need a newer version of Arduino C:\Users\tozz\AppData\Local\Temp\arduino_build_492947\libraries\HID-Project\HID-Project.a(Touchscreen.cpp.o): In function |
|
@tozz88 Looks like pluggableUSB functions are not being linked. IDE 1.8.13 should be fine. Are you sure you are using a board with native USB support? Do the rest of the HID library (especially single report devices) work fine? |
|
Hey guys, |
@ilufang, Yes I'm using an MKRZERO board and have been using Multi-Report devices in this library successfully for years. It is SamD21 based. However, if I try any of the Single-Report devices I get the 4 missing entry points. It doesn't have to be Touchscreen. I tried SingleAbsoluteMouse and it also fails the same way. I assume Multi-Report also depends on pluiggableUSB? Is there some way I can inspect my pluggableUSB library? where does it reside on my system? Is it possible that SamD21-based boards support Multi-Report but not Single-Report? thanks for your help, |
|
@tozz88 I did a quick search through the Arduino cores and it appears that samd has a different interface than avr for some of its usb functions:
So if you compare the PluggableHID implementation of AVR to that of SAMD, you can see that a few calls to core usb routines may need to be replaced. Ideally Arduino library maintainers should make a better PluggableUSB, but for the time being, you can either edit the Touchscreen source, #define replace the USB_* functions, or if you prefer not to touch the library, link wrapper functions of the missing symbols. (BTW, don't you get warnings of using those functions without a header declaration? If so, make sure to fix the headers too) Unfortunately I do not have an SAMD-based device to test, so good luck on doing the workaround. |
|
@NicoHood any news? |
Hi, I was trying to implement an Arduino project that would be recognized as a multi-touch screen by Windows when I saw #123 . After messing around with information from there and a whole bunch of random places on the Internet, I am able to get it work on my PC by adding a new device under the frameworks of this library. Here's my code that does it and I hope you find it useful.
A few implementation notes not already mentioned in the comments:
This is the first time I tried to work with USB, so please correct me if my understanding, code or anything is incorrect.