midi: allow bigger sysex messages to be sent, such as the OLED image by bfredl · Pull Request #174 · SynthstromAudible/DelugeFirmware

bfredl · 2023-07-08T19:18:51Z

Some simplifications were made to the driver code by Rohan under the
assumption we were only going to queue up to 32 usb-midi messages
for a device per "tick" of the main routine.
These had to be reconsidered, like there is now again a callback from the usb driver
interrupt back to midi_engine to check for more data to send.

It doesn't seem to be possible to increase the amount of data sent in
each call to the driver more than 32 messages (or 128 bytes).
Instead, implement a ring buffer for midi data waiting to be sent over USB.
When we get an interrupt for usbSendComplete, we can check the ring
buffer again and restart the low-level send until the ring buffer is
empty.

Implement a sysex request for sending the contents of the OLED display
as a sysex reply message. This needs more work before being
generally useful as an external display (not to mention client code for multiple platforms)
, but serves as a test case for larger sysex messages. This also tangents #121 , an unified build
would eventually allow developers with a 7SEG display to test how their code
would look on a virtual OLED display.

bfredl · 2023-07-08T19:20:48Z

I have tested these for

serial midi
deluge acting as a USB device
deluge acting as USB host

I will try to find a way to test the last one before marking this as ready

jamiefaye · 2023-07-09T02:35:00Z

Looking forward to this one going in.

Some simplifications were made to the driver code by Rohan under the assumption we were only going to queue up to 32 usb-midi messages for a device per "tick" of the main routine. These had to be reconsidered, like there is now again a callback from the usb driver interrupt back to midi_engine to check for more data to send. It doesn't seem to be possible to increase the amount of data sent in each call to the driver more than 32 messages (or 128 bytes). Instead, implement a ring buffer for midi data waiting to be sent over USB. When we get an interrupt for usbSendComplete, we can check the ring buffer again and restart the low-level send until the ring buffer is empty. Implement a sysex request for sending the contents of the OLED display as a sysex reply message. This needs more work before being generally useful as an external display, but serves as a test case for larger sysex messages.

…icies These objects are intended to be used this way. If disconnected these get disabled, but not pointers into invalid memory above class MIDIDevice: > These never get destructed. So we're safe having various Instruments etc holding pointers to them.

bfredl · 2023-07-09T18:49:01Z

Update: sending as a host to devices now works for most devices. a buffer limit of 64 bytes seems common, so lets use that for devices. some devices have even lower limits tho, like my MIDIMATE II serial MIDI to USB converter which only accepts sending one (1) midi packet at a time. (We could try to figure this out from the configuration data, I tried with the bMAXPacketSize field but it doesn't seem to be the right one).

m-m-adams

I've added some comments where I think this could be improved a bit.

The only big issue is that the current behaviour of send all messages to all devices doesn't work well for sysex since it's much more data and sysex parsing bugs are very common (especially with older gear). In peripheral mode that's fixable by enabling the third upstream port (already in the config), in host mode it would need some extra code to check that the device being sent to is the one requesting data

m-m-adams · 2023-07-10T02:15:57Z

 class MIDIDeviceUSBUpstream final : public MIDIDeviceUSB {
 public:
-	MIDIDeviceUSBUpstream(uint8_t portNum = 0) { portNumber = portNum; }
+	MIDIDeviceUSBUpstream(uint8_t portNum = 0) : MIDIDeviceUSB(portNum) {}


Is the portnum still necessary after giving it a default value in the mididevice USB class?

m-m-adams · 2023-07-10T02:17:51Z

-// (3 payload bytes per USB-MIDI message)
-#define MIDI_SEND_BUFFER_LEN 32
+// NOTE: increasing this even more doesn't work.
+// Looks like a hardware limitation (maybe we more in FS mode)?


What happens?

I've just been reading the the usb midi spec and we might be able to use the maximum buffer size specified by a downstream device instead of this guesswork.

m-m-adams · 2023-07-10T02:39:29Z

+
+	for (int d = 0; d < MAX_NUM_USB_MIDI_DEVICES; d++) {
+		if (connectionFlags & (1 << d)) {
+			connectedDevice = &connectedUSBMIDIDevices[ip][d];


I don't believe this works properly, won't it just send to the first connected device? Since each ConnectedUsbMidiDevice holds an array of 4 USBMidiDevices it should check that the current device is actually held by the connecteddevice it finds

This follows the existing logic in sendMessage and it does seem to work. connectionFlags is private to the inner USBMIDIDevice and should only have a flag set for the specific connected device. It's a bit weird way of structuring things tho.

What I think we don't support at all is virtual port/cable differentiation for a specific connected device. That would be an important follow-up refactor.

bfredl · 2023-07-10T08:37:27Z

The only big issue is that the current behaviour of send all messages to all devices doesn't work well for sysex since it's much more data and sysex parsing bugs are very common (especially with older gear). In peripheral mode that's fixable by enabling the third upstream port (already in the config), in host mode it would need some extra code to check that the device being sent to is the one requesting data

This is already what this branch implements. Sysex replies are being sent to the specific device that sent a request. not sure what makes it seem like otherwise.

I agree the thrird port should be added at this point, but differentiation between virtual ports is already implemented (request the oled data on virtual midi out 2; get a reply on virtual midi in 2).

bfredl · 2023-07-10T10:04:16Z

Thanks for the review. I tried to make the naming of new stuff more consistent and added a bit more comments.

m-m-adams · 2023-07-10T12:35:33Z

This is already what this branch implements. Sysex replies are being sent to the specific device that sent a request. not sure what makes it seem like otherwise.

My assumption was based on the original firmware behaviour on flush to send the buffered midi to all devices and I don't see logic that changes that. It's differentiated by midiDeviceUSB but that's not respected by the flush functions. If I've misread the code to change that then no worries, I've got nothing that can be hosted and log midi messages to test with so that statement is purely based on reading the code

I agree the thrird port should be added at this point, but differentiation between virtual ports is already implemented (request the oled data on virtual midi out 2; get a reply on virtual midi in 2).

Yeah it's not a huge deal, it just needs to be declared with the other 2. It wouldn't have to be added to the menu structures or preferences files since it should just be used by a companion app or similar

bfredl · 2023-07-10T12:42:43Z

My assumption was based on the original firmware behaviour on flush to send the buffered midi to all devices .

what happened is that all devices gets flushed at the same time even though one device's buffer was full. I'd like to untangle that temporal mix-up at some point as well. But there was still a dedicated buffer for each physical device so it was always possible to send messages to a specific device (even though with note/cc data they often contained exactly the same contents).

m-m-adams · 2023-07-10T13:03:35Z

Makes sense, thanks

bfredl · 2023-07-10T14:34:03Z

Added the third port dedicated for sysex data (it never sends channel/MPE data).

I kept some refactors in the midi device menu even though I didn't add the new port to the menu right now, as there is no relevant settings. we could add it later by just adjusting `lowestDeviceNum`

dctucker · 2023-07-11T06:07:32Z

+
+	rle = false; // not yet implemented
+
+	uint8_t reply_hdr[5] = {0xf0, 0x7e, 0x02, 0x40, rle ? 0x02 : 0x01};


If I'm reading this correctly, the message being sent is "Universal Non-realtime sample data packet".

Should this be changed to use 0x7d as the second byte, or is there a specific reason this message type was chosen?

no you are right, it is supposed to be 0x7d

bfredl force-pushed the oled_sysex branch 2 times, most recently from fe72bae to b39b9b2 Compare July 8, 2023 20:20

bfredl force-pushed the oled_sysex branch from 6852f6b to 8812585 Compare July 9, 2023 18:00

bfredl added 3 commits July 9, 2023 20:02

midi: fix for sending as a host to a device

8812585

bfredl marked this pull request as ready for review July 9, 2023 18:51

m-m-adams requested changes Jul 10, 2023

View reviewed changes

cleanup and address review comments by m-m-adams

606c770

midi: add third midi port dedicated for sysex

8fb4679

I kept some refactors in the midi device menu even though I didn't add the new port to the menu right now, as there is no relevant settings. we could add it later by just adjusting `lowestDeviceNum`

jamiefaye added this pull request to the merge queue Jul 10, 2023

Merged via the queue into SynthstromAudible:community with commit 7dbb780 Jul 10, 2023

dctucker reviewed Jul 11, 2023

View reviewed changes


		rle = false; // not yet implemented

		uint8_t reply_hdr[5] = {0xf0, 0x7e, 0x02, 0x40, rle ? 0x02 : 0x01};

Uh oh!

Uh oh!

Conversation

bfredl commented Jul 8, 2023

Uh oh!

bfredl commented Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamiefaye commented Jul 9, 2023

Uh oh!

bfredl commented Jul 9, 2023

Uh oh!

m-m-adams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

m-m-adams Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m-m-adams Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

bfredl Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

m-m-adams Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

bfredl Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

bfredl commented Jul 10, 2023

Uh oh!

bfredl commented Jul 10, 2023

Uh oh!

m-m-adams commented Jul 10, 2023

Uh oh!

bfredl commented Jul 10, 2023

Uh oh!

m-m-adams commented Jul 10, 2023

Uh oh!

bfredl commented Jul 10, 2023

Uh oh!

dctucker Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

bfredl Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bfredl commented Jul 8, 2023 •

edited

Loading