8 years, 4 months ago.

I think you have a timing and buffer size issue. This is the sort of problem that it's very common to get tripped up by if you aren't used to embedded stuff that has to deal with the hardware.

Currently as soon as some data is received you copy it into your stringOverSerialBuffer buffer. The rx interrupt doesn't wait for the entire message, it triggers as soon as the first byte is received.

That removes that data from the serial port buffer rx and you end up with a string of length 1 and print out an error.

Printing out the error is slow and so takes time (printf is slow but more importantly sending data over the serial is very slow if the message is longer than the hardware Tx buffer.) During that time several more bytes are received. You read them (or at least the most recent few, the hardware receive buffer will be 16 bytes or less depending on the CPU, anything over that buffer size is lost), realize you have less than 40 bytes and print an error. This repeats until the end of the input string.

What you need to do is read the data from the serial port into your buffer as it arrives but only process the message once you have the whole thing.

e.g. (note - this is untested but I think it will work)

//attributes - program variables
char stringOverSerialBuffer[41];    // buffer to store received string over pc
int bytesRecieved = 0;

volatile bool newCommandFlag = false;             // flag for ISR - volatile so that the main loop picks up changes from the ISR.
 
void serialDataCallback() { 

    while (pc.readable()) { // read all available data
        if ((bytesRecieved  == 40) || newCommandFlag)  // avoid buffer overflow
          pc.getc();
        else {
          stringOverSerialBuffer[bytesRecieved] = pc.getc();   // get waiting data
          bytesRecieved++;
          if ((bytesRecieved == 40) || (stringOverSerialBuffer[bytesRecieved-1] == '\n')) {   // buffer full or a new line
            stringOverSerialBuffer[bytesRecieved] = 0;                                                              // append a null
            newCommandFlag  = true;
          }
    }
}
 
int main() {
    pc.attach(&serialDataCallback);  // attach pc ISR
 
    while (1) {
        while (!newCommandFlag) {
            pc.printf("waiting for command.\n");
            wait(1);
        }
        if (newCommandFlag) {  // if command has been read
            if (strlen(stringOverSerialBuffer) != 40) {
                pc.puts("unknown command");   // puts has less overhead than printf.
            } else {
                pc.printf("received: %s \n\r", stringOverSerialBuffer);
 
                if (startsWith(stringOverSerialBuffer, "L_ON")) {
                    pc.puts("turn light on.\n");
                    myled = 1;
 
                } else if (startsWith(stringOverSerialBuffer, "L_OFF")) {
                    pc.puts("turn light off.\n");
                    myled = 0;
                }
            }
 
            bytesRecieved = 0;  // clear the message size pointer so the next message is at the start of the buffer
            newCommandFlag = false;   // clear the flag.
        }
    }
}

To answer your questions: Q1 - toggling/setting LEDs is a good way to track the progress of the code with minimal impact on timings. If you want serial port tracing in time critical sections of code use putc to output a single character rather than printf and a long string. Assuming the serial port transmit buffer is empty putc has minimal overhead.

Q2 - Not really, see above.

Q3 - You shouldn't need to but you could disable them while processing the message. Your current code could lock up if a byte arrived at the wrong time. You ignore interrupts without reading the incoming data while newCommandFlag is set, the interrupt for that byte won't get generated again. If that was the last byte you will never see it. Some systems only generate an rx interrupt on the first byte, if you ignore it you'll never get another rx interrupt until you empty the buffer.

Q4 - They are effectively identical. RxIrq is the default interrupt type if not stated.

Q5 - No. the best way out of blocked code is to not block. There are situations where a timeout is useful (e.g. in my code above you could have a timeout if you received some data but didn't get 40 bytes or a line end within a reasonable time).

One other gotcha to keep in mind - If you have code along the lines of

int myflag;

void someInterruptCode () {
  myflag = false;
}

main {
  myflag = true;
  setupInterrupt();
  while (myflag) {
    ...
  }

  FlagChanged()
}

The system could get stuck forever in the while loop even if the interrupt was triggered. The compiler tries to be clever, it knows that myflag is set to true, if nothing in the while loop changes that then that condition is always true and so could potentially optimize out that check.

This can really throw people because one compile it may work and the next compile it won't with no clear reason why things changed.

The keyword volatile (as used in my code above) indicated that the variable could change without notice and so the compiler should always check the value. Any variables that are changed within an interrupt and then checked in the main code should be marked as volatile in order to ensure the compiler doesn't try to be too clever. stringOverSerialBuffer is OK since that's a pointer (in c pointers and arrays are basically the same thing). The compiler may assume that the address it points to is unchanged (which is true) but it won't assume that the data in that address is unchanged.