1

I am working on building a serial command receiver. The idea is came from the boot loader. However, I got a very strange problem that I have no idea what could cause it.

As the first screenshot show, the print isn't finished because "===a=" is stuck, like this. enter image description here

In the code, it should print out and the others.

char* receiveCmd(Stream &_serial, char &cmd_ptr, int &l_ptr) {
  Stream* serial = &_serial;

  if (serial->available() && (uint8_t)serialRead(_serial) == CMD_START) {
    int buf_count = 0; //start at data
    cmd_ptr = serialRead(_serial); //cmd_byte

    char low_byte = serialRead(_serial);
    char high_byte = serialRead(_serial);

    uint16_t length = low_byte; //data_length
    length = length | (high_byte << 8);
    

    Serial.println("\n\n===a===");
    Serial.println(length);
    Serial.println("===b===");

    l_ptr = (int)length;

    char* _buf = (char*)malloc(length * sizeof(char));

    while (buf_count != length) {
      if (serial->available()) {
        _buf[buf_count] = serial->read();
        buf_count++;
      }
    }

    char checksum = serialRead(_serial); //checksum, don't bother it yett
    char stop_byte = serialRead(_serial);

    if (stop_byte != CMD_EOF) {
      free(_buf);
      return NULL; //error
    }

    return _buf;
  }

  return NULL;
}

Anyway, the problem is fixed when I print out other variables first, then the length var..

char* receiveCmd(Stream &_serial, char &cmd_ptr, int &l_ptr) {
  Stream* serial = &_serial;

  if (serial->available() && (uint8_t)serialRead(_serial) == CMD_START) {
    int buf_count = 0; //start at data
    cmd_ptr = serialRead(_serial); //cmd_byte

    char low_byte = serialRead(_serial);
    char high_byte = serialRead(_serial);

    uint16_t length = low_byte; //data_length
    length = length | (high_byte << 8);
    

    Serial.println("\n\n===a===");
    Serial.println(low_byte, HEX);
    Serial.println(high_byte, HEX);
    Serial.println(length);
    Serial.println("===b===");

    l_ptr = (int)length;

    char* _buf = (char*)malloc(length * sizeof(char));

    while (buf_count != length) {
      if (serial->available()) {
        _buf[buf_count] = serial->read();
        buf_count++;
      }
    }

    char checksum = serialRead(_serial); //checksum, don't bother it yett
    char stop_byte = serialRead(_serial);

    if (stop_byte != CMD_EOF) {
      free(_buf);
      return NULL; //error
    }

    return _buf;
  }

  return NULL;
}

Other code is exactly the same. Does anyone know what may cause it? I can't print out those thing during production because it is only for debugging. If I remove those print, the code is also stuck(because of the length variable again).

For additional information, the serial signal is generated from other Arduino and AltSerial Library. The structure is like this.

void sendCmd(Stream &_serial, char cmd, char* payload, int length) {
  Stream* serial = &_serial;
  char buf[6 + length]; //start, data_length, data, checksum, stop

  buf[0] = CMD_START; //start_byte
  buf[1] = cmd; //cmd_byte
  buf[2] = length & 0xff; //data_length - low byte
  buf[3] = (length >> 8) & 0xff; //data_length - high byte
  buf[4 + length] = (char)calcCRC(payload); //checksum
  buf[5 + length] = CMD_EOF; //stop_byte

  for (int i = 0; i < length; i++) //load buf
    buf[4 + i] = payload[i];

  for (int i = 0; i < sizeof(buf); i++)
    serial->print(buf[i]);
}

I have used Serial.print(altSerial.read(), HEX) to check whether the byte is received correctly. They are all correct. That's why it is confusing me.QAQ

===== UPDATED INFORMATION =====

I have narrowed the problem. It seems like the below code is the main cause regard the issue. But no solution for this still.

uint16_t length = low_byte; //data_length
length = length | (high_byte << 8);

Thank you.

16
  • please, no pictures of text ... add the actual text
    – jsotola
    Jun 4 at 22:12
  • 1
    @jsotola Do you mean the code? Ok, I will change it. Jun 4 at 22:14
  • Are you sure _buf is actually being allocated? You should always check if malloc() returns NULL before using the memory you think you've allocated.
    – Majenko
    Jun 4 at 22:45
  • @Majenko Thank you so much! It seems like the cause of the problem, but how do I check the buffer has been allocated? Jun 4 at 22:50
  • if (_buf == NULL) { // do something to alert the user and abort the operation }
    – Majenko
    Jun 4 at 22:51
2

Why it does not work? You already got the answer in comments:

  • You are reading more bytes from the serial stream than what you know are available. Then, your length variable can contain garbage, which makes malloc() fail even if you have plenty of available memory.

  • You write into the memory buffer even if the allocation failed, which makes your program corrupt its memory. From this point, anything can happen.

You may notice that the behavior of your program depends on the timing. Adding more instructions, or delays, may make it work. This is simply because you give your peer more time to send its data packet. Of course, relying on the timings is not a good approach: you instead have to make sure you never read() more than what is actually available().

Here is an (untested) attempt to do this correctly and in a non-blocking fashion. It is based on a finite state machine. What is original about this state machine is that it is implemented as a switch/case where every single case falls through:

/*
 * Packet format:
 *   CMD_START cmd_byte length_L length_H [payload] checksum CMD_EOF
 */
char *receiveCmd(Stream &serial, char &cmd_ref, int &l_ref) {

    // The current state is defined by what parts of the packet
    // we have already received.
    static enum {
        GOT_NONE, GOT_START, GOT_HEADER, GOT_PAYLOAD
    } state = GOT_NONE;

    // Variables only valid in the states GOT_HEADER and GOT_PAYLOAD.
    static char cmd_byte;
    static uint16_t length;
    static char *buffer;
    static size_t buffer_pos;

    switch (state) {
        case GOT_NONE:
            if (serial.available() < 1 || serial.read() != CMD_START)
                break;
            state = GOT_START;
            // fallthrough
        case GOT_START:
            if (serial.available() < 3)
                break;
            cmd_byte = serial.read();
            length = serial.read();
            length |= (uint16_t) serial.read() << 8;
            buffer = (char *) malloc(length);
            if (!buffer) {
                Serial.print(F("ERROR: Failed to allocate "));
                Serial.print(length);
                Serial.println(F(" bytes."));
                state = GOT_NONE;
                break;
            }
            buffer_pos = 0;
            state = GOT_HEADER;
            // fallthrough
        case GOT_HEADER:
            while (buffer_pos < length && serial.available())
                buffer[buffer_pos++] = serial.read();
            if (buffer_pos < length)
                break;
            state = GOT_PAYLOAD;
            // fallthrough
        case GOT_PAYLOAD:
            if (serial.available() < 2)
                break;
            serial.read();  // consume checksum
            if (serial.read() != CMD_EOF) {
                Serial.println(F("ERROR: Expected CMD_EOF."));
                free(buffer);
                state = GOT_NONE;
                break;
            }
            cmd_ref = cmd_byte;
            l_ref = length;
            state = GOT_NONE;
            return buffer;
    }
    return NULL;
}

You can think of this switch/case as a set of actions to perform sequentially in order to process the whole packet. However, if the function gets stuck for lack of incoming data, it just bails out. On the next call, the switch puts it back where it left.

1
  • Thank you very much. I have found the problem. It is just not enough memory. therefore, it fails. However, I will take your suggestion for the 'receiveCmd' function. Jun 5 at 11:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.