dsacre / mididings

A MIDI router/processor based on Python
http://das.nasophon.de/mididings/
GNU General Public License v2.0
120 stars 40 forks source link

Accept sysex strings in ascii hexadecimal notation with no white-space separation #7

Closed SpotlightKid closed 1 year ago

SpotlightKid commented 10 years ago

... and comma/white space-separated single-digits bytes in sysex_to_bytearray.

Raise ValueError when both commas and white-space is used.

I.e. this is allowed:

sysex_data('F0040815162342F7')
sysex_data('f0040815162342f7')
sysex_data('f0 4 8 15162342f7')
sysex_data('f0,4,8,15,16,23,42,f7')

But not this:

sysex_data('f0,4,8 15162342f7')

The check in added line 288, searching for each white-space character, may be a bit excessive, esp. for long sysex strings, but the expression is ordered in such a way that the performance cost is only incurred when the sysex string contains a comma, which should be less common and even then it's still negligible, I think.

Instead, we could also only allow commas or spaces as separators:

    if sysex.startswith('F0') or sysex.startswith('f0'):
        if ',' in sysex and ' ' in sysex:
            raise ValueError("mixed comma and space chars in sysex string")
        result = bytearray()
        for group in sysex.split(',' if ',' in sysex else ' '):
            for hex in (group[i:i+2] for i in range(0, len(group), 2)):
                result.append(int(hex, 16))
        return result

(More than one consecutive space is handled by range(0, len(group), 2) yielding an empty sequence for empty string groups.)

But then other white-space characters may throw off the pair-wise conversion of hex chars to ints and produce hard to understand errors, e.g. this input:

sysex_data('f0,4,8\t\t15162342f7')
dsacre commented 10 years ago

As far as I can tell, the present implementation already supports single-digit bytes. It also allows arbitrary additional whitespace after separators; in particular, this is (and should be) perfectly valid:

SysEx('f0, 4, 8, 15, 16, 23, 42, f7')

That said, I'm all for allowing sysexes with no whitespace or separators. I'm not quite sure about 'f0 4 8 15162342f7' though. It's not really ambiguous, but the combination of single-digit bytes and sections with no separators just looks like an error to me. Is there any particular use-case for this?

SpotlightKid commented 10 years ago

Well, I just intended to follow the guideline "be lenient in what you accept and strict in what you output". Musicians often are no programming experts and usually just copy&paste sysex strings they found somewhere. I remember somebody on a musician's forum not long ago reporting problems with entering a sysex string into Setlist Manager (an iOS app). Turned out the program couldn't handle sysex strings without whitespace between the hex numbers and that didn't occur to that person as a source of the problem.

As to the mix of whitespace/comma separated and non-separated numbers, support for this resulted from the same wish to avoid possible user error, but I'm not too sure whether it is a good idea myself. If the numbers are separated, single digit numbers should remain supported though, IMHO.

SpotlightKid commented 9 years ago

That said, I'm all for allowing sysexes with no whitespace or separators

Ok, I've simplified sysex_to_bytearray: sysex strings must be comma or whitespace--separated or contain only hexchars.

SpotlightKid commented 1 year ago

Closing this since I'm cleaning my unmerged PRs.