Need HELP!! Serial communication has a very "Strange Problem"

Question

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.

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.

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 04 '21 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? — Lu Chih Yuan, Jun 04 '21 at 22:50
`if (_buf == NULL) { // do something to alert the user and abort the operation }` — Majenko, Jun 04 '21 at 22:51
@Majenko It is not allocated because of the lack of memory? I think that I have enough memory for that. — Lu Chih Yuan, Jun 04 '21 at 22:54
It's *always* worth checking. You may *think* you have enough, but you may not, since other things besides you use the memory. — Majenko, Jun 04 '21 at 22:56
@Majenko char serialRead(Stream &_serial) { Stream* serial = &_serial; delay(1); return (uint8_t)serial->read(); } — Lu Chih Yuan, Jun 04 '21 at 23:24
You should only ever read from serial if `.available()` has told you that there is actually a byte there to read, or if you check that you actually read a byte after reading (it will return -1 if there was nothing to read). You check if there is at least one byte available, then proceed to read loads of bytes that may, or may not, be there. You're entirely beholden to timing quirks to get it right, and that is bad. You should always wait for a byte to actually be there before attempting to read it. — Majenko, Jun 04 '21 at 23:30
[Here](https://github.com/MajenkoLibraries/ICSC/blob/master/src/ICSC.cpp#L229) is an example of how to handle reading of packets in a non-blocking manner. — Majenko, Jun 04 '21 at 23:31
@Majenko I did, there is a loop function to repeatedly check there is any available byte to read. Only after that, function receiveCmd will be called. The baud rate is 115200. — Lu Chih Yuan, Jun 04 '21 at 23:36
Yes, but you only check once for a single byte at the start of readCommand, and then proceed to read multiple bytes. How do you know those bytes have arrived? You don't. — Majenko, Jun 04 '21 at 23:38
what for is the pointer alias `Stream* serial` if you have a reference? — Juraj, Jun 05 '21 at 05:07

score 2 · Accepted Answer · answered Jun 05 '21 at 09:40

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.

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. — Lu Chih Yuan, Jun 05 '21 at 11:01

Need HELP!! Serial communication has a very "Strange Problem"

1 Answers1