Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link
  1. Replace/mark the byte with an error byte

    Replace/mark the byte with an error byte

    If you do this then you can be more sure that you will get 10 bytes; but you can only do it if the protocol allows you to recognize an illegal byte value.

  2. Or remove the error byte

    If you do this then you must be able to recover from getting 9 bytes sometimes.

If you do this then you can be more sure that you will get 10 bytes; but you can only do it if the protocol allows you to recognize an illegal byte value. 2. Or remove the error byte

If you do this then you must be able to recover from getting 9 bytes sometimes.

  1. Replace/mark the byte with an error byte

If you do this then you can be more sure that you will get 10 bytes; but you can only do it if the protocol allows you to recognize an illegal byte value. 2. Or remove the error byte

If you do this then you must be able to recover from getting 9 bytes sometimes.

  1. Replace/mark the byte with an error byte

    If you do this then you can be more sure that you will get 10 bytes; but you can only do it if the protocol allows you to recognize an illegal byte value.

  2. Or remove the error byte

    If you do this then you must be able to recover from getting 9 bytes sometimes.

try to explain the bug and how to fix it
Source Link
ChrisW
  • 13.1k
  • 1
  • 35
  • 76

I think there's an obvious bug in the code. After you read 16 bytes, you completely clear your serial_buffer by zeroing si_processed, and you then think "success!" after each next 10 bytes you read.

Instead you should:

  • If you read too much data, don't discard the extra bytes: instead use memmove to move them to the beginning of the buffer (assuming they're the start of the next packet), and then read the subsequent bytes into the vacant space after them.

Something like:

// after reading and processing a 10-byte packet
if (si_processed > 10) // already have the start of the next packet
{
 // move start of next packet to start of buffer
 memmove(serial_buffer, serial_buffer+10, si_processed - 10);
 // not 0: remember how many start bytes we already have
 si_processed -= 10;
}
  • After you read any bytes, verify whether the start byte is your 0xA5 value, and discard the bytes if not.

Something like:

if (si_processed > 0) // have some bytes
{
 int i;
 for (i = 0; i < si_processed; ++i)
 if (serial_buffer[i] == 0xA5) // found start of packet
 break;
 if (i > 0)
 {
 // start of packet is not start of buffer
 // so discard bad bytes at the start of the buffer
 memmove(serial_buffer, serial_buffer+i, si_processed - i);
 si_processed -= i;
 }
}

I think there's an obvious bug in the code. After you read 16 bytes, you completely clear your serial_buffer by zeroing si_processed, and you then think "success!" after each next 10 bytes you read.

Instead you should:

  • If you read too much data, don't discard the extra bytes: instead use memmove to move them to the beginning of the buffer (assuming they're the start of the next packet), and then read the subsequent bytes into the vacant space after them.

Something like:

// after reading and processing a 10-byte packet
if (si_processed > 10) // already have the start of the next packet
{
 // move start of next packet to start of buffer
 memmove(serial_buffer, serial_buffer+10, si_processed - 10);
 // not 0: remember how many start bytes we already have
 si_processed -= 10;
}
  • After you read any bytes, verify whether the start byte is your 0xA5 value, and discard the bytes if not.

Something like:

if (si_processed > 0) // have some bytes
{
 int i;
 for (i = 0; i < si_processed; ++i)
 if (serial_buffer[i] == 0xA5) // found start of packet
 break;
 if (i > 0)
 {
 // start of packet is not start of buffer
 // so discard bad bytes at the start of the buffer
 memmove(serial_buffer, serial_buffer+i, si_processed - i);
 si_processed -= i;
 }
}
Post Undeleted by ChrisW
Post Deleted by ChrisW
added 412 characters in body
Source Link
ChrisW
  • 13.1k
  • 1
  • 35
  • 76

As well as handling too few bytes (above), you could modify the above to check for receiving more than 10 bytes: after you receive 10 bytes, do another blocking-read-with-small-timeout to verify that there are no more bytes to receive. If there are extra bytes, then read them all before returning to the 'waiting-for-next-10-bytes' state (otherwise, these extra bytes will mess up your next 10-byte packet).

As well as handling too few bytes (above), you could modify the above to check for receiving more than 10 bytes: after you receive 10 bytes, do another blocking-read-with-small-timeout to verify that there are no more bytes to receive. If there are extra bytes, then read them all before returning to the 'waiting-for-next-10-bytes' state (otherwise, these extra bytes will mess up your next 10-byte packet).

Source Link
ChrisW
  • 13.1k
  • 1
  • 35
  • 76
Loading
lang-c

AltStyle によって変換されたページ (->オリジナル) /