adafruit / Adafruit_CircuitPython_MIDI

A CircuitPython helper for encoding/decoding MIDI packets over a MIDI or UART connection
MIT License
46 stars 25 forks source link

Why is `send` always overwriting `MIDIMessage.channel`? #56

Open konstantint opened 8 months ago

konstantint commented 8 months ago

Just curious, but why is send forcing a channel on a message (here) even if the message had its channel field set explicitly? Isn't the main point of the message having a "channel" field in being able to prepare a set of messages assigned to different channels and have the MIDI interpreter send them to those channels, exactly as given?

Sure, I could always make sure to write midi.send(message, message.channel), but this now seems to defeat the purpose of the send method's channel argument (and the MIDI object out_channel field) in being able to configure an out_channel as some sort of a default when necessary.

A more meaningful design, IMO, could be:

tannewt commented 8 months ago

It originates from this conversation I think: https://github.com/adafruit/Adafruit_CircuitPython_MIDI/pull/9#discussion_r271080602

I agree it is a bit weird to overwrite it.

@kevinjwalters may have thoughts too.

kevinjwalters commented 8 months ago

Not much to add as there's a lot covered in aforementioned conversation which is better than my memory here as this was quite a while back! I don't have an opinion on discussed change other than it may affect existing application code.

There might have been some influence here from the original implementation too.

todbot commented 8 months ago

I've been bitten by this too. Having so many useful places to set channel makes the logic a bit tricky. I would expect the precedence to be: (send(), msg, MIDI) as @konstantint suggests. I think swapping the channel assignment in the isinstance() conditional would set that precedence. E.g. from this:

        if isinstance(msg, MIDIMessage):
            msg.channel = channel

to this:

        if isinstance(msg, MIDIMessage):
            channel = msg.channel or channel  # guard against msg channel not set

But I've not tested this.

tannewt commented 8 months ago

I wouldn't use or because 0 may be considered false. But this logic with an if statement would be fine.