Controlling motors using USB serial connection from Raspberry Pi to Arduino

I am working on building an RC car/robot with Raspberry Pi and Arduino. I connected Arduino to Raspberry Pi using USB and send serial commands with python. I haven't done much programming with Arduino nor Python.

This just the beginning of this project and would like to get feedback on the code. Is it a good practice to use byte, bit masks and bitwise operators to send and decode commands? Is it better to send string commands? Also would I would like to get feedback on general structure and code logic for this type of program.

The car that I am using has two motors that control each side of the car. I think I have to slowdown the right side to go right and left side to go left.

I am using pygames to capture key presses. Some of the code was taken from GitHub.

Arduino

```
//Pin Functions
#define motorLeftSpeedPin 6 //pwm
#define motorLeftForwardPin A0
#define motorLeftBackwardPin A1

#define motorRightSpeedPin 5 //pwm
#define motorRightForwardPin A2
#define motorRightBackwardPin A3

#define speakerPin A5
#define ledPin 2

//define commands

#define STOP 0x00 //0000
#define FORWARD_DIRECTION 0x01 //0001
#define BACKWARD_DIRECTION 0x02 //0010
#define LEFT_DIRECTION 0x04 //0100
#define RIGHT_DIRECTION 0x08 //1000
#define MOTORLEFT 0x10 //0001 0000
#define MOTORRIGHT 0x20 //0010 0000
#define SET_SPEED 0x40 //0100 0000

#define TURN_SPEED_OFFSET 20
#define MINIMUM_MOTOR_SPEED 100
#define MAXIMUM_MOTOR_SPEED 250

struct Motor
{
byte mSide;
byte mSpeed;
}motorLeft, motorRight;

struct Command
{
byte cmdID;
byte data1;
byte data2;
byte checkSum;
};

enum COMMAND_IDS
{
INVALID_CMD = 0,
DRIVE = 10
};

byte currentDirection = 0x00;

void dbg_print(const char * s)
{
//#if DEBUG
Serial.print(s);
//#endif
}

void processCommand(struct Command &command)
{
//prcess recieved command
switch(command.cmdID)
{
case DRIVE:
dbg_print("Drive ...");
driveCar(command);
break;
default:
//unknown comm

C++

Please use some form of consistent indentation, currently it's all over the place. For example:

if (command.data1 & FORWARD_DIRECTION){
      goForward();
      dbg_print("Drive Forward ...");   
 }else if (command.data1 & BACKWARD_DIRECTION){
   goBackward();
   dbg_print("Drive Backward ...");
 }

This is so much easier to read if the indentation is always the same amount.

Prefer const variables over #define, for example:

#define STOP 0x00 //0000

Could be better written as:

const uint8_t STOP = 0x00;

If you end up using a proper in circuit emulator you will probably find the additional information about the variable names useful.

If you aren't changing a parameter consider passing it by const reference. For example:

void driveCar(struct Command &command){

Can become

void driveCar(Command const& command){

Then if you attempt to modify command you will get a compiler error. This can help you write more correct code by avoiding a class of bugs.

Consider placing your incoming command handler in it's own ISR, this way you won't lose incoming information when other parts of your code use delays. In simpler code you might be fine because the USART on the atmega chips have a hardware buffer, but if your code spends too long somewhere else you will potentially lose data. This could be a problem especially if you use busy-wait delays in your code. If writing a proper ISR is a pain in the Adruino IDE you might also want to going with a proper C++ toolchain such as AVR-GCC.

You also have a lot of strings scattered throughout your code, these will use up the SRAM on the Arduino boards. You don't have a lot of space on these devices so moving these strings into progmem will save you precious memory.
Python

There is a bunch of trailing whitespace, this can cause issues when you import your code into a version control system. In general this is a bit of a code smell, you probably want to clean this up.

You have a disturbingly large number of global variables, you might want to consider grouping related variables into other structures.

For example:

STOP = 0x00 
FORWARD_DIRECTION = 0x01
BACKWARD_DIRECTION = 0x02  
LEFT_DIRECTION = 0x04
RIGHT_DIRECTION = 0x08 
MOTORLEFT = 0x10
MOTORRIGHT = 0x20 
SET_SPEED = 0x40

Could perhaps be better stored in a dictionary

COMMANDS = {
    "STOP": 0x00,
    "FORWARD_DIRECTION": 0x01,
    "BACKWARD_DIRECTION": 0x02,
    "LEFT_DIRECTION": 0x04,
    "RIGHT_DIRECTION": 0x08,
    "MOTORLEFT": 0x10,
    "MOTORRIGHT": 0x20,
    "SET_SPEED": 0x40,
}

You might find doing some similar cleaning up with all the GUI variables to also be worthwhile.

It seems like your Python code doesn't handle the incoming prints from the Arduino, you might want to put some sort of input handling for the serial in place. This might require a separate thread if you wish to also be able to send commands at the same time as receiving data.

StackExchange Code Review Q#100548, answer score: 3