Add retry count for USB MIDI Tx

[ndonald2]

Summary

Fix an issue where attempting to send MIDI messages over USB in rapid succession would frequently fail due to the USB CDC driver Tx not having finished yet and still being in the "busy" state.

Details

I noticed that attempting to send a bunch of messages immediately back to back from Daisy over USB MIDI was causing some messages to be lost. After some investigation I was able to trace this to the USB CDC implementation (in usb_cdc_if.c):

uint8_t CDC_Transmit_FS(uint8_t* Buf, uint16_t Len)
{
    uint8_t result = USBD_OK;
    /* USER CODE BEGIN 7 */
    USBD_CDC_HandleTypeDef* hcdc
        = (USBD_CDC_HandleTypeDef*)hUsbDeviceFS.pClassData;
    if(hcdc->TxState != 0)
    {
        return USBD_BUSY;
    }
    USBD_CDC_SetTxBuffer(&hUsbDeviceFS, Buf, Len);
    result = USBD_CDC_TransmitPacket(&hUsbDeviceFS);
    /* USER CODE END 7 */
    return result;
}

The problem is that a previous transmit attempt had not actually completed yet - apparently this function does not block for the duration of the actual transmit, and the state flag isn't reset by the time the next transmit is attempted so it was silently failing.

The "fix" is to simply retry USB MIDI transmits up to a user-configurable amount of times (default 3) after a short delay. Using 100 microseconds for the delay is an empirical best guess. It solved the issue for my use case (sending a bunch of Sysex messages in a row).

Note, minor formatting changes are a result of running clang-format on these files after I added code.

[ndonald2]

I'm not sure why the clang-format checker failed. I ran clang-format -i <paths> on the files in this PR to double check and nothing changes from the commit.

[stephenhensley]

@ndonald2 its so weird, I've had this happen a few times depending on the workstation I'm on.

Honestly, not sure why even with the same .clang-format file and version it'll act different. Maybe undefinied behavior, or slight differences on different OSes.

The manual fix is just the spacing in this one line in usb_midi.h:

-    MidiUsbTransport(const MidiUsbTransport& other)            = default;
+    MidiUsbTransport(const MidiUsbTransport& other) = default; 

---

Anyway, thanks again for the PR this looks good. Should be able to merge it today.
Also, since you are obviously using MIDI tx via USB, have you tested any of the system realtime messages?
I opened #564 to resolve an issue reading MIDI (specifically 1-byte messages) from USB, but haven't had a chance to make sure sending them works, as well.

If you don't know off the top of your head, that's fine its not necessarily the same scope as either PR anyway.

[ndonald2]

@stephenhensley weird indeed!

Also, since you are obviously using MIDI tx via USB, have you tested any of the system realtime messages?
I opened #564 to resolve an issue reading MIDI (specifically 1-byte messages) from USB, but haven't had a chance to make sure sending them works, as well.

I haven't tried System Realtime yet, but that is definitely going to come up fairly soon for the project I'm working on. The way I discovered this bug was by sending System Exclusive messages in rapid succession, so at least can verify that works.

[stephenhensley]

@ndonald2
No worries, just figured I'd see if you ran into it!
Great to know about the sysex messaging working well, though!

[TheSlowGrowth]

@stephenhensley just chiming in for a comment on clang-format:

Honestly, not sure why even with the same .clang-format file and version it'll act different.

In most commerical repos I worked on in the past, we had a local clang-format binary commited to a tools/ subdirectory in the repo - to avoid these issues alltogether. VSCode has the benefit that we can specify a local clang-format executable in the repository's .vscode/settings.json file so that this local version is automatically used.
We could take this even further and install a git hook that, when you make a commit, automatically warns about wrong formatting, even before the commit is pushed to origin.

I'm a bit tight on time right now, but I'll see if I can make a PR to propose something like this.

[stephenhensley]

In most commerical repos I worked on in the past, we had a local clang-format binary commited to a tools/ subdirectory in the repo - to avoid these issues alltogether. VSCode has the benefit that we can specify a local clang-format executable in the repository's .vscode/settings.json file so that this local version is automatically used.
We could take this even further and install a git hook that, when you make a commit, automatically warns about wrong formatting, even before the commit is pushed to origin.

@TheSlowGrowth that's an excellent idea, and would certainly make things quite a bit easier for everyone.
If you don't find the time to make a PR, a link to an example repo that does either, or both of these things well would be a welcome alternative.

[stephenhensley]

@ndonald2 looks great! Thanks again for the PR!

[ndonald2]

Thanks! I also love the idea of auto-formatting git hooks. Curious if @TheSlowGrowth is referring to client-side git hooks?

For context, I think client-side hooks are fantastic for this kind of thing (and much more) however I'm not sure there's actually a way to enforce usage of them. My understanding is that users can install whatever client side hooks they want (or not) per repository so when I've seen them used on projects it's usually as an optional install-script in the repo that sets them up client side. Server-side hooks can also be useful (e.g. reject commit if it doesn't pass muster) but since Github doesn't allow user-configured server side hooks, you don't see them as often, except on private git servers.

In any case - +1 for automation tooling, usually a win for developer experience regardless of what form it takes.

[TheSlowGrowth]

Yes, what I meant are client side hooks.
Yes, they have to be installed once after initially cloning the repository. We could help users by providing a tasks.json entry for VScode, so that all they have to do is to run this task once.
My suggestion would be to use something like the pre-commit tool to handle all of this. The benefit of this tool is that it automatically downloads the correct clang-format binary. It can also be used on the client side as well as the server side (which we still need because the client side hooks are not enforce-able). The only problem of this tool is that it requires python, which is not included in the toolchain installer, as far as I can tell.
An alternative solution would be bash scripts and clang-format binaries commited to the repo. Maybe that would work better for us.

The workflow would be:

• developer makes a commit in their local working copy.
• if ill-formatted: commit is rejected by the pre-commit hook and the suggested changes are written to the working copy. User can just stage the changes and commit again. This time, the commit is accepted and can be pushed to origin.

I'll see if I can draft something, but as I said, I can't guarantee I can find the time...