usbd_ep_send in the vendor firmware has a race condition where it can enable transmission on the endpoint before updating the usb_transc object that the ISR will update. This might be safe within the ISR, but the function can be called from outside the ISR.

Even with interrupts disabled, the USBD peripheral runs asynchronously from the CPU core, and can change registers in ways that conflict with the firmware's modifications. This can lead to race conditions.

We should use "invariant" values for unchanged values when modifying EPxCS registers: zeroes for toggle bits, and ones for clear-only bits.

This is a little difficult to describe. The vendor USBD firmware has a bug where if an IN transaction completes on an endpoint as an OUT transaction completion is being handled, and is followed in quick succession by another OUT or SETUP transaction, the OUT or SETUP transaction will take priority over the IN transaction. This is mostly relevant for control transfers. This means that multiple Status IN completions can shadow each other, so that the handler executes only once.

This might cause missed notification of completion events, and corruption of some control transfers. It's definitely visible with trace logging turned on, because logging with reasonable UART speeds adds enough delay to exhibit the problem.

I've been running into what appears to be an instance of a documented hardware erratum in the I2C peripheral. Interrupts that delay the reading of I2C_DATA can cause invalid data to be read. See Device Limitations of GD32F30x, Version 1.0, 2022-06-20.

The erratum note says that "I2C will read an additional data", implying that the controller will clock in an additional byte from the target. That doesn't appear to happen, as far as I can tell. What appears to happen instead is data corruption: the controller will erroneously duplicate the previously-read byte instead of reading in the next byte.

Observed effects: with USB trace logging active to the hardware serial port, I sometimes got random activations of the Consumer Control volume keys when pressing arrow keys. (These parts of my keymap are the default.) Also, when pressing keys on the right half, Num Lock would spuriously activate. I couldn't make this happen reproducibly, but when it did happen, there tended to be bursts of the corruption. Notably, the Consumer Control key that activated seemed to correspond to the arrow key immediately above it in the same column, which makes sense given the hardware key map. The spurious Num Lock activation seemed to be caused by duplication of the 0x05 length prefix byte.

I was unable to reliably reproduce the problem by making the trace logging emit more characters. I did eventually add a delay directly in the I2C receive function, which did replicate the symptoms that I previously observed.

I have a fix in progress that appears to work so far.

USB_Send currently blocks forever if there's nothing polling the endpoint. We should fix this. AVR has a 250ms timeout, though it's not clear that's the best choice for us.

The vendor firmware has a bug where it clears the RX_ST flag too early. An undocumented feature of this flag (though partially documented in the documentation of the STM32F1 peripheral that the GD32 one seems to be trying to emulate) is to "freeze" the SETUP flag in USBD_EPxCS so that incoming tokens won't change it. If RX_ST is clear, then an OUT token not associated with a SETUP transaction can still clear the SETUP flag, even if the transaction is NAKed by the peripheral.

The result is that an incoming SETUP transaction is processed as an OUT transaction, which can lead to all kinds of chaos.

There are a few flags in USBD_CTL that can be changed asynchronously by hardware. To avoid race conditions, we should enable and disable interrupts at the NVIC instead of by read-modify-write operations on USBD_CTL.

The AVR core uses double buffering for the non-default endpoints. GD32 only supports double buffering for isochronous and bulk endpoints, so we can't use it for interrupt endpoints. (The GD32 also has only 512 bytes of USB packet RAM, less than the AVR.)

We should emulate double buffering, given that we're already maintaining a buffer for each endpoint, to accumulate multiple writes for some situations, etc.

One scenario that could cause problems is a user pressing and releasing a key to resume from suspend. On AVR, this enqueues two HID reports (one key down, one key up) on the peripheral, and nothing blocks or times out until the next transmission attempt on that endpoint. On GD32, the key up event blocks (or times out, if patched to implement timeouts).

Another scenario is the null reports sent at startup. This causes the M100 to lock up when talking to a UEFI or BIOS that only talks to boot devices, because the multi-report endpoint send multiple null reports.

In the longer term, GD32 should implement timeouts, and KeyboardioHID should be more aware of timeouts. For now, emulating double buffering on GD32 might mitigate some of the existing problems.