FortySevenEffects / arduino_midi_library

MIDI for Arduino
MIT License
1.6k stars 258 forks source link

Korg SYSEX 8 bytes encoding : inverted msb bit storing logic #92

Closed TheKikGen closed 6 years ago

TheKikGen commented 6 years ago

I had some issues with the decode and encode sysex functions on a Korg Electribe (and other Korg devices). In the encode/decodesysEx funcs, MSB bits are encoded on the 8th byte from bit 6 to bit 0 corresponding respectively to bytes 1 to 7 in the set. I got somes stranges values when decoding dumps from an Korg Electribe ES1, and saw that the logic of encoding bits was inverted, i.e. from bit 0 to bit 6.

For example, the following message received from the an ES1 (02) 07 7F 00 00 00 00 00 is corresponding to the decoded msg 07 FF 00 00 00 00 00 but encoded with encodeSysEx, I get (20) 07 7F 00 00 00 00 00

That is clear in the code of encodeSysEx when moving bits in outSysEx[0].

   outSysEx[0] |= (msb << (6 - count));

This match with the way to encode sample and file dumps in the midi standard but however I suggest to add an optional parameter to cover that case from such manufacturer, and making these sysex utility funcs a "bit" more universal.

/////////////////////////////////////////////////
// ENCODE 8BITS TO 7BITS SYSEX
/////////////////////////////////////////////////
unsigned encodeSysEx(const byte* inData, byte* outSysEx, unsigned inLength,bool fromHighbit)
{
    unsigned outLength  = 0;     // Num bytes in output array.
    byte count          = 0;     // Num 7bytes in a block.
    outSysEx[0]         = 0;

    for (unsigned i = 0; i < inLength; ++i)
    {
        const byte data = inData[i];
        const byte msb  = data >> 7;
        const byte body = data & 0x7f;

        outSysEx[0] |= (msb << (fromHighbit ? 6-count : count ));
        outSysEx[1 + count] = body;

        if (count++ == 6)
        {
            outSysEx   += 8;
            outLength  += 8;
            outSysEx[0] = 0;
            count       = 0;
        }
    }
    return outLength + count + (count != 0 ? 1 : 0);
}
/////////////////////////////////////////////////
// DECODE 7BITS SYSEX TO 8BITS
/////////////////////////////////////////////////
unsigned decodeSysEx(const byte* inSysEx, byte* outData, unsigned inLength,bool fromHighbit)
{
    unsigned count  = 0;
    byte msbStorage = 0;
    byte byteIndex  = 0;

    for (unsigned i = 0; i < inLength; ++i)
    {
        if ((i % 8) == 0)
        {
            msbStorage = inSysEx[i];
            byteIndex  = 6;
        }
        else
        {
            const byte body = inSysEx[i];
            const byte msb  = ((msbStorage >> (fromHighbit ? byteIndex : 6 - byteIndex) ) & 1) << 7;
            byteIndex--;
            outData[count++] = msb | body;
        }
    }
    return count;
}
franky47 commented 6 years ago

Hi, I have pushed a fix for this in the #95 feat/4.4.0 branch, with a little edit from your proposal.

As I believe the natural order of the MSBits should be the same as the associated following bytes (it makes it easier to read and follow through), it seems Korg is reversing the order (for some implementation or optimisation reason), so the extra boolean parameter is named inFlipHeaderBits, and is false by default to keep current behaviour consistent.

Extended documentation available here: https://github.com/FortySevenEffects/arduino_midi_library/blob/feat/4.4.0/doc/sysex-codec.md

TheKikGen commented 6 years ago

Bonjour François.

Je switch au french comme je le suis et toi aussi non ? Tu as parfaitement raison : la logique voudrait que les bits suivent l'ordre des "bytes".... Content d'avoir contribué à ton super boulot sur cette librairie.

Franck

2018-03-10 17:03 GMT+01:00 Francois Best notifications@github.com:

Hi, I have pushed a fix for this in the #95 https://github.com/FortySevenEffects/arduino_midi_library/pull/95 feature/4.4.0 branch, with a little edit from your proposal.

As I believe the natural order of the MSBits should be the same as the associated following bytes (it makes it easier to read and follow through), it seems Korg is reversing the order (for some implementation or optimisation reason), so the extra boolean parameter is named inFlipHeaderBits, and is false by default to keep current behaviour consistent.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/FortySevenEffects/arduino_midi_library/issues/92#issuecomment-372040796, or mute the thread https://github.com/notifications/unsubscribe-auth/AiDKq6ltsqKAlXItzcsB2WLaUT0FhJrVks5tc_lTgaJpZM4SNukN .

franky47 commented 6 years ago

Merci à toi ! Tu me diras si ça marche, j'ai ajouté un test pour vérifier la théorie, mais je n'ai pas de machine Korg sous la main pour tester in situ.