Fix some issues regarding getReport and setReport

[juico]

When i was trying to fix the Alps4TUSB from blankmac i ran into some issues with setReport and getReport.

For the setReport i looked up the linux i2c-hid code and there they skipped the first byte of the featureReport as it contains the report_id. Later in the code they add the report_id to the message again. In the voodooI2CHID it is not removed in the first place but is added again like in the linux driver. This introduces a double report_id and thus corrupts the featureReport.

For the getReport function there seems to be a issue that the last two bytes of the report are not written. Fixed by making the readbuffer two bytes bigger and writing the complete result back into the report memoryDescriptor.

These two fixes make the communication part of blankmac's code work. As his code works when using USB and not using I2C i am convinced these fixes are needed to make voodooI2CHID work better.

[ben9923]

Thanks a lot!

Can someone with an I2C-HID precisiopn touchpad device (Basically most I2C VoodooI2CHID users) test this branch to confirm it doesn't break anything?

[mishurov]

Doesn't work. Asus UX310, ELAN1200. Here're some related lines from the log:

VoodooI2CHIDDevice:0x1000002d2 Matching has vendor DeviceUsagePage : ff0c bundleIdentifier com.apple.AppleUserHIDDrivers ioclass AppleUserHIDEventService but transport and vendorID is missing

IOHIDPointingEventDevice:0x10000038a Matching has vendor DeviceUsagePage : ff0c bundleIdentifier com.apple.AppleUserHIDDrivers ioclass AppleUserHIDEventService but transport and vendorID is missing

VoodooI2CPrecisionTouchpadHIDEventDriver id:0x100000375 primaryUsagePage:0x1 primaryUsage:0x2 transport:I2C reportInterval:8000 batchInterval:0 events:0 mask:0x0

[juico]

Thank you for testing, I am not quite sure what goes wrong in your case. My first guess would be in VoodooI2CPrecisionTouchpadHIDEventDriver::enterPrecisionTouchpadMode() as there setReport() is used.

Maybe you can try to see which of the two changes i made breaks your touchpad.

Perhaps the fix i proposed allows for the method suggested in the comments of the function below and use the SetValue instead of manually writing a report. It is a pitty i dont have the hardware to test it but i will try to take a better look at the code when i have some more time.

void VoodooI2CPrecisionTouchpadHIDEventDriver::enterPrecisionTouchpadMode() {
    // We should really do this using `input_mode_element->setValue(INPUT_MODE_TOUCHPAD)`
    // but I am not able to get it to work.
    
    //VoodooI2CPrecisionTouchpadFeatureReport buffer;
    //buffer.value = INPUT_MODE_TOUCHPAD;
    //buffer.reserved = 0x00;

    //IOBufferMemoryDescriptor* report = IOBufferMemoryDescriptor::inTaskWithOptions(kernel_task, 0, sizeof(VoodooI2CPrecisionTouchpadFeatureReport));
    //report->writeBytes(0, &buffer, sizeof(VoodooI2CPrecisionTouchpadFeatureReport));
    //hid_interface->setReport(report, kIOHIDReportTypeFeature, digitiser.input_mode->getReportID());
    //report->release();
    digitiser.input_mode->setValue(INPUT_MODE_TOUCHPAD);
    ready = true;
}

[mishurov]

In Linux, my primary OS, in userspace, I can do this:

#define INPUT_MODE_REPORT_ID 0x3
#define INPUT_MODE_TOUCHPAD 0x03
#define LATENCY_MODE_REPORT_ID 0x7
#define LATENCY_MODE_NORMAL 0x00

unsigned char buf[2];
buf[0] = INPUT_MODE_REPORT_ID;
buf[1] = INPUT_MODE_TOUCHPAD;
res = ioctl(fd, HIDIOCSFEATURE(3), buf);

Just using the file desctiptor /dev/hidraw0 and writing into it. In kernel space it is a bit more complicated since it uses struct hid_device and its functionality. I am not quite sure what goes wrong in your case. But I'll try your fix when i have some more time.

[ben9923]

@mishurov Is master properly working for you?

Indeed this PR may break the current I2C code, as the PTP entry code is kind of a workaround (Not sure if it properly follows the i2c-hid spec).
Please try the suggested changes. They are part of #48 too :)

[mishurov]

@ben9923 yes, master works, I just wanted to check if the PR fixes this issue VoodooI2C/VoodooI2C#321 but it doesn't work at all. I'll try the fix when I have free time or when I need to do some work in MacOS.

[mishurov]

@ben9923 yes, the suggested changes do work. Doesn't solve the aforementioned issue with random blackouts though, unfortunately.

[ben9923]

@mishurov Thanks for testing it!
I didn't really think it would solve the other issue though...

[ben9923]

@juico Just found the time to properly review it.

Good catch on the getReport fix, looking at the I2C-HID spec clearly shows the problem (two extra bytes for length at the beginning).

Regarding setReport, I see that the Linux kernel just checks if the first byte is nonzero, then 'marks' it as reportID and removes it from the buffer. Would you explain why your code behaves differently? :)

Also, any idea why it's even transferred as part of the report in the first place? I see macOS is using the options argument.
In Linux, when the reportID argument is 'introduced' in the code flow, it's eliminated from the buffer.

Thanks again! I would be happy to merge this in soon, just making sure we don't break anything by accident.

[juico]

@ben9923 Thanks for checking it out. For the setReport i remember comparing the first byte against the supplied reportID from the options argument as this is expected to be the case. Just checking for nonzero byte would also work, perhaps that would make more sense and just return a error if the reportID in the first byte doesnt match that of the one supplied in the options argument. Some of the other differences are due to the differences in the types of buffer used but it is possible there is a cleaner solution.

Not sure why it is a part of the report itself. I tried to figure that out but had some trouble finding clear documentation about it. So i ended up using the webHID interface of chrome to call setReport and checked how it looked like by logging the reports fed to setReport. That lead to the conclusion that internaly the reportID is placed a the beginning of the report itself and in the options argument.

In terms of breaking stuff it seemed that by merging the master branch to my fork it broke some things for me, havent had time to check out why this is the case. Perhaps if i have some spare time in the weekend so i can figure out why it breaks.