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

native_midi_linux_alsa: Add support for System Exclusive messages #679

Merged
merged 1 commit into from
Mar 23, 2025

Conversation

tatokis
Copy link
Contributor

@tatokis tatokis commented Mar 17, 2025

This PR implements support for sending SysEx messages to MIDI devices using the native MIDI ALSA implementation.

Tested by playing back various SMF files containing SysEx messages and comparing them to aplaymidi using aseqdump.

$ ALSA_OUTPUT_PORTS=aseqdump ./playmus file.mid
$ ALSA_OUTPUT_PORTS=aseqdump aplaymidi file.mid

@sezero
Copy link
Contributor

sezero commented Mar 17, 2025

CC @fragglet

@tatokis
Copy link
Contributor Author

tatokis commented Mar 17, 2025

@sezero I just saw 62bb7bb and I'm curious; which version of gcc had trouble with it? I'd expect even super old gcc to support gnu99/partial c99.

I'm asking to avoid having this PR also break something by accident.

@sezero
Copy link
Contributor

sezero commented Mar 17, 2025

@sezero I just saw 62bb7bb and I'm curious; which version of gcc had trouble with it? I'd expect even super old gcc to support gnu99/partial c99.

I'm asking to avoid having this PR also break something by accident.

It was my old gcc4.4 on my very outdated centos 6.10. It still supports the feature, but when in c99 mode which, who knows why, isn't requested by the cmake'ry yet.

if (event->status == MIDI_CMD_COMMON_SYSEX && event->extraLen) {
/* Sanity check in case something changes in the future */
/* This is safe to do since we can't have an F0 manufacturer ID */
assert(event->extraData[0] != 0xF0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use SDL_assert or one of its variants here? (CC @slouken )

Copy link
Contributor Author

@tatokis tatokis Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I used assert explicitly so that it gets compiled out in non debug builds.

IIRC SDL_assert makes it into more than just debug, and a normal user/application developer is never going to hit it. It's only for if there's a change in SDL_mixer itself.

@tatokis
Copy link
Contributor Author

tatokis commented Mar 17, 2025

@sezero I just saw 62bb7bb and I'm curious; which version of gcc had trouble with it? I'd expect even super old gcc to support gnu99/partial c99.
I'm asking to avoid having this PR also break something by accident.

It was my old gcc4.4 on my very outdated centos 6.10. It still supports the feature, but when in c99 mode which, who knows why, isn't requested by the cmake'ry yet.

Ah, understood. Might be worth looking into set_target_properties(target PROPERTIES C_STANDARD 99), which should set gnu99 by default, at least on Linux with gcc/clang. (I do not remember what happens on other platforms.)

@sezero sezero requested review from slouken and icculus March 17, 2025 21:32
@sezero
Copy link
Contributor

sezero commented Mar 21, 2025

@fragglet : Is this OK with you too?

@tatokis tatokis force-pushed the native-midi-linux-sysex branch from ff3f702 to e12630e Compare March 23, 2025 16:47
@tatokis
Copy link
Contributor Author

tatokis commented Mar 23, 2025

Rebased and applied the following for consistency.

diff --git a/src/codecs/native_midi/native_midi_linux_alsa.c b/src/codecs/native_midi/native_midi_linux_alsa.c
index 795a469d..21983817 100644
--- a/src/codecs/native_midi/native_midi_linux_alsa.c
+++ b/src/codecs/native_midi/native_midi_linux_alsa.c
@@ -473,7 +473,7 @@ NativeMidiSong *native_midi_loadsong_IO(SDL_IOStream *src, bool closeio)
         if (event->status == MIDI_CMD_COMMON_SYSEX && event->extraLen) {
             /* Sanity check in case something changes in the future */
             /* This is safe to do since we can't have an F0 manufacturer ID */
-            assert(event->extraData[0] != 0xF0);
+            assert(event->extraData[0] != MIDI_CMD_COMMON_SYSEX);
 
             /* Resize by + 1 */
             Uint8 *newData = SDL_realloc(event->extraData, event->extraLen + 1);
@@ -489,7 +489,7 @@ NativeMidiSong *native_midi_loadsong_IO(SDL_IOStream *src, bool closeio)
             /* Prepend the F0 */
             event->extraData = newData;
             SDL_memmove(event->extraData + 1, event->extraData, event->extraLen);
-            event->extraData[0] = 0xF0;
+            event->extraData[0] = MIDI_CMD_COMMON_SYSEX;
             event->extraLen++;
         }

@sezero sezero merged commit cadfafb into libsdl-org:main Mar 23, 2025
5 checks passed
@sezero
Copy link
Contributor

sezero commented Mar 23, 2025

This is in. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants