Create unions for fields we receive from the host with backing buffers that are modulo 16 bits in size.

[obra]

buffers that are modulo 16 bits in size.

This is currently necessary because these fields are read directly from the USB peripheral with ‘usbd_ep_data_read’, which can only read in 16-bit chunks (as of version 2.1.2 of the firmware library).

[gedankenexperimenter]

Is the problem that (on GD32), USB_RecvControl(&foo, 1) writes two bytes to foo instead of one? That seems like an obvious bug in the underlying GD32 code (or a very misleading API), and even if it's not practical to fix it at the source, this change does not strike me as the best workaround.

[obra]

It writes multiples of 16 bits. It is absolutely a bug in Gigadevice's libraries, but one we have to live with for now.

@bjc - Would @gedankenexperimenter's alternate fix work?

[bjc]

Yes, @gedankenexperimenter's thought occurred to me as well. I preferred doing it in the union to avoid the extra copy, and because I felt it was better documented there (less likely to be lost while browsing code).

He is correct that it screwed up the use of the enclosing setReportData because now it's one byte larger. I missed that in my review.

It wouldn't have shown up in testing unless you inspected it in gdb, unfortunately. Since the size was now incorrect, when the host would attempt to HID_SET_REPORT with both the report id and leds, the test wouldn't have matched, and nothing would be done.

[gedankenexperimenter]

Incidentally, this change requires a change in Kaleidoscope, as well, where the virtual HID implementation replaces the one from KeyboardioHID. That may be irrelevant, but I think it would be a lot cleaner, and more comprehensible to read the data into a temporary local variable first, and isolate the changes for this workaround to one or two spots, inside method definitions. It took me a good hour of head-scratching to figure out what was going on with this union, and I'm still not sure I understand it fully.

[bjc]

IMHO there's going to be head-scratching no matter what, either "why is this a union", "why is there padding", "why are we reading into a temporary?" I can't say which one will be easier to reason about.

This is a union, because originally I added a padding byte to the requisite classes after odd-sized-byte fields that are directly read by USB_RecvControl. Because that function calles into usbd_ep_data_read as its method of pulling data from the USB, it requires buffers that are 16-bit mulitples in size (2 bytes, 4 bytes, 6 bytes, etc) because that function does a cast of the buffer into uint16_t* and writes into it that way (presumably because reads from the peripheral bus have to be 16 bits wide, and they didn't code a temporary for odd-sized cases).

So my code looked ilke:

    uint8_t someBuffer[7];
    uint8_t _padding;

That way the overflow byte of garbage from the peripheral bus didn't corrupt any legitimate fields that would have come after someBuffer.

I thought this would work, but @obra had worries that it might be optimized out is -Os scenarios. I, personally, don't think that's possible, but I'm unable to provide any proof to back that claim up. So an alternate solution of using a union was made, where there's the "real" data field, and a backing buffer of that data field, plus an extra byte to make sure overflows went somewhere harmless:

union {
    uint8_t someBuffer[7];
    uint8_t _padding[8];
} wrapper;

The _padding field only exists as a place to store that bogus byte. All accesses should go through the someBuffer field. This does have the effect of making wrapper too big, but that also shouldn't be referred to in the implementation code, only its singular, valid, inner field wrapper.someBuffer.

If I had my druthers, I'd go back to just adding a padding byte, so if you know for sure that it can't be optimized out (since it's never directly used), then I'd love to hear it. In the mean time, for me, the union approach seemed better than writing to a temporary of whatever size, adjusting the calls to USB_RecvControl to take sizeof(temporary)-1 and then copying that temporary (minus 1 byte) into the final field.

With the union, I can slap a very fat comment on it explaining what's going on, and it seems a natural place, rather than doing it in the setup functions for the various drivers where it'll be necessary.

Of course, this is all completely subjective. I felt this was better, but that's just, like, my opinion, man. In the end this is much more your code than mine, and as long as the problem's fixed, I'm not invested in any particular solution enough to get into a shouting match. Hopefully it'll all go away when gd fixes their end anyway.

[gedankenexperimenter]

With the union, I can slap a very fat comment on it explaining what's going on, and it seems a natural place, rather than doing it in the setup functions for the various drivers where it'll be necessary.

The problem is that the very fat comment is (a) in the class definition, not in the method that's actually involved in the data transfer, and (b) almost totally incomprehensible to someone like me who is (1) unaccustomed to unions in C++ (apart from knowing that there are some gotchas), and (2) needs to see the code to really understand explanatory comments (even ones that are very well-written and clear) — code that is not even in the same file as that comment.

Writing into a temporary variable, with an array that is explicitly marked as being the size needed to work around the bug, then reading the specific bytes from that array into the member variables would mean the header file isn't affected, and it's much easier to remove the workaround after the bug is fixed. I would just comment out the code that we eventually want, too, so that restoring it would be trivial.

This is a special case because, despite being a private member data structure, it's also in a sense a part of a public API because of the virtual HID device that uses the same header file, but a different implementation for the testing simulator. And that source file lives in a different repository than this one, which makes it an even bigger nuisance to deal with, because a change here is not automatically synchronized with a change there.

[obra]

I think it sounds like we're going to get the bug we're working around fixed in pretty short order and can then revert this.

On Mon, Apr 4, 2022 at 4:57 PM Michael Richters @.***> wrote:

With the union, I can slap a very fat comment on it explaining what's going on, and it seems a natural place, rather than doing it in the setup functions for the various drivers where it'll be necessary.

The problem is that the very fat comment is (a) in the class definition, not in the method that's actually involved in the data transfer, and (b) almost totally incomprehensible to someone like me who is (1) unaccustomed to unions in C++ (apart from knowing that there are some gotchas), and (2) needs to see the code to really understand explanatory comments (even ones that are very well-written and clear) — code that is not even in the same file as that comment.

Writing into a temporary variable, with an array that is explicitly marked as being the size needed to work around the bug, then reading the specific bytes from that array into the member variables would mean the header file isn't affected, and it's much easier to remove the workaround after the bug is fixed. I would just comment out the code that we eventually want, too, so that restoring it would be trivial.

This is a special case because, despite being a private member data structure, it's also in a sense a part of a public API because of the virtual HID device that uses the same header file, but a different implementation for the testing simulator. And that source file lives in a different repository than this one, which makes it an even bigger nuisance to deal with, because a change here is not automatically synchronized with a change there.

— Reply to this email directly, view it on GitHub https://github.com/keyboardio/KeyboardioHID/pull/83#issuecomment-1088131809, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2E3DDKOOR3S6FLFQSLVDN6UZANCNFSM5SN4TC4A . You are receiving this because you were mentioned.Message ID: @.***>