register MIDI packet handlers as TMIDIPacketHandlerEx

[soyersoyer]

https://circle-rpi.readthedocs.io/en/47.0/devices/audio-devices.html#_CPPv4N14CUSBMIDIDevice21RegisterPacketHandlerEP20TMIDIPacketHandlerExPv

[github-actions]

Build for testing:
MiniDexed_2024-11-28-7800184
Use at your own risk.

[probonopd]

@soyersoyer what does this change solve that didn't work before?
Or is it about the reduction in lines of code?

[soyersoyer]

Reduces WTF/min while reading code.

There is a new RegisterPacketHandler function that can save a pointer, eliminating the need to write multiple packet handler functions.

[diyelectromusic]

Reduces WTF/min while reading code.

(But not WTF/min when reading PRs, but anyway...)

I'll have to look at the changes in circle before commenting, then follow through how the different instances are instantiated and parameters passed around and get back to you...

Has this been tested with multiple USB MIDI devices yet? That's the main risk I guess.

I'm not sure if the instance number should still be managed in midikeyboard anymore now or how that relates to CConfig::MaxUSBMIDIDevices which governs how minidexed invokes them. They were tied but now are separate, which is probably better, but I'm not familiar enough with how the two interrelate yet to know for sure... I'll try to have a proper look rather than guess.

(As Rene did all this code originally, I've only looked into changing things that needed changing in code he wrote as a rule - he is, after all, the definitive authority on circle! :))

Kevin

[soyersoyer]

The new RegisterPacketHandler was introduced here:
rsta2/circle@7930341

I tested it with 4 controllers on an RPI4.

The m_nInstance is really not used anymore, nice point.

[github-actions]

Build for testing:
MiniDexed_2024-11-29-9d5b6a2
Use at your own risk.

[diyelectromusic]

Do you know how nInstance, the USB device name umidi[nInstance+1] and nDevice in the handler are related?

I still wonder if miniDexed should not be passing in numbers at all, and instead CMIDIKeyboard should "know" how many instances have been instantiated... I guess that comes down to where the AddDevice ought to happen...

I also wonder if we ought to be doing something with nDevice - at least checking it is the right device for the handler or something like that (or don't we care? I'm not sure atm...

The problem might be reconstructing sysex messages - but they ought to be tied to the instance of CMIDIKeyboard, so there shouldn't be cross-over... but can that be checked? Is that what nDevice should be doing?

But as I say, I need to spend a bit of time untangling it all to see what is meant to be happening... (and that won't happen until later this weekend).

Kevin

[soyersoyer]

nDevice equals CUSBMIDIDevice::m_nDeviceNumber.
The CMIDIKeyboard is always registered to only one (or zero) CUSBMIDIDevice, so it will not receive messages with different nDevices.

I think nDevice was added to the parameters to give circle users more flexibility. So for example in a simpler case a static function could be enough to handle all midi devices.

[diyelectromusic]

Ok, this needs a little untangling - it looks like the MIDI device class uses m_nDeviceNumber (grabbed from a device number pool) to AddDevice using umidiN (https://github.com/rsta2/circle/blob/c243194670720c8a13e6a553654b6d26ec7845a0/lib/usb/usbmidi.cpp#L49)... but in MiniDexed that happens in the MIDI Keyboard class (which is a MIDIDevice too)... (https://github.com/probonopd/MiniDexed/blob/main/src/midikeyboard.cpp#L49) - so m_nDeviceNumber surely won't ever be set?

It probably doesn't matter too much I suppose if we don't use nDevice, but if the goal was reducing WTF/min when reading code, I'm not sure this is helping as we now seem to have two independent ways of assigning a device instance number and device string and no way to tie the two together.

As I say, I think we need to pick one system or the other for managing instance numbers... but I can't work through all that right now to think properly about where that should be...

I think nDevice was added to the parameters to give circle users more flexibility. So for example in a simpler case a static function could be enough to handle all midi devices.

Somewhat ironically, that sounds a fair bit like the code that has been taken out for this PR (although granted it was 4 static instances of the same code)...

Kevin

[soyersoyer]

In the end they will be the same. Minidexed on an RPI4 has 4 CMIDIKeyboard instances with device names umidi1, umidi2, umidi3, umidi4. For every hw change, Circle is asked if there is a CUSBMIDIDevice named with the device name. And CUSBMIDIDevice uses the same naming scheme. If an RPI has 5 USB MIDI devices the fifth one will not work.

This PR doesn't want to fix everything, just does the same thing with less code. Uses the new api, lightens code by ~50 lines, removes 1 function calls per MIDI message. (CUSBMIDIDevice::CallPacketHandler() -> CMIDIKeyboard::MIDIPacketHandler() -> CMIDIKeyboard::USBMIDIMessageHandler() instead of CUSBMIDIDevice::CallPacketHandler() -> CUSBMIDIDevice::ProxyHandler() - > CMIDIKeyboard::MIDIPacketHandlerN() -> CMIDIKeyboard::USBMIDIMessageHandler() )

[diyelectromusic]

Right, I've attempted to work through what the nDevice parameter is for - I see it should be the same as the instance number used when setting the device name. I do wonder then that we ought to store that instance number and check it against the device number to ensure all is sensible and assert() if not. This does assume that circle will allocate numbers sequentially of course, but as the comment for nDevice says: "Device number of the MIDI device (e.g. 1 for "umidi1")" I trust this is a sensible assumption (although I've not seen this stated anywhere...). And if it doesn't then we are unlikely to find our umidi1..4 devices anyway...

I wonder if we should still assert that m_nInstance < 4 somehow - or maybe we don't care anymore and could allow for more devices anyway... (it's not user-configurable, it is build-time configurable)?

I must admit I can't quite work out where the limit of four came from anyway - I guess that was just a MiniDexed limit? If we plugged in five devices, I think circle just automagically recognises umidi5 regardless...? It looks like no maximum is set on the device number pool (up to the circle limit of 63)...

I see that as soon as a USB MIDI device is plugged in, all of the existing CMIDIKeyboard instances will be doing their "plug and play" thing looking for their own device number if not already found... and this will keep happening until all four are found.

... so I guess it would be possible to plug in five USB MIDI devices and then remove umidi1 through 4, leaving just umidi5, but MiniDexed will ignore that one even though it is now the only USB MIDI device plugged in. I guess this is the same as the pre-change behaviour though anyway... so we probably don't care?

I short, from what I think I understand how it works, I think we probably ought to check nDevice against the instance number and assert() if different - if nothing else, as an early warning if the assumptions around device numbers change in the future. But apart from that, we're probably then ok?

But I'm not overly familiar with the USB plug-and-play system - so that is probably about as far as I can go with a view on this change.

Kevin

[soyersoyer]

I must admit I can't quite work out where the limit of four came from anyway

It's from the config.h

#if RASPPI <= 3
	static const unsigned MaxUSBMIDIDevices = 2;
#else
	static const unsigned MaxUSBMIDIDevices = 4;
#endif

I don't think it's important to check the nDevice, it's not practical to rename umidi2 to umidi1 or something like that.

[diyelectromusic]

I must admit I can't quite work out where the limit of four came from anyway

It's from the config.h

That is where it was configured for MiniDexed based on Pi version. It was limited in midikeyboard.h:

static const unsigned MaxInstances = 4;

I guess it must have been simply due to the number of static handlers hard-coded into MiniDexed, but I'm just guessing.

I don't think it's important to check the nDevice, it's not practical to rename umidi2 to umidi1 or something like that.

It's not important today, I'm thinking how we might protect ourselves against the assumptions about instance/device we're making changing in the future.

As I say, from what I've seen and can work out, that would be my recommendation if we are to change this code.

Kevin

[soyersoyer]

I don't think there will be such a change in the API on purpose, but it could be a bug like this by accident. I added it.

[github-actions]

Build for testing:
MiniDexed_2024-12-03-7fcaf71
Use at your own risk.

[diyelectromusic]

As per discussion, ok to accept this change (within the limits of my understanding of the USB plug-and-play system in circle).

Kevin

[probonopd]

Thanks @soyersoyer and @diyelectromusic