current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Because the throughput of my radio and device is limited, I wanted to compress my control string. I decided to compress the command into an 8 bytes (quasi) string (throttle high, throttle low, pitch, roll, yaw plus/minus, yaw value, checksum, stop byte) and after parsing in four variables:

  • Pitch (between -45° and 45°)
  • Roll (between -45° and 45°)
  • Yaw (between -180° and 180°)
  • Throttle (between 1000 and ~1900)

Because Some number ranges don't fit into a single byte (0 to 255 number range), I used two instead (throttle: one byte for hundreds (1-9) and one byte for tenners (1-99); Yaw: One byte for plus or minus and one byte for 0° - 180°). I send and read these bytes as characters. Then I convert them into numbers (0-255).

I noticed that sometimes a few reads are totally out of range (e.g. value: -31001) but checksum test is OK. I don't know exactly why (maybe radio, maybe I caused unexpected behavior because of type conversions here). This is why I made an additional range test. Now the remote control seems to be reliable.

However I don't feel very nice about it, because I do signed/unsigned and! number range conversions, the checksum function is also not very reliable. Maybe someone has suggestions how to make the code better.

Code (The command stored in buffer is already red correctly until the stop byte and valid in length):

bool Receiver::parse_radio(char *buffer) {
  int16_t throttle_high  = (uint8_t)buffer[0];       // 1. 1 - 9
  int16_t throttle_low   = (uint8_t)buffer[1];       // 2. 0 - 99
  int16_t pit            = (uint8_t)buffer[2] - 127; // 3. -45° - 45°
  int16_t rol            = (uint8_t)buffer[3] - 127; // 4. -45° - 45°
  int16_t yaw_pm         = (uint8_t)buffer[4] - 127; // 5. -1 || +1
  int16_t yaw_val        = (uint8_t)buffer[5];       // 6. yaw angle in degrees 180°

  int16_t chk            = (uint8_t)buffer[6];       // 7. checksum

  int16_t thr            = 1000 + (throttle_high * 100) + throttle_low;
  int16_t yaw            = yaw_val * yaw_pm;

  // Calculate checksum
  uint8_t checksum = 0;
  for(int i = 0; i < 6; i++) {
    checksum = (checksum + buffer[i]) << 1; // % 256 can be saved because of the uint8_t
  }

  // Validity check: 
  // First checksum
  if(checksum != chk)
    return false;
  // Later other values
  if(yaw_pm < -1 || yaw_pm > 1)
    return false;
  if(rol > 45 || rol < -45)
    return false;
  if(pit > 45 || pit < -45)
    return false;
  if(yaw > 180 || yaw < -180)
    return false;
  if(thr > RC_THR_80P || thr < RC_THR_MIN)
    return false;

  // Do rest here
  return true;
}
share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

I don't know exactly why (maybe radio, maybe I caused unexpected behavior because of type conversions here).

Can you keep statistics on how many checksum errors there are?

  • If there are many that implies that the communications link is noisy
  • If there are few or none then it's more probably something else (e.g. a software error)

I'm not sure that you're using a good checksum algorithm.; you might like to use a CRC instead.

You're only using 7 of your 8 bytes so you could have a 2-byte CRC (or checksum).

You could compress your data better:

  1. '1 - 9'
  2. '0 - 99' => 10 bits to store 0..999 throttle
  3. '-45° - 45°' => 7 bits to store 90 angle values
  4. '-45° - 45°' => 7 bits to store 90 angle values
  5. '-1 || +1'
  6. 'yaw angle in degrees 180°' => 9 bits to store 360 angle values

10+7+7+9 => 37 bits = 5 bytes.

Alternatively 999 * 90 * 90 * 360 = 2913084000 which is less than 232. So you could fit your data into 4 bytes and use a 4-byte CRC.

share|improve this answer
    
For this I have to use bitmasks. I noticed, that appr. 1/20 commands fails checksum and maybe one of ~200 is out of range. But I was not doing specific statistics. I will make an update if i did. –  dgrat Feb 20 at 11:27
    
You don't need bitmasks. To encode to a 4-byte unsigned integer, use this formula: throttle + 1000* pitch + 1000*91* roll + 1000*91*91* yaw where the variables e.g. pitch are zero-based unsigned integers. The decode the 4-byte number, use the equivalent division / and remainder % operators. –  ChrisW Feb 20 at 11:47
    
1/20 is a lot of errors. Assuming those are hardware/communications errors (not software errors) I suggest a strong CRC. For improved error detection you could also transmit each packet twice, and ensure that they match as well as passing their CRC check. –  ChrisW Feb 20 at 12:11
add comment
up vote 1 down vote

I suggest you cast you buffer elements to int16_t not uint8_t.

Here is an example

#include <stdio.h>
#include <inttypes.h>

int main()
{
    char buffer = -45;
    printf("Original = %d\n", buffer);
    printf("uint8_t  = %d\n", (uint8_t)buffer);
    printf("int16_t  = %d\n", (int16_t)buffer);

    int16_t a = (uint8_t)buffer - 127;
    int16_t b = (int16_t)buffer - 127;
    printf("a = %d\n", a);
    printf("b = %d\n", b);
}

Output:

Original = -45
uint8_t  = 211
int16_t  = -45
a = 84
b = -172

As you can see in the second line, when you change a negative integer to an unsigned integer it cannot retain its negative status and instead becomes a large positive number.

Edit: Sorry I didn't realize the date on this question. I guess I'll just leave it.

share|improve this answer
    
I like the answer anyway. I changed some uint8 to 16. Exception is the checksum. However not tested yet. –  dgrat May 13 at 11:56
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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