Strange issue with == operator in conditional statement

The following sketch works as expected (on receipt of incoming MIDI data an LED is lit for 4ms, this will ultimately be a 4ms pulse trigger for some analogue drum machine voices) with the uncommented if statement at line 26. As soon as I add ‘noteByte == 36’ to the conditional statements (see line 25, commented) it fails.

I have confirmed MIDI note number 36 is being sent with some MIDI monitoring software. I have Serial.print’d ‘noteByte’ to the serial monitor and ‘$’ is output. This is ASCII character ’36’. I assume therefore that noteByte does indeed equal ’36’.

So what’s wrong with the code? Worth noting that I did try Serial.print(noteByte, DEC) and ‘$’ was still output. Is noteByte being cast to a non integer somehow (which would make noteByte == 36 be falsey). The only other thing I can think of is you can’t apply == operator to a const - but that sounds like madness!

Thanks
Jim

byte commandByte;
byte noteByte;
byte velocityByte;
byte const noteOn = 144;
byte const bass = 36;
int const exampleOutputPin = 13;
unsigned long bassTimer;
int bassPlaying = 0;
int pulseWidth = 4;

void setup(){
  bassTimer = millis();
  Serial.begin(31250);
  pinMode(exampleOutputPin, OUTPUT);
  digitalWrite(exampleOutputPin, LOW);
}

void checkMIDI(){
  do{ 
    if (Serial.available()) {
      commandByte = Serial.read();
      noteByte = Serial.read();
      velocityByte = Serial.read();  
      if (commandByte == noteOn){
        // if (noteByte == 36 && velocityByte > 0 && bassPlaying == 0){ <-- FAILS, Serial.print(noteByte) --> $
        if (velocityByte > 0 && bassPlaying == 0){ // <-- WORKS
          bassTimer = millis() + pulseWidth;
          bassPlaying = 1;
          digitalWrite(exampleOutputPin,HIGH);//turn on led
        }
      }
      
    }
  }
  while (Serial.available() > 2);
}
    
void loop(){
  checkMIDI();
  if (millis() > bassTimer) {
    digitalWrite(exampleOutputPin,LOW);
    bassPlaying = 0;
  }
}
    if (Serial.available()) {
      commandByte = Serial.read();
      noteByte = Serial.read();
      velocityByte = Serial.read();

That means "if 1 or more characters are available, read three characters". If only the first character has actually arrived you will get "commandByte" and then get 0xFF (-1) for noteByte and velocityByte. That would produce the symptom you see: 0xFF != 36. Try:

    if (Serial.available() >= 3) {
      commandByte = Serial.read();
      noteByte = Serial.read();
      velocityByte = Serial.read();

Thanks, the code is cut'n'pasted from somewhere

Doesn't the surrounding while loop chunk the incoming data into three bytes though?

 do{ 
    if (Serial.available()) {
      commandByte = Serial.read();
      noteByte = Serial.read();
      velocityByte = Serial.read();  
...
     }
  while (Serial.available() > 2);

And how does that explain why it works without the noteByte condition?

No, it does not. When it enters the if you only know for sure there is one. And when you enter you use three... But it is possible that all three are there already but you're just not sure...

I would suggest instead:

while(Serial.available()<3);   // wait until 3 bytes are in the buffer
commandByte = Serial.read();   // then get them!
noteByte = Serial.read();
velocityByte = Serial.read();

by using the if() statement,

if (Serial.available() >= 3) {

it will be completely skipped if there is less available yet, but you wouldn’t simply wait.
by

if (Serial.available() ) {

you enter the if-corpus already at the 1st byte available before the buffer is filled with 3 -
also nonsense!

Or you could use something like:

   char buffer[4];
   int charsRead;

   if (Serial.available() > 0) {
      charsRead = Serial.readBytesUntil('\n', buffer, sizeof(buffer) - 1); 
      commandByte  = buffer[0];   // then get them!
      noteByte     = buffer[1];
      velocityByte = buffer[2];
      // rest of code

The only difference here is that once the first byte is read, it stays there until 3 bytes or a newline is read. Just a slightly different tool to hang on your tool belt.

NO!!
you have to wait until the Serial buffer is filled with 3 bytes!
don't assume anything weird - do it like I adviced you!

@ArthurD: To whom are you saying No?

to anyone who wants to resolve and fix this bug!
this is the correct way to wait for 3 bytes, and then for follow-up reading!

I don't have the precedence order handy but you might add additional
() to make sure it interprets things correctly.

(noteByte == 36 && velocityByte > 0 && bassPlaying == 0)

Dwight

why the heck not just first of all use my suggestion!?
why is there a need to assume and suggest anything weird instead before?

let dcjim, the TO, try my suggestion and we'll see.

ArthurD:
why the heck not just first of all use my suggestion!?
why is there a need to assume and suggest anything weird instead before?

let dcjim, the TO, try my suggestion and we'll see.

Strange issue with == operator in conditional statement - #5 by ArthurD - Programming Questions - Arduino Forum

lol, i shall do so right now and report back! :slight_smile:

@AthurD: Silly me. I thought there might be more than one way to solve his problem. Somehow I missed the crown on your head.

econjack:
@AthurD: Silly me. I thought there might be more than one way to solve his problem. Somehow I missed the crown on your head.

lol

like this for example, i took a slightly different approach - if a note on command was received assume the next two bytes when received to be note value and velocity - and filled out my spec a little - it works, i have two LEDs blinking on separate MIDI notes - later to be drum voices not LEDs of course

thanks for the help gents

int rxByte = 0;
boolean noteOnRx = false;
byte const noteOn = 144;
boolean noteRx = false;
byte note;
byte veloc;

int const bassTriggerPin = 13;
byte const bass = 36;
unsigned long bassTimer;
boolean bassTriggering = false;

int const snareTriggerPin = 12;
byte const snare = 38;
unsigned long snareTimer;
boolean snareTriggering = false;

unsigned long pulseWidth = 4;

void setup() {
  Serial.begin(31250);
  pinMode(bassTriggerPin, OUTPUT);
  digitalWrite(bassTriggerPin, LOW);
  pinMode(snareTriggerPin, OUTPUT);
  digitalWrite(snareTriggerPin, LOW);
}

void loop() {

  // process serial in
  
  if (Serial.available() > 0) {
    
    // byte available
    rxByte = Serial.read();

    // is it beginning of note on command sequence   
    if (rxByte == noteOn && !noteOnRx) {   
        
      noteOnRx = true;    
       
    } else if (noteOnRx && !noteRx) {
      
      // had note on command but no note, assume to be note
      note = rxByte;
      noteRx = true; 
           
    } else if (noteOnRx && noteRx) {   
        
      // had note on and note, command sequence ends with velocity
      veloc = rxByte;
      
      // re-initiate command sequence
      noteOnRx = false;
      noteRx = false;

      // process note, if not already triggered
      
      // bass trigger
      if (note == bass && !bassTriggering) {
        bassTriggering = true;
        digitalWrite(bassTriggerPin, HIGH);
        bassTimer = millis() + pulseWidth;
      }
      
      // snare trigger
      if (note == snare && !snareTriggering) {
        snareTriggering = true;
        digitalWrite(snareTriggerPin, HIGH);
        snareTimer = millis() + pulseWidth;
      } 
           
      // debug @ 31250 bps
      // see miniterm.py http://www.sinneb.net/?p=136
      Serial.println(note,DEC);
      Serial.println(veloc,DEC);     
    }
  }

  // expire triggers when complete

  if (millis() > bassTimer) {
    digitalWrite(bassTriggerPin, LOW);
    bassTriggering = false;
  }

  if (millis() > snareTimer) {
    digitalWrite(snareTriggerPin, LOW);
    snareTriggering = false;
  }
  
}

@econjack:
of course one may try to iron out a mistake by a workaround - or resolve the fundamental mistake by a fundamental logical aproach.
If you want to read 3 bytes from a buffer (i.e., the Serial buffer), you are well advised to first wait until those 3 bytes will have finally been stored to the buffer.
And when they finally are where you expected them to be, then you may read them from the buffer.
That's a matter of logic skills, not of a crown.

ArthurD:
@econjack:
of course one may try to iron out a mistake by a workaround - or resolve the fundamental mistake by a fundamental logical aproach.
If you want to read 3 bytes from a buffer (i.e., the Serial buffer), you are well advised to first wait until those 3 bytes will have finally been stored to the buffer.
And when they finally are where you expected them to be, then you may read them from the buffer.
That's a matter of logic skills, not of a crown.

Plain and true - if you expect 3 bytes - wait for three.

My view is that there is a major "urban legend" how "available" works due to two "false doctrines" - never use delay() or any kind of wait for "available" in you code - use loop() and if( available) so the processor is kept busy irregardless if it is required or not. ( And add millis() to convolute the code more - famous blink without delay())) .
Or maybe using "while / do " is taboo in Arduino "experts" circles.

@ArthurD: Did you even bother to read the code I posted? Please explain to me why my code does not wait for 3 bytes of code or a newline character. (Someone or a malfunction could abort the input stream.)
Do you really think there's only one software solution to a given problem or that yours is the only one that will work?

@econjack, your method is fine if you expect the characters to become available within the set timeout, however it is always good to check the return of readBytesUntil to ensure the correct number of bytes was received.

@ArthurD,
Then again, if it takes over a second (or whatever timeout occurs), the data may be completely invalid (no automated system would really take this long), and you can detect these timing errors, which is not directly detectable if your app simply relies upon using x.available() to wait an undetermined amount of time.

Both methods have their merit when used appropriately.

I have posted it -
Your question at the beginning of this thread was, why the former "== operator in conditional statement" don't work, and the reason for that I explained above:

I will go on record as saying all of those approaches are wrong, and will eventually fail. You can’t depend on timing, except as a timeout to decide the message is either not coming, or incomplete. If you always wait for three characters before doing anything, then if you ever get out of sync with the sender, you’ll never recover. Consider the case where you miss a character, and pickup the last two characters of one packet, and the first character of the next. The correct way to handle it is by looking at the characters AS THEY COME IN, and finding the beginning and end of each packet, discarding any invalid messages. Anything else will simply not be robust.

Regards.
Ray L.