midifixes #214
midifixes #214
Conversation
…atus causes an infinite loop
…di active sense message filtering turned off, midiclkin now working
danomatika
added
the
bug/fix
There was a problem hiding this comment.
since i heartily despise MIDI i don't have any real comment. luckily coverity-scan discovered a few implementation details for me (see the note on dead code).
the implementation in s_midi.c seems to not have this problem, by doing something like status = (status>=MIDI_SYSEX)?status:(status&0xf0)
src/s_midi_alsa.c
Outdated
| snd_seq_ev_set_noteon(&ev,channel,b,c); | ||
| } | ||
| else if (a >= 128) // note off | ||
| status = a & 0xf0; |
There was a problem hiding this comment.
so status <= 0xf0 (240)...
There was a problem hiding this comment.
The status value is the upper nibble and the channel is the lower nibble for those messages which have them. This masks out the channel.
There was a problem hiding this comment.
Ah, I see what you're saying. That should be:
status = a & 0xFF;
if (status < MIDI_SYSEX) status = status & 0xF0;
| /* b and c are already correct but alsa needs to recalculate them */ | ||
| snd_seq_ev_set_pitchbend(&ev, channel, (((c<<7)|b)-8192)); | ||
| break; | ||
| case MIDI_TIMECODE: |
There was a problem hiding this comment.
and MIDI_TIMECODE:=0xf1 (241); so how on earth is status == MIDI_TIMECODE ever going to be true?
There was a problem hiding this comment.
the same holds for SONGPOS and SONGSELECT
There was a problem hiding this comment.
Good catch. Fixed.
|
@umlaeute As you detest MIDI, I'm guessing you're not interested in running some tests? |
|
I added the midi test patch I've ben using to |
|
On 08/30/2017 05:37 PM, Dan Wilcox wrote:
@umlaeute As you detest MIDI, I'm guessing you're not interested in running some tests?
i honestly wouldn't know how.
(that is: i don't have any MIDI gear ready at hand to perform some real
world tests)
|
|
@umlaeute Neither do I right now. That's why I wrote a test utility: https://github.com/danomatika/miditester It you run it with the |
src/x_midi.c
Outdated
| @@ -586,11 +586,21 @@ static void midiout_float(t_midiout *x, t_floatarg f) | |||
| outmidi_byte(x->x_portno - 1, f); | |||
| } | |||
|
|
|||
| static void midiout_list(t_midiout *x, t_symbol *s, int ac, t_atom *av) | |||
| { | |||
| for (int i = 0; i < ac; ++i) | |||
There was a problem hiding this comment.
this is only valid in C99; since adding support for ANSI C is pretty straightforward, i would suggest to use:
int i;
for (i = 0; i < ac; ++i)
{
// ..
}There was a problem hiding this comment.
Got it. Is C99 still a hard limit? I suppose so if we want to continue supporting Windows XP builds? :P
This PR is an offshoot of #211 which only contains the MIDI bug fixes and code cleanups:
The work so far has been tested with a simple utility program I wrote (miditester) and Logic Pro X on macOS. This will need testing on other platforms.