djipco / webmidi

Tame the Web MIDI API. Send and receive MIDI messages with ease. Control instruments with user-friendly functions (playNote, sendPitchBend, etc.). React to MIDI input with simple event listeners (noteon, pitchbend, controlchange, etc.).
Apache License 2.0
1.52k stars 115 forks source link

Incorrect CC name for registered and non-registered parameters #325

Closed djipco closed 1 year ago

djipco commented 1 year ago

Discussed in https://github.com/djipco/webmidi/discussions/324

Originally posted by **dannybullo** January 4, 2023 Hi, I realized you have an incorrect CC name: **registeredparametercoarse: 100 registeredparameterfine: 101** This is _reversed_ as the correct MIDI CC Names are: **100 Registered Parameter (LSB) 101 Registered Parameter (MSB)** Suggestion: Can you change the Array MIDI_CONTROL_CHANGE_MESSAGES to have more friendly names so that getCcNameByNumber() returns more "readable" values? for example: use **"Registered Parameter (LSB)"** instead of **"registeredparameterfine"** use **"Sostenuto Pedal "** instead of **"sustenutopedal"** use **"Bank Select (MSB)"** instead of **"bankselectcoarse"** Maybe there is an official standardized cc name list. See this. https://professionalcomposers.com/midi-cc-list/ Thanks! Danny Bullo
djipco commented 1 year ago

You are right and it's the same problem for CC98 and C99 (NRPNs). I guess the proper way to fix this is to change the name but also add aliases to preserver compatibility. I'll have to think about the best way to approach this...

Regarding the nomenclature, you have to realize that those names are event names. By convention, JavaScript event names are lowercase and do not use separators. You can read more about event naming in this discussion. Also, the official list is the one from the MIDI association.

Having said that, I could easily add an extra property on the dispatched event that contains a friendlier name meant to be displayed to end users.

dannybullo commented 1 year ago

``Thanks djipco.

I'm not sure I fully understand.....Sure, the event naming convention is all lower case, such as keyup, mousedown, etc... Where is the event involved here?

in utilities.js you have this

static getCcNameByNumber(number) { return Utilities.getPropertyByValue(Enumerations.MIDI_CONTROL_CHANGE_MESSAGES, number); }

As the name suggests, it resolved the CC # to NAME. Sure it is Using the MIDI_CONTROL_CHANGE_MESSAGES array, but I do not see the relation to event names. Then I see the array used in _parseEventForParameterNumber().... Again, not related to events there, just comparing the incoming controller to specific ones...

` const list = Enumerations.MIDI_CONTROL_CHANGE_MESSAGES;

// A. Check if the message is the start of an RPN (101) or NRPN (99) parameter declaration.
if (
  controller === list.nonregisteredparameterfine ||         // 99
  controller === list.registeredparameterfine               // 101
).......`

Anyway, thanks for your attention. Maybe it'd be possible to create another array with friendly names if you thing this may be a breaking change.

Regards,

Danny Bullo

djipco commented 1 year ago

Where is the event involved here?

You are right to point out that the names found in MIDI_CONTROL_CHANGE_MESSAGES are not currently being dispatched as events. However, if you look at the roadmap, it is already planned for them to be. This will allow you to listen to one specific control change message as such:

WebMidi.inputs[0].channels[1].addListener("controlchange-volumecoarse", someFunction);

Currently, you can do something similar with the controller number instead of the name:

WebMidi.inputs[0].channels[1].addListener("controlchange-controller7", someFunction);
djipco commented 1 year ago

Maybe it'd be possible to create another array with friendly names if you thing this may be a breaking change.

This is what I intend to do.

dannybullo commented 1 year ago

I created the following mapping. It is a mix between the official list and a lib that I found. The official one had inconsistencies - "on" vs "On", "inc"/"increment" or names such as "LSB for Control 0 (Bank Select)" instead of "Bank Select LSB" which in my opinion is not that "friendly" for a user select box. I was harder than it seems.... Maybe we can make it more UI-friendly and rename LONG NAMES???

For instance: Non-Registered Parameter Number LSB => NRPN LSB Registered Parameter Number LSB => RPN LSB

Anyway, hope this helps. I can create a PR if you wish. We just need a new method:

static getCcFriendlyNameByNumber(number) {...}

That needs to read from a new array as opposed to do it from "MIDI_CONTROL_CHANGE_MESSAGES"

Regards, Danny Bullo

0: 'Bank Select MSB', 1: 'Modulation Wheel MSB', 2: 'Breath Controller MSB', 3: 'Undefined MSB', 4: 'Foot Controller MSB', 5: 'Portamento Time MSB', 6: 'Data Entry MSB', 7: 'Channel Volume MSB', 8: 'Balance MSB', 9: 'Undefined MSB', 10: 'Pan MSB', 11: 'Expression Controller MSB', 12: 'Effect Control 1 MSB', 13: 'Effect Control 2 MSB', 14: 'Undefined MSB', 15: 'Undefined MSB', 16: 'General Purpose Controller 1 MSB', 17: 'General Purpose Controller 2 MSB', 18: 'General Purpose Controller 3 MSB', 19: 'General Purpose Controller 4 MSB', 20: 'Undefined MSB', 21: 'Undefined MSB', 22: 'Undefined MSB', 23: 'Undefined MSB', 24: 'Undefined MSB', 25: 'Undefined MSB', 26: 'Undefined MSB', 27: 'Undefined MSB', 28: 'Undefined MSB', 29: 'Undefined MSB', 30: 'Undefined MSB', 31: 'Undefined MSB', 32: 'Bank Select LSB', 33: 'Modulation Wheel LSB', 34: 'Breath Controller LSB', 35: 'LSB for Control 3', 36: 'Foot Controller LSB', 37: 'Portamento Time LSB', 38: 'Data Entry LSB', 39: 'Channel Volume LSB', 40: 'Balance LSB', 41: 'LSB for Control 9', 42: 'Pan LSB', 43: 'Expression Controller LSB', 44: 'Effect control 1 LSB', 45: 'Effect control 2 LSB', 46: 'LSB for Control 14', 47: 'LSB for Control 15', 48: 'General Purpose Controller 1 LSB', 49: 'General Purpose Controller 2 LSB', 50: 'General Purpose Controller 3 LSB', 51: 'General Purpose Controller 4 LSB', 52: 'LSB for Control 20', 53: 'LSB for Control 21', 54: 'LSB for Control 22', 55: 'LSB for Control 23', 56: 'LSB for Control 24', 57: 'LSB for Control 25', 58: 'LSB for Control 26', 59: 'LSB for Control 27', 60: 'LSB for Control 28', 61: 'LSB for Control 29', 62: 'LSB for Control 30', 63: 'LSB for Control 31', 64: 'Damper Pedal', 65: 'Portamento', 66: 'Sostenuto', 67: 'Soft Pedal', 68: 'Legato', 69: 'Hold 2', 70: 'Sound Variation', 71: 'Filter Resonance', 72: 'Release Time', 73: 'Attack Time', 74: 'Brightness', 75: 'Decay Time', 76: 'Vibrato Rate', 77: 'Vibrato Depth', 78: 'Vibrato Delay', 79: 'Sound Controller 10', 80: 'General Purpose Controller 5', 81: 'General Purpose Controller 6', 82: 'General Purpose Controller 7', 83: 'General Purpose Controller 8', 84: 'Portamento Control', 85: 'Undefined', 86: 'Undefined', 87: 'Undefined', 88: 'High Resolution Velocity Prefix', 89: 'Undefined', 90: 'Undefined', 91: 'Effects 1 Depth', 92: 'Effects 2 Depth', 93: 'Effects 3 Depth', 94: 'Effects 4 Depth', 95: 'Effects 5 Depth', 96: 'Data Increment', 97: 'Data Decrement', 98: 'Non-Registered Parameter Number LSB', 99: 'Non-Registered Parameter Number MSB', 100: 'Registered Parameter Number LSB', 101: 'Registered Parameter Number MSB', 102: 'Undefined', 103: 'Undefined', 104: 'Undefined', 105: 'Undefined', 106: 'Undefined', 107: 'Undefined', 108: 'Undefined', 109: 'Undefined', 110: 'Undefined', 111: 'Undefined', 112: 'Undefined', 113: 'Undefined', 114: 'Undefined', 115: 'Undefined', 116: 'Undefined', 117: 'Undefined', 118: 'Undefined', 119: 'Undefined', 120: 'All Sound Off', 121: 'Reset All Controllers', 122: 'Local Control On/Off', 123: 'All Notes Off', 124: 'Omni Mode Off', 125: 'Omni Mode On', 126: 'Mono Mode On', 127: 'Poly Mode On'

djipco commented 1 year ago

That's awesome! Thank you so much. Before submitting a PR, please give me some time to figure out the best way to approach this. In any case, your list will definitely save me some typing. Cheers!

djipco commented 1 year ago

Work has started on this. It turns out to be a little tricky to insure backwards-compatibility. The new changes will come with v3.1.

djipco commented 1 year ago

This issue has been fixed in release 3.1.0. Pleases read the release notes for more info.

Note that you can now access a user-friendly representation of the controller's function in the controller.description property of the event payload (starting with v3.1.2):

WebMidi.channels[1].addListener("controlchange-volumecoarse", e => {
  console.log(e.controller.description); // Channel Volume (Coarse)
  console.log(e.controller.position); // msb or lsb
}); 

There is also a new Enumerations.CONTROL_CHANGE_MESSAGES that provides more info than before. The Enumerations.MIDI_CONTROL_CHANGE_MESSAGES enum will remain for backwards compatibility.

The updated documentation will be available on webmidijs.org soon.

dannybullo commented 1 year ago

Nice Work djipco! ;)