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

HID functions to read back the status of an HID controller (like MIDI SYSEX) #3317

Merged
merged 32 commits into from
Feb 21, 2021

Conversation

JoergAtGithub
Copy link
Member

@JoergAtGithub JoergAtGithub commented Nov 14, 2020

The new hidapi 0.10 offers new functionality:
I implemented two new HID functions to read back the status of an HID controller:
QList getInputReport(unsigned int reportID);
QList getFeatureReport(unsigned int reportID);

Usage example 1 (Read and Set all buttons, knob or fader positions at Mixxx startup):

    // Set initial for all values of an InputReport to the bitwise inverted value
    // because the common-hid-packet-parser only triggers the callback
    // functions in case of a delta to the previous data.
    
    // 10 Byte InputReport with ReportID 0x01 
    var data = new Uint8Array;
    data = controller.getInputReport(0x01);
    var dataInverted = new Uint8Array(data.length);
    dataInverted[0] = data[0];
    for (let index = 1; index < data.length; ++index) {
        dataInverted[index] = ~data[index];
    }
    MyController.incomingData(dataInverted);
    MyController.incomingData(data);

Usage example 2 (Read and print state of Input an Feature reports):

    HIDDebug(controller.getFeatureReport(0xF1));    
    HIDDebug(controller.getInputReport(0x01));

EXAMPLE OUTPUT:
3 bytes received by getFeatureReport from Traktor Kontrol Z2 13EA_4 serial # C86013EA (including one byte for the report ID: F1 )
HID 255,255
10 bytes received by hid_get_input_report Traktor Kontrol Z2 13EA_4 serial # C86013EA (including one byte for the report ID: 01 )
HID 1,115,6,0,0,128,128,0,0,0

Usage example 3 (Set a single bit in FeatureReport):

   var data = controller.getFeatureReport(0xF1);  
   data[0] = data[0] | 0x02; // Set bit 2 in 1st byte of the report, to enable feature of the device
   controller.sendFeatureReport(data, 0xF1);

@JoergAtGithub JoergAtGithub marked this pull request as draft November 14, 2020 19:26
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping track of the latest additions ;)

Unfortunately we cannot safely assume that version 0.10 is available on all platforms. We need a minimum version check and static linkage if it fails.

@github-actions github-actions bot added the build label Nov 15, 2020
CMakeLists.txt Outdated Show resolved Hide resolved
<< "(including report ID of" << reportID << ")");
bytesRead -= kReportIdSize;

if (bytesRead == m_iLastPollSize &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty if arm is unusual.

Moreover this code for buffering and de-duplicating the input should be extracted into a private member function instead of duplicating it. I suggest to return a const char* pointer to the incoming data on success and nullptr otherwise. Pass the actual hidapi call as a function pointer, the signatures of hid_get_input_report and hid_read should match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or add an enum that you pass as argument and a switch case, might be easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved buffering and de-duplicating in a private member function, as suggested.
Regarding merging getInputReport and getFeatureReport I decided against it, for multiple reasons:

  • A function pointer would expose the symbols of the low level library below hidcontroler to one abstraction level above hidcontroller
  • The argument list would differ between the C++ method and the JavaScript proxy method
  • The internal buffer of de-duplicating code must never be used for a feature report. This would require copying of the buffer array.
  • Code differences would require multiple branches inside the function

src/controllers/hid/hidcontroller.cpp Show resolved Hide resolved
src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
controllerDebug(bytesRead << "bytes received by hid_get_input_report" << getName()
<< "serial #" << hid_serial
<< "(including report ID of" << reportID << ")");
bytesRead -= kReportIdSize;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stripping off the last byte unconditionally is wrong in many ways.

Depending on the arguments the report ID will be included in the result or not. And it will be included as the first byte, not the last byte. Please refer to the implementation in hid.c.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Windows implementation of hid_get_input_report in HIDAPI is wrong, it returns always a byte count incremented by 1. I started a upstream PR to fix this: libusb/hidapi#232
I don't know, if the implementations for the other platforms return the correct byte count.
Nethertheless, this has no influence on the Mixxx application logic. You just see one byte more than read from the device.

@uklotzde
Copy link
Contributor

@Be-ing @Holzhaus Btw, I suggest to drop the kReportIdSize constant, because the report id is by definition a single byte. This is obvious in most cases and if not we can add a comment like // strip report id from result. This comment is needed even with the constant definition, because a statement like bytesRead -= kReportIdSize; needs an explanation anyway.

JoergAtGithub and others added 2 commits December 28, 2020 00:30
Force static linkage if system HIDAP is older than 0.10.0
Suggested by Holzhaus

Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
@JoergAtGithub JoergAtGithub force-pushed the add_HID_readback branch 2 times, most recently from 8a40b82 to 2b74b98 Compare December 27, 2020 23:40
Co-authored-by: Be <be.0@gmx.com>
JoergAtGithub and others added 2 commits February 17, 2021 23:47
Co-authored-by: Be <be.0@gmx.com>
Co-authored-by: Be <be.0@gmx.com>
@JoergAtGithub JoergAtGithub marked this pull request as draft February 17, 2021 22:53
@Be-ing
Copy link
Contributor

Be-ing commented Feb 17, 2021

Please be mindful to type a useful commit message when applying suggestions from the GitHub web UI.

Removed class name before private method call
@Be-ing
Copy link
Contributor

Be-ing commented Feb 18, 2021

Considering getInputReport triggers the incomingData callback, could the script get into an infinite loop if getInputReport is called in an input handling function? If so, this should be documented.

@JoergAtGithub
Copy link
Member Author

Considering getInputReport triggers the incomingData callback, could the script get into an infinite loop if getInputReport is called in an input handling function? If so, this should be documented.

Good point! I'm considering to remove the automatic trigger of incomingData and let it up to the user to call incomming data on JavaScript level. This is maybe some lines of code more, but it's easier to understand for the mapping developer.

@Be-ing
Copy link
Contributor

Be-ing commented Feb 19, 2021

I agree that would be easier to understand. The call to getInputReport could return the data, so the script could do MyController.incomingData(controller.getInputReport(0xwhatever))

@JoergAtGithub JoergAtGithub marked this pull request as ready for review February 21, 2021 08:58
JoergAtGithub and others added 2 commits February 21, 2021 19:56
Co-authored-by: Be <be.0@gmx.com>
Co-authored-by: Be <be.0@gmx.com>
@Be-ing Be-ing merged commit 3d16052 into mixxxdj:main Feb 21, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Feb 21, 2021

Thank you!

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

Successfully merging this pull request may close these issues.

5 participants