Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Requiring EOX for creating a SysEx message is not spec compliant #363

Open
rdoursenaud opened this issue Feb 20, 2022 · 3 comments · May be fixed by #370
Open

Requiring EOX for creating a SysEx message is not spec compliant #363

rdoursenaud opened this issue Feb 20, 2022 · 3 comments · May be fixed by #370
Assignees
Labels
enhancement feature:protocol MIDI Protocol implementation
Milestone

Comments

@rdoursenaud
Copy link
Member

rdoursenaud commented Feb 20, 2022

Specification

The MIDI specification states, page 5 (emphasis added by me):
"Exclusive messages [...] can be terminated either by an End of Exclusive (EOX) or any other Status byte (except Real Time messages)."

Rationale

Currently, the parser does not interpret these SysEx messages and drops them!
It is also impossible to create a SysEx message without EOX from hex or string.

IMHO Since EOX (0xF7) is just another Status byte, it should not be part of the SysEx message.

My understanding of "An EOX should always be sent at the end of a System Exclusive message" in the specification is that the sender is responsible for adding the EOX after a SysEx message, not that it is part of it!

I’d be happy to contribute a patch if you approve of these potentially breaking changes.

Examples

mido.parse_all(bytes.fromhex("f000801212"))

Current result: [Message('note_off', channel=0, note=18, velocity=18, time=0)]
Expected result: [Message('sysex', data=(0,), time=0), Message('note_off', channel=0, note=18, velocity=18, time=0)]

mido.Message.fromhex("f000")

Current result: ValueError: invalid sysex end byte 0
Expected result: Message('sysex', data=(0,), time=0)

@rdoursenaud
Copy link
Member Author

To reinforce my point, EOX is defined as a separate System common message page 27 of the specification. It definitely is its own message and should not be bundled with SysEx messages!

@jerekapyaho
Copy link

While F7h is its own message, it is solely defined as "End of System Exclusive", with no data bytes, but it's really messy in practice. Every SysEx dump from a synthesiser I've seen contains the F7h byte at the end. The book "MIDI For the Professional" by Lerhman and Tully (1993) states (on page 25): "At the end of any System Exclusive message is an F7H byte", and (on page 27): "Like all SysEx messages, Universal System Exclusive messages must feature an EOX byte (F7H) at the end."

In the article "Everything You Ever Wanted To Know About System Exclusive (Part 2)" from 1989, Martin Russ states: "[in fact, t]he EOX ($F7) status byte can be replaced with any other status byte except for a Real-Time message, although I have never seen this used."

My approach in my own code has been that for simplicity, and because these other messages practically never occur in System Exclusive dumps from synthesizers, it is practical to deal with System Exclusive messages as being strictly delimited by 0xF0 and 0xF7. There are also SysEx dump files in the wild that have multiple SysEx messages inside, delimited with F7h.

I wouldn't mind either way, as long as it's clearly documented.

@rdoursenaud
Copy link
Member Author

Thanks for the great insights @jerekapyaho!

The reason I need the EOX to be a separate message is to be fully spec compliant.
There’s a reason it’s written this way. I believe the rationale is to give musical information priority over SysEx messages.

An hypothetical use case is when using merging. AFAIK Most mergers will not issue an EOX but will prioritize other Status Bytes.
Hardware receivers will then drop processing the SysEx and register the note for example.
I need my Software to behave in the same way which is currently impossible using MIDO.

An option would be for me to write my own library but I’d rather contribute to an existing popular project and hopefully make it better rather than rolling my own.

If you have any suggestions on how to implement this while not breaking the current API for other users,
I was thinking about either using a runtime configuration option or try to make a unified API.

rdoursenaud added a commit to EMATech/mido that referenced this issue Apr 11, 2022
Fixes mido#363

Also implements:
 - receiving Running Status (Does *not* implement sending with Running Status yet)
   Fixes mido#340
 - getting delta time from rtmidi backend (In a bad way for the moment)
   Fixes mido#241

 NOT PRODUCTION READY, CONTAINS A BUNCH OF UNRESOLVED FIXMEs AND TODOs.
 ONLY PUBLISHED FOR REVIEW PURPOSES!
@rdoursenaud rdoursenaud linked a pull request Apr 13, 2022 that will close this issue
13 tasks
@rdoursenaud rdoursenaud added enhancement feature:protocol MIDI Protocol implementation labels Jan 19, 2023
@rdoursenaud rdoursenaud self-assigned this Jan 19, 2023
@rdoursenaud rdoursenaud added this to the Version 2.0.0 milestone Jan 19, 2023
rdoursenaud added a commit to EMATech/mido that referenced this issue Jan 22, 2023
Fixes mido#363

Also implements:
 - receiving Running Status (Does *not* implement sending with Running Status yet)
   Fixes mido#340
 - getting delta time from rtmidi backend (In a bad way for the moment)
   Fixes mido#241

 NOT PRODUCTION READY, CONTAINS A BUNCH OF UNRESOLVED FIXMEs AND TODOs.
 ONLY PUBLISHED FOR REVIEW PURPOSES!
rdoursenaud added a commit to EMATech/mido that referenced this issue Jan 22, 2023
Fixes mido#363

Also implements:
 - receiving Running Status (Does *not* implement sending with Running Status yet)
   Fixes mido#340
 - getting delta time from rtmidi backend (In a bad way for the moment)
   Fixes mido#241

 NOT PRODUCTION READY, CONTAINS A BUNCH OF UNRESOLVED FIXMEs AND TODOs.
 ONLY PUBLISHED FOR REVIEW PURPOSES!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature:protocol MIDI Protocol implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants