debounceFingerHoles() might set a newNote to -1 which becomes MIDI note 127

[MrMep]

at https://github.com/amowry/WARBL2/blob/1de83c0922f754817d6164c8935dc6ddcd8c9969/warbl2_firmware/Functions.ino#L749 tempNewNote gets checked if it's valid and different from current note, but the "if" closes here (https://github.com/amowry/WARBL2/blob/1de83c0922f754817d6164c8935dc6ddcd8c9969/warbl2_firmware/Functions.ino#L757

Shouldn't it close here:https://github.com/amowry/WARBL2/blob/1de83c0922f754817d6164c8935dc6ddcd8c9969/warbl2_firmware/Functions.ino#L769 ? Otherwise, if the note is -1, 127 is sent to MIDI, and if the note is the same, it is sent again.

gl

[joebowbeer]

I'm not seeing how -1 will send 127

There are 129 different values:

Valid note range is 0..127 (0x00 .. 0x7F) unsigned

-1 indicates no new note (0xFFFFFFFF)

The conditionals filter out the -1 case, right?

[MrMep]

-1 becomes 127 because of binary representation of numbers and conversion from a negative integer and the MIDI values.

• the number of values is actually 128.
• The condition is filtered, but lines starting from https://github.com/amowry/WARBL2/blob/1de83c0922f754817d6164c8935dc6ddcd8c9969/warbl2_firmware/Functions.ino#L765 are executed anyway, that's why the condition needs to be extended to those line too. This code works only if getNote() never returns -1, which is accidentally the case in the current source code because all positions of all fingerings are possibly associated with a note. But when getNote() returns -1, a high note is played (it happened to me and it's easily reproducible).

[joebowbeer]

I see what you mean.

The charts hold byte (unsigned char) values 0..255 and tempNewNote is signed 16-bit integer.

getNote returns a signed 16-bit int to tempNewNote

No problem so far.

HOWEVER, newNote is unsigned char (byte)

https://github.com/amowry/WARBL2/blob/1de83c0922f754817d6164c8935dc6ddcd8c9969/warbl2_firmware/warbl2_firmware.ino#L265

So the assignment from tempNewNote to newNote is broken, as you say, and needs to be protected.

Otherwise, the errant note will be sent in the following line:

https://github.com/amowry/WARBL2/blob/1de83c0922f754817d6164c8935dc6ddcd8c9969/warbl2_firmware/Functions.ino#L1451-L1453

[amowry]

Thanks for spotting this! Yes, I think that this line should be moved higher into the previous "if" statement:

newNote = tempNewNote;

I'll also get rid of all the -1 stuff (and have getNote return a byte rather than an int) unless you want it in there for your own purposes. It's just a holdover from early code but as you mentioned the lookup tables will now always return a value.

[MrMep]

You might want to preserve the ability of defining "muted" positions: in my experience with Warbl, having all the "impossibile" or unused fingerings simply ignored, reduces a lot the probability of unwanted glitches. In my custom fingering schemes, I set these unused position to a zero value (thus renouncing to generate the corresponding MIDI note 0), and then translate these to -1 in getNote. We could invent smarter ways to accomplish the same, but this way looks to me as the less invasive of the existing code. thanks, gl

EDIT: yes, if I remeber correctly, a very similar problem(the -1 from get_note()) is in the original Warbl firmware as well.

[amowry]

Currently, MIDI note "0" will produce silence because no note is sent in sendNote() if the note is zero. (I may have added that after I sent you your WARBL). I did that so that anyone can use a zero or a blank line in a custom chart to produce a blank note. Does that work for you, or would you rather be able to have a value of -1?

[joebowbeer]

The init of newNote to -1 (see my comment above) is also suspect, esp. since newNote is unsigned and 0 is the value singled-out to not send a note.

[amowry]

Yep, good point :). I would propose having all these variables be bytes (unsigned char) initialized to 0, and not use -1 anywhere.

[MrMep]

no note is sent in sendNote() if the note is zero.

Sorry, I hadn't noticed :) May I suggest to keep a check for "0" values (or "-1", or "0xff", see below) also well before sending a NoteOn? debounceFingers looks a good place: maybe it would avoid some unnecessary processing. That's why I suggested to move the "if" ending (and not just the newNote = tempNewNote assignement as you suggested): if getNote returns a "null" value, you simply don't trigger a new note at all. What do you think?

would you rather be able to have a value of -1?

If you ask me, I would rather have an equivalent of -1, in case something is wrong with the result of the function. As long as we are speaking of MIDI, I would suggest 0xff as "zero" or "error" result, so that we re-gain access to the MIDI note 0 and keep the ability to return a "null" value. In general, I would initialize all byte vars that keep MIDI values to 0xFF, although I understand this would require a value check before sending out any MIDI message.

gl

[amowry]

Okay, thanks--that makes sense to me to use 0xFF as an error result.

I just pushed some changes. The "byte" data type is now used for tempNewNote, newNote, and prevNote, and they are all initialized to 0xFF. I moved the "if" ending as you suggested, and I'm checking for a note value of 0xFF before changing the value of newNote. I didn't check for a zero value there, though, because sendNote() still needs to be called in order to turn off the previously playing note. I guess we don't need to call getShift() or getState() if the note is zero, but I'm not sure it matters too much if we do. Does that seem okay?

[joebowbeer]

I'm just an impartial observer, but I think having all the note values use a consistent byte type (unsigned char) and "wasting" note 0 will be easier than introducing a new None value such as -1 (or 0xFF).

This is closest to what is implemented now, and documented in the charts.

But 0xFF (unsigned) seems like a better choice, in hindsight...

[joebowbeer]

However, the "mute" test is still != 0 in Functions.ino

https://github.com/amowry/WARBL2/blob/main/warbl2_firmware/Functions.ino#L1471-L1473

[amowry]

Yes, my intention was that "0" is used as a "blank" (silent) note, whereas 0xFF is used as an error. If 0xFF is encountered then the note will remain unchanged from the previous one. If a "0" is encountered then the previous note is turned off and no new note is turned on. This allows users to enter "blank" notes in custom fingering charts.

[joebowbeer]

My concern was that, after this check, the shift is added to the newNote which would result in a wraparound value that may be difficult to filter out. But maybe newNote=0xFF is impossible here.

        if (newNote != 0) {                                                 // With a custom chart a MIDI note of 0 can be used as a silent position, so don't play the note.
            sendMIDI(NOTE_ON, mainMidiChannel, newNote + shift, velocity);  // Send the new note.
        }

[amowry]

I'm not 100% sure I understand your concern there but there was an issue in that I was flagging that a note was playing even if the note was 0 and so was never sent. Now I check towards the beginning of the function if the note is 0, and also down below where we see if we need to turn off a previous note.

[joebowbeer]

My concern is that if newNote=0xFF reaches the conditional, then it will fall through and emit the note shift-1 (from unsigned newNote+shift)

[amowry]

Oh, I understand now, I don't think newNote could ever be 0xFF there because we don't transfer that value from tempNewNote. Maybe newNote shouldn't be initialized as 0xFF, though? I could initialize it as 0 as a precaution.

[MrMep]

If a "0" is encountered then the previous note is turned off and no new note is turned on.

Ok, now I realize we mean two different things! My idea was that if you encounter "0" (or "0xFF") you don't turn off the previous note, but simply ignore the change in finger position and keep everything the same. I understand we come from different places: you may be thinking of a pipe-like style of playing, in this case I can see the usefulness of a "stop" position. On the other side, I'm going after glitches and how to avoid them.

The solution could be simple:

• "0" means "stop" the current note and stay quiet
• "0xFF" means: ignore this position and carry on with current state.

Am I getting this right?

gl

[amowry]

Oh, I understand now-- sorry, I'm a little slow :). I added 0 as a "silent" position because others had asked for it, but I see the desire for a "blank" position as well. I had thought that you were just asking for an error code to indicate if getNote() had failed to return anything.

For custom fingering charts it's a bit easier for me to use only 7-bit values because I'm sending charts to the WARBL using MIDI CC. So, how about 0 for a silent position and 127 for a blank position?

[MrMep]

how about 0 for a silent position and 127 for a blank position?

Sorry for the delay, that sounds great! Thank you gl

[amowry]

Thanks! I have one more question about the "blank" position. If we are playing a valid note and then stop blowing, then switch to a "blank" position before blowing again, what should happen? Should it play the previous valid note, or play no note at all? I can't decide which is more logical :).

[essej]

Good question, if the finger positions haven't changed between the blowing then it should probably play the same as it was before... I think the main point of the blank positions is that they are a shortcut to represent fingerings that "don't matter" and are really the same note as another "real" one, which presumably was active just prior. It is kind of weird, because really all combinations should have a particular meaning, it's just a pain to fully spec out a custom fingering for all combinations? I think a detailed example from @MrMep would be great!

[amowry]

That's the current behavior (and easiest to implement) so I'll leave it at that unless I hear otherwise. I just pushed that change.

[MrMep]

what should happen?

A good question indeed! I think that if a position is silent (or "blank" as you call them), it should be always silent. In my view:

• if the user is playing (blowing) and hits a silent position, that is ignored completely (so far so good, I think it's what you've already done);
• If the user stops blowing and restarts with a silent position, no sound should be emitted;
• As well, if the user is playing, hits a silent position and then stops blowing, when he/she restarts blowing still on a silent position, no sound should be emitted.

Here's my rationale: on acoustic wind instruments there are a lot of "false" positions that don't "engage" easily, i.e. they require over-or-underblowing and/or special attacks, and usually are some kind of out-of-tune harmonics, double notes, etc.. When playing normally, if you quickly hit one of those positions, chances are that the current note or the following note will be heard, not the glitch. Maybe if you keep the "wrong" position long enough, eventually the sound will break.

In my custom fingering, one of those position is, for example: X 011 1101 (thumb hole doesn't matter) As a position per se it wouldn't make any sense to assign a note to it, but I keep hitting it as a passage note, which is really boring.

Of course, it would be great if we could assign a dynamic threshold (pressure) to these positions and maybe a detuning value, and maybe a second concurrent note (I'm dreaming, but who knows :)

For the time being, I would say the way you did it @amowry works well, thank you.

[amowry]

Just to clarify, I'm using the terms "silent" and "blank" to mean two different things now-- the "silent" note (0) will always produce silence, while the "blank" note (127) won't have any effect, i.e. the previous note will continue playing. I'm realizing this terminology will be confusing because if someone leaves a blank value in the custom chart that they paste into the Config Tool, it will be converted to 0 :).

I'm happy to change the "blank" note so it produces silence if blowing is stopped and then restarted, if you'd like. I don't anticipate too many people using it so I'm fine with going with your preference.

[MrMep]

Thanks Andrew, I guess we can close this as it is now ;) Is this documented anywhere ?

[amowry]

Thanks! It's not documented yet but I'll add that when I release the firmware.