Skip to content

Comments

added a test for nrpn message#74

Open
electricmonk wants to merge 1 commit intoInvisibleWrench:masterfrom
electricmonk:master
Open

added a test for nrpn message#74
electricmonk wants to merge 1 commit intoInvisibleWrench:masterfrom
electricmonk:master

Conversation

@electricmonk
Copy link

No description provided.

@electricmonk electricmonk mentioned this pull request Jan 27, 2023
@mortenboye
Copy link
Contributor

I think there are a few things to consider here.
First thing is that NRPN can be sent either as Running Status CC's or individual CC's.
In the case of running status the total length of a full NRPN message (including value LSB) is 9 bytes, whereas it is 12 bytes if sent as individual CC's. This is mostly related to your unit test that only tests for the long form.
Saving 25% of the transmitted data is not insignificant when you are sending data over a 31250 baud connection.
Same goes for always sending the optional LSB value byte if it is not necessary.

@electricmonk
Copy link
Author

This is my first time dealing with MIDI in software so there's a lot of things I'm not familiar with yet. However, if there are MIDI implementations that can't deal with the shorter form, maybe there's no choice?

For my specific use case I guess I can create a FullNrpnMessage class in my project that deals with my synth properly.

Also, I think you got the LSB controller wrong, you're using 0x38 where it should be decimal 38/

@mortenboye
Copy link
Contributor

You are right about the value LSB being off. Thanks.

I just did some testing with some other equipment (I dont have a Rev2) and monitored some midi logs, and it leads me to believe that I've "overinterpreted" the NRPN spec.
I think that instead of a single NRPN message that decides to include the valueLSB based on the value instead it should be decided with the message type. Some parameters/controls seems to always send/receive 14 bit values (MSB+LSB) and some only 7 bit.
I will make an update that includes two types of NRPN messages.

expect(m.data[1], equals(0x63));
expect(m.data[2], equals(0x20));

expect(m.data[3], equals(0xb0));
Copy link
Contributor

Choose a reason for hiding this comment

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

With running status, these status bytes are omitted and should to be tested for.

Copy link
Contributor

@mortenboye mortenboye left a comment

Choose a reason for hiding this comment

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

Feel free to update to match latest master

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.

2 participants