Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added a test for nrpn message #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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