0

The code is controlling 3 blinds. There are 2 buttons for each blind for going each way and 2 buttons controlling all of them at the same time. The motorSerial is a way to control all of the blinds through the serial monitor where sending 1 goes one way and 2 the other way.

What I'm most concerned about is when pressing a button, it turns on the motor, waits for 0.2 seconds and turns the off. But if I'm holding the button down it will continuously keep turning it on and off. Does anyone have a better way of doing this?

const int PWMA = 13;
const int AI2_left = 12;
const int AI1_left = 11;
const int PWMB = 18;
const int BI1_middle = 10;
const int BI2_middle = 9;

const int PWMA2 = 8;
const int AI2_right = 7;
const int AI1_right = 6;

const int greenLeft1 = 5;
const int greenLeft2 = 4;
const int redMiddle1 = 3;
const int redMiddle2 = 2;
const int yellowRight1 = 14;
const int yellowRight2 = 15;
const int blueAll1 = 16;
const int blueAll2 = 17;

int motorSpeed_left = 0;
int motorSpeed_middle = 0;
int motorSpeed_right = 0;
int motorSpeed_all = 0;
int motorSerial = 0;

void setup() {
  pinMode(PWMA, OUTPUT);
  pinMode(AI2_left, OUTPUT);
  pinMode(AI1_left, OUTPUT);
  pinMode(BI1_middle, OUTPUT);
  pinMode(BI2_middle, OUTPUT);
  pinMode(PWMA2, OUTPUT);
  pinMode(AI2_right, OUTPUT);  
  pinMode(AI1_right, OUTPUT);

  pinMode(greenLeft1, INPUT_PULLUP);
  pinMode(greenLeft2, INPUT_PULLUP);
  pinMode(redMiddle1, INPUT_PULLUP);
  pinMode(redMiddle2, INPUT_PULLUP);
  pinMode(yellowRight1, INPUT_PULLUP);
  pinMode(yellowRight2, INPUT_PULLUP);
  pinMode(blueAll1, INPUT_PULLUP);
  pinMode(blueAll2, INPUT_PULLUP);

  Serial.begin(9600);
}

void loop() {

  if(Serial.available() > 0) {
    motorSerial = Serial.parseInt();
  }


  if(digitalRead(greenLeft1) == LOW) {
    motorSpeed_left = 200;
    spinMotor_left(motorSpeed_left);
    delay(200);
    motorSpeed_left = 0;
    spinMotor_left(motorSpeed_left);
  }
  else if(digitalRead(greenLeft2) == LOW) {
    motorSpeed_left = -200;
    spinMotor_left(motorSpeed_left);
    delay(200);
    motorSpeed_left = 0;
    spinMotor_left(motorSpeed_left);
  }
  else if(digitalRead(redMiddle1) == LOW) {
    motorSpeed_middle = 200;
    spinMotor_middle(motorSpeed_middle);
    delay(200);
    motorSpeed_middle = 0;
    spinMotor_middle(motorSpeed_middle);
  }
  else if(digitalRead(redMiddle2) == LOW) {
    motorSpeed_middle = -200;
    spinMotor_middle(motorSpeed_middle);
    delay(200);
    motorSpeed_middle = 0;
    spinMotor_middle(motorSpeed_middle);
  }
  else if(digitalRead(yellowRight1) == LOW) {
    motorSpeed_right = 200;
    spinMotor_right(motorSpeed_right);
    delay(200);
    motorSpeed_right = 0;
    spinMotor_right(motorSpeed_right);
  }
  else if(digitalRead(yellowRight2) == LOW) {
    motorSpeed_right = -200;
    spinMotor_right(motorSpeed_right);
    delay(200);
    motorSpeed_right = 0;
    spinMotor_right(motorSpeed_right);
  }
  else if(digitalRead(blueAll1) == LOW) {
    motorSpeed_left = 200;
    motorSpeed_middle = 200;
    motorSpeed_right = 200;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
    delay(200);
    motorSpeed_left = 0;
    motorSpeed_middle = 0;
    motorSpeed_right = 0;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
  }
  else if(digitalRead(blueAll2) == LOW) {
    motorSpeed_left = -200;
    motorSpeed_middle = -200;
    motorSpeed_right = -200;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
    delay(200);
    motorSpeed_left = 0;
    motorSpeed_middle = 0;
    motorSpeed_right = 0;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
  }
  else if(motorSerial == 1) {
    motorSpeed_left = 200;
    motorSpeed_middle = 200;
    motorSpeed_right = 200;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
    delay(2500);
    motorSpeed_left = 0;
    motorSpeed_middle = 0;
    motorSpeed_right = 0;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
    motorSerial = 0;
  }
  else if(motorSerial == 2) {
    motorSpeed_left = -200;
    motorSpeed_middle = -200;
    motorSpeed_right = -200;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
    delay(2500);
    motorSpeed_left = 0;
    motorSpeed_middle = 0;
    motorSpeed_right = 0;
    spinMotor_left(motorSpeed_left);
    spinMotor_middle(motorSpeed_middle);
    spinMotor_right(motorSpeed_right);
    motorSerial = 0;
  }


}

/*****************************************************/
void spinMotor_left(int motorSpeed_left)
{
  if (motorSpeed_left > 0)
  {
    digitalWrite(AI1_left, HIGH);
    digitalWrite(AI2_left, LOW);
  }
  else if (motorSpeed_left < 0)
  {
    digitalWrite(AI1_left, LOW);
    digitalWrite(AI2_left, HIGH);
  }
  else
  {
    digitalWrite(AI1_left, LOW);
    digitalWrite(AI2_left, LOW);
  }
  analogWrite(PWMA, abs(motorSpeed_left));
}


void spinMotor_middle(int motorSpeed_middle)
{
  if (motorSpeed_middle > 0)
  {
    digitalWrite(BI1_middle, HIGH);
    digitalWrite(BI2_middle, LOW);
  }
  else if (motorSpeed_middle < 0)
  {
    digitalWrite(BI1_middle, LOW);
    digitalWrite(BI2_middle, HIGH);
  }
  else
  {
    digitalWrite(BI1_middle, LOW);
    digitalWrite(BI2_middle, LOW);
  }
  analogWrite(PWMB, abs(motorSpeed_middle));
}


void spinMotor_right(int motorSpeed_right)
{
  if (motorSpeed_right > 0)
  {
    digitalWrite(AI1_right, HIGH);
    digitalWrite(AI2_right, LOW);
  }
  else if (motorSpeed_right < 0)
  {
    digitalWrite(AI1_right, LOW);
    digitalWrite(AI2_right, HIGH);
  }
  else
  {
    digitalWrite(AI1_right, LOW);
    digitalWrite(AI2_right, LOW);
  }
  analogWrite(PWMA2, abs(motorSpeed_right));
}
1

ratchet freak's long answer has many good points. I would also agree with Code Gorilla's suggestion to add a constructor to the struct. However, once you have a constructor, it pretty much looks like a regular C++ class¹, so it would make sense to add the motor-related functions as members of the class. Thus I would start by building a class that abstracts out the details of driving the motors:

class Motor {
public:
    Motor(uint8_t pin_1, uint8_t pin_2, uint8_t pin_pwm)
    : pin_1(pin_1), pin_2(pin_2), pin_pwm(pin_pwm) {}

    void begin() {
        pinMode(pin_1, OUTPUT);
        pinMode(pin_2, OUTPUT);
        pinMode(pin_pwm, OUTPUT);
    }

    void spin(int speed) {
        digitalWrite(pin_1, speed > 0 ? HIGH : LOW);
        digitalWrite(pin_2, speed < 0 ? HIGH : LOW);
        analogWrite(pin_pwm, abs(speed));
    }

private:
    const uint8_t pin_1, pin_2, pin_pwm;
};

I have left the buttons out of the class, because you have those blue “all motors” buttons that do not fit well with the notion that each button “belongs” to a particular motor.

Then, I would use some other abstraction for representing the buttons. Maybe from a debouncing library. If button bounces are not problematic in this application, then the “abstraction” would simply be:

// Is that button pressed?
static inline bool pressed(uint8_t pin) {
    return digitalRead(pin) == LOW;
}

Of course, as an abstraction, this does not provide a lot of value. It can be seen as a small bit of “syntactic sugar” that makes writing (and reading) the rest of the code easier.

With those in place, the program would look like:

// Pinout.
const uint8_t left_1   = 5;
const uint8_t left_2   = 4;
const uint8_t middle_1 = 3;
const uint8_t middle_2 = 2;
const uint8_t right_1  = 14;
const uint8_t right_2  = 15;
const uint8_t all_1    = 16;
const uint8_t all_2    = 17;
const int button_count = 8;
const uint8_t buttons[button_count] = {
    left_1, left_2, middle_1, middle_2, right_1, right_2, all_1, all_2
};

// Motors.
const int SPEED = 200;
Motor motor_left(11, 12, 13);
Motor motor_middle(10, 9, 18);
Motor motor_right(6, 7, 8);

void setup() {
    motor_left.begin();
    motor_middle.begin();
    motor_right.begin();
    for (int i = 0; i < button_count; i++)
        pinMode(buttons[i], INPUT_PULLUP);
    Serial.begin(9600);
}

void loop() {
    // Read the serial command, if any.
    int serial_command = Serial.read();  // is -1 if there is no input
    bool serial_1 = serial_command == '1',  // serial request for '1'
         serial_2 = serial_command == '2';  // serial request for '2'

    // Operate the motors.
    if      (pressed(left_1))   motor_left.spin(SPEED);
    else if (pressed(left_2))   motor_left.spin(-SPEED);
    else if (pressed(middle_1)) motor_middle.spin(SPEED);
    else if (pressed(middle_2)) motor_middle.spin(-SPEED);
    else if (pressed(right_1))  motor_right.spin(SPEED);
    else if (pressed(right_2))  motor_right.spin(-SPEED);
    else if (pressed(all_1) || serial_1) {
        motor_left.spin(SPEED);
        motor_middle.spin(SPEED);
        motor_right.spin(SPEED);
        if (serial_1) delay(2500);
    } else if (pressed(all_2) || serial_2) {
        motor_left.spin(-SPEED);
        motor_middle.spin(-SPEED);
        motor_right.spin(-SPEED);
        if (serial_2) delay(2500);
    } else {
        motor_left.spin(0);
        motor_middle.spin(0);
        motor_right.spin(0);
    }
}

¹ The struct keyword in C++ actually declares a class.

3
if(digitalRead(greenLeft1) == LOW) {
    motorSpeed_left = 200;
    spinMotor_left(motorSpeed_left);
    delay(200);
    motorSpeed_left = 0;
    spinMotor_left(motorSpeed_left);
}
else if(digitalRead(greenLeft2) == LOW) {
    motorSpeed_left = -200;
    spinMotor_left(motorSpeed_left);
    delay(200);
    motorSpeed_left = 0;
    spinMotor_left(motorSpeed_left);
}

Doesn't quite do what you want it to do.

Instead you want something like:

if(digitalRead(greenLeft1) == LOW) {
    motorSpeed_left = 200;
    spinMotor_left(motorSpeed_left);

} else if(digitalRead(greenLeft2) == LOW) {
    motorSpeed_left = -200;
    spinMotor_left(motorSpeed_left);
} else {
    motorSpeed_left = 0;
    spinMotor_left(motorSpeed_left);
}

However because you also want a master switch and control through serial. You can pull out the tests on each button to a variable and then make the master switch override those:

int motor_left = 0;
if(digitalRead(greenLeft1) == LOW) {
    motor_left = 1;
} else if(digitalRead(greenLeft2) == LOW) {
    motor_left = -1;
} else {
    motor_left = 0;
}

//...

if(digitalRead(blueAll1) == LOW){
    motor_left   = 1;
    motor_center = 1;
    motor_right  = 1;
} else if(digitalRead(greenLeft2) == LOW) {
    motor_left   = -1;
    motor_center = -1;
    motor_right  = -1;
} 

if(motor_left > 0) {
    motorSpeed_left = 200;
    spinMotor_left(motorSpeed_left);

} else if(motor_left < 0) {
    motorSpeed_left = -200;
    spinMotor_left(motorSpeed_left);
} else {
    motorSpeed_left = 0;
    spinMotor_left(motorSpeed_left);
}

Also the pin number is just and int so you can pull those out as a parameter as well and have only one copy of the spinMotor function.

void spinMotor(int motor_pin1, int motor_pin2, int motorSpeed)
{
  if (motorSpeed > 0)
  {
    digitalWrite(motor_pin1, HIGH);
    digitalWrite(motor_pin2, LOW);
  }
  else if (motorSpeed < 0)
  {
    digitalWrite(motor_pin1, LOW);
    digitalWrite(motor_pin2, HIGH);
  }
  else
  {
    digitalWrite(motor_pin1, LOW);
    digitalWrite(motor_pin2, LOW);
  }
  analogWrite(PWMA, abs(motorSpeed));
}

To reduce the number of globals you can group the state for each motor into a struct and initialize them in init():

struct motorState {
   int motor_pin1, motor_pin2;
   int pwm_pin;
   int button1, button2;
   int direction;
   int speed;
}

motorState motor_left;
motorState motor_center;
motorState motor_right;

then pass the globals by pointer or ref to the functions doing the input and output.

void readMotorButton(motorState *motor){
    if(digitalRead(motor->button1) == LOW) {
        motor->direction = 1;
        motor->speed = 200;
    } else if(digitalRead(motor->button2) == LOW) {
        motor->direction = -1;
        motor->speed = 200;
    } else {
        motor->direction = 0;
        motor->speed = 0;
    }
}

void spinMotor(motorState *motor)
{
  if (motor->direction > 0)
  {
    digitalWrite(motor->motor_pin1, HIGH);
    digitalWrite(motor->motor_pin2, LOW);
  }
  else if (motor->direction < 0)
  {
    digitalWrite(motor->motor_pin1, LOW);
    digitalWrite(motor->motor_pin2, HIGH);
  }
  else
  {
    digitalWrite(motor->motor_pin1, LOW);
    digitalWrite(motor->motor_pin2, LOW);
  }
  analogWrite(motor->pwm_pin, abs(motor->speed));
}
  • 1. If neither blueAll1 nor greenLeft2 are pressed, your else clause will prevent the other button from affecting the motors. 2. By motor_left > 1 you probably mean motor_left > 0. 3. The PWM pin should also be a parameter of spinMotor(). 4. readMotorButton() and spinMotor() look like they want to be methods of motorState. – Edgar Bonet Apr 25 '18 at 10:53
  • 1&2 yeah mistake; 3. I didn't realize that each motor had its own PWM pin; another argument in favor of grouping the motor states like that; 4. I'm not that much of a fan of creating member functions for simple stuff like this. YYMV though – ratchet freak Apr 25 '18 at 11:26
2

There is a code review group on here where you will get a more generic code review if you want.

First thing I noticed is there are no comments. You have written a paragraph explaining what the code does, but that should be in the code where you will be able to read it when you find the code 12 months down the line.

Rachet Freak has covered loads of good points, but one point I would disagree with is passing globals to functions using pointers. I would use references instead, the only reason for this is IF you mess up references can't be NULL, so you don't need to check within the function to see if they are NULL. Of course it will never happen, you never pass a NULL pointer to one of these functions, but as you code grows in size the chances of you doing that will increase and should you ever write a library, the chances of someone else passing a NULL pointer to your library is very high, and then you have got "a problem in your library".

Moving on from that if the parameter's value will not be modified during the life of a function pass it in as a const reference. The main reason for this is it explains to the reader that the value won't change in the function, its a simple way of documenting your intention without having to write a comment. So

void spinMotor(int motor_pin1, int motor_pin2, int motorSpeed){...}

would become

void spinMotor(const int& motor_pin1, const int& motor_pin2, const int& motorSpeed) {...}

Personally I would always add a constructor to structures. This way you know that every field has the correct value to start with. (I believe the Arduino compiler will initialise everything, but not all compilers are so kind.) Also it allows you to write neater code, i.e.

struct motorState {
   int motor_pin1;
   int motor_pin2;
   int button1;
   int button2;
   int direction;

  // Parameterised constructor can be called either:
  //    motorState mState;
  //    motorState mState2 = motorState (2, 3, 4, 5, -1);
  motorState (const int& pin1 = 5, const int& pin2 = 6, const int& b1 = 7, const int& b2 = 8, const int& dir = 1)
  : motor_pin1(pin1)
  , motor_pin2(pin2)
  , button1(b1)
  , button2(b2)
  , direction(dir)
{}

}

Finally, I have always been told to never put two declarations on the same line.

int motor_pin1, motor_pin2;

There is no difference between the above and this as far as the compiler is concerned.

int motor_pin1; 
int motor_pin2;

It is purely for readability you don't realise you are doing it, taking longer to read the first case than the second because your brain is more attuned to seeing a type, variable name and optional value.

int 
motor_pin1,
motor_pin2;

The above is legal, but what type in motor_pin2? It is just a convention that is widely used and makes code easier to read, even though it is harder to write, in the same way that code has to be indented by 4 spaces on here. If it isn't indented it has the same text, but is virtually unreadable.

Finally, if you find yourself copy and pasting lines, it should probably be a function or an array or both.

  • You don't want to pass an int as a reference unless the intention is for the function to change the original value. Ints are too small, and the overhead of passing a reference is larger than the overhead of copying the int. And if you do want the original value to be modified, your intention may be clearer with a pointer (though there is more potential for a crash). – piojo Apr 25 '18 at 14:52
  • 1
    I see what you mean but it may depend on the platform you are targeting. – Code Gorilla Apr 26 '18 at 10:43
  • Very good point. Though I don't think a reference could possibly have less overhead than a small primitive type. – piojo Apr 26 '18 at 17:27

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.