7
\$\begingroup\$

I've written code for an Arduino library to abstract away some of the underlying logic in a particular way of moving robots. Code is posted after explanations.


I'm not assuming a high degree of familiarity here so, I'll include abstracts from the README that will allow you to understand what the code is trying to do, and how its structured. If needed, I've taken the following code from its public repository at commit 5e25311 and you can view a more detailed README there.

This library provides an interface to easily control a differential wheeled robot using an Arduino-compatible microcontroller.

The expectation is that you have an even number of motors, with one half on either side of a structure that looks like a rover or a car. You then control the direction the robot moves in by controlling which motors turn on and in which direction. For example, if the left motors spin forward and the right motors spin backwards, the robot will rotate clockwise.

Types of robots offered by the library

There are classes for two kinds of robots included in this library.

  • Standard Differential Drive Robot

    This is a normal differential drive robot, with motors on each side. It is what we described above: the direction is controlled by controlling which motors turn on and in which direction.

    The class for this is DDBot.

  • Forward-biased Differential Drive Robot

    The physical robot for this can be the same as a standard differential drive robot, but the control is different. In this case, the robot's motor's are always set to move forward. The direction is controlled by varying the speeds of the motors. In certain cases (like line followers), it is expected that this library leads to more less jerky motion of the robot.

    The class for this is ForwardDDBot.

Different files

File name and location Purpose
DDBot.h Header file for declaring classes and inheritance.
DDBot.cpp Source for function definitions.
examples/Square/Square.ino Example 1 of using the library.
examples/SpeedTuner/SpeedTuner.ino Example 2 of using the library.

DDBot.h

#include <Arduino.h>

#ifndef DDBot_h
#define DDBot_h

/* Differential Drive Bot
    This is a conventional **differential wheeled robot**, and direction is set by varying which motors spin
    and in which direction.

    Here, speed control of a motor simply controls how fast the robot moves. The expected use of this library
    just involves the user calling the method of the appropriate direction and/or speed in their main loop.
*/

class DDBot {
   private:
   public:

    // the sequence of pins is important, and is used throughout the library
    // the first two pins are for the left motor, and the second two are for the right motor
    // the first pin in each pair is for the forward direction, and the second is for the backward direction
    uint8_t directionPins[4];

    // when each motor has a dedicated PWM pin, it's possible to control the speed of each motor independently
    // in other cases, the speed of each motor is controlled by the same PWM pin
    // two declarations exist to accommodate both cases, and this pattern is used throughout the library
    uint8_t PWMPins[2] = {0, 0};
    uint8_t PWMPin = 0;

    // the constructors are responsible for setting the pin numbers from the arguments to the class properties
    DDBot();  // allow the user to directly set the arrays
    DDBot(uint8_t directionPins[4]);
    DDBot(uint8_t directionPins[4], uint8_t PWMPins[2]);
    DDBot(uint8_t directionPins[4], uint8_t PWMPin);

    void setPinModes();

    void setSpeed(uint8_t speed);
    void setSpeed(uint8_t leftSpeed, uint8_t rightSpeed);

    void writeDirections(bool leftForward, bool leftBackward, bool rightForward, bool rightBackward);
    void writeDirections(bool leftForward, bool leftBackward, bool rightForward, bool rightBackward, uint8_t speed);
    void writeDirections(bool leftForward, bool leftBackward, bool rightForward, bool rightBackward, uint8_t leftSpeed, uint8_t rightSpeed);

    void forward();
    void forward(uint8_t speed);
    void forward(uint8_t leftSpeed, uint8_t rightSpeed);

    void backward();
    void backward(uint8_t speed);
    void backward(uint8_t leftSpeed, uint8_t rightSpeed);

    void left();
    void left(uint8_t speed);
    void left(uint8_t leftSpeed, uint8_t rightSpeed);

    void right();
    void right(uint8_t speed);
    void right(uint8_t leftSpeed, uint8_t rightSpeed);

    void clockwise();
    void clockwise(uint8_t speed);
    void clockwise(uint8_t leftSpeed, uint8_t rightSpeed);

    void counterClockwise();
    void counterClockwise(uint8_t speed);
    void counterClockwise(uint8_t leftSpeed, uint8_t rightSpeed);

    // it doesn't make sense to specify a PWM variant of a stop command
    void stop();

    ~DDBot();
};


/* Forward Differential Drive Bot
    This is a **differential wheeled robot** with a forward bias, and direction is set by varying just the
    speed of each motor. So while the classic differential drive bot can use the same physical structure,
    the functionality is different. For example, a forward differential drive bot can't turn in place or
    move backwards.

    This class was built for line following robots, where the robot is biased towards moving forward, and
    just incrementally varying the speed provides smoother control than the classic differential drive bot.
    This is important to ensure that the robot doesn't overshoot and lose track of the line.

    Conventionally, the direction of a differential wheeled robot is set by turning motors on
    or off in a specific direction. In this setup, they are set to move forward perpetually, and
    direction control is achieved by varying the speed of each motor instead. This allows for
    smoother turns and will (hopefully) help the robot to stay on its line more reliably.

    The main loop should be structured to use feedback control. To use this library, the user should call
    the method of the appropriate direction when they want, and call the `write()` method once per loop.
    This will update the speed of each motor to come closer to the target speed. This is done to avoid
    sudden changes in speed. A delay might have to be added to the main loop to allow the changes to
    propagate.
*/

class ForwardDDBot : public DDBot {
   private:

    // this is the value used to "slow down" a motor
    // it is used when we want to turn, but don't want to set the speed of the other motor to 0
    // this will be calculated in the init() method by multiplying the maxPWM by the adjustment factor
    uint8_t _adjustedPWM;

    // "actual" values are written to a motor during a given call of the write() method
    // "target" values are the values that the user wants to write to a motor
    // in each call of the write() method, the actual values are adjusted to come closer to the target values
    uint8_t leftActualPWM, leftTargetPWM, rightActualPWM, rightTargetPWM;

   public:
    uint8_t maxPWM;    // maximum "speed" for each motor
    float adjustment;  // scaling factor used when slowing down a given motor


    // the constructors are responsible for setting the pin numbers from the arguments to the class properties
    ForwardDDBot();  // allow the user to directly set the arrays or the default PWM values
    ForwardDDBot(uint8_t maxPWM, float adjustment);
    ForwardDDBot(uint8_t directionPins[4], uint8_t PWMPins[2]);
    ForwardDDBot(uint8_t directionPins[4], uint8_t PWMPins[2], uint8_t maxPWM, float adjustment);

    void init();
    void calculateAdjustedPWM();

    void left();
    void centre();
    void right();
    void stop();

    // recall that rotations and moving backwards are not possible with this type of robot
    // so these methods are not implemented

    void write();

    ~ForwardDDBot();
};

#endif  // DDBot_h

DDBot.cpp

#include "DDBot.h"

#include <Arduino.h>

#ifndef DDBot_cpp
#define DDBot_cpp

DDBot::DDBot() {}
DDBot::~DDBot() {}

DDBot::DDBot(uint8_t directionPins[4]) {
    for (size_t i = 0; i < 4; i++) {
        this->directionPins[i] = directionPins[i];
    }
}

DDBot::DDBot(uint8_t directionPins[4], uint8_t PWMPins[2]) {
    for (size_t i = 0; i < 4; i++) {
        this->directionPins[i] = directionPins[i];
    }
    for (size_t i = 0; i < 2; i++) {
        this->PWMPins[i] = PWMPins[i];
    }
}

DDBot::DDBot(uint8_t directionPins[4], uint8_t PWMPin) {
    for (size_t i = 0; i < 4; i++) {
        this->directionPins[i] = directionPins[i];
    }
    this->PWMPin = PWMPin;
}

void DDBot::setPinModes() {
    for (size_t i = 0; i < 4; i++) {
        pinMode(directionPins[i], OUTPUT);
    }

    // do not set pinMode(s) for PWM pin(s) if they are not set (i.e. if they are 0)
    if (PWMPins[0] != 0 && PWMPins[1] != 0) {
        for (size_t i = 0; i < 2; i++) {
            pinMode(PWMPins[i], OUTPUT);
        }
    }

    if (PWMPin != 0) {
        pinMode(PWMPin, OUTPUT);
    }
}

void DDBot::setSpeed(uint8_t speed) {
    // do not set speed for PWM pin(s) if they are not set (i.e. if they are 0)
    if (PWMPins[0] != 0 && PWMPins[1] != 0) {
        analogWrite(PWMPins[0], speed);
        analogWrite(PWMPins[1], speed);
    }

    if (PWMPin != 0) {
        analogWrite(PWMPin, speed);
    }
}

void DDBot::setSpeed(uint8_t leftSpeed, uint8_t rightSpeed) {
    analogWrite(PWMPins[0], leftSpeed);
    analogWrite(PWMPins[1], rightSpeed);
}

void DDBot::writeDirections(bool leftForward, bool leftBackward, bool rightForward, bool rightBackward) {
    digitalWrite(directionPins[0], leftForward);
    digitalWrite(directionPins[1], leftBackward);
    digitalWrite(directionPins[2], rightForward);
    digitalWrite(directionPins[3], rightBackward);
}

void DDBot::writeDirections(bool leftForward, bool leftBackward, bool rightForward, bool rightBackward, uint8_t speed) {
    digitalWrite(directionPins[0], leftForward);
    digitalWrite(directionPins[1], leftBackward);
    digitalWrite(directionPins[2], rightForward);
    digitalWrite(directionPins[3], rightBackward);
    setSpeed(speed);
}

void DDBot::writeDirections(bool leftForward, bool leftBackward, bool rightForward, bool rightBackward, uint8_t leftSpeed, uint8_t rightSpeed) {
    digitalWrite(directionPins[0], leftForward);
    digitalWrite(directionPins[1], leftBackward);
    digitalWrite(directionPins[2], rightForward);
    digitalWrite(directionPins[3], rightBackward);
    setSpeed(leftSpeed, rightSpeed);
}

// the following methods set the direction of the robot by controlling which
// direction motors are going to turn and in which direction
// you can figure out these methods by trying to imagine the robot, maybe
// using a model or a drawing
void DDBot::forward() {
    writeDirections(HIGH, LOW, HIGH, LOW);
}

void DDBot::forward(uint8_t speed) {
    writeDirections(HIGH, LOW, HIGH, LOW, speed);
}

void DDBot::forward(uint8_t leftSpeed, uint8_t rightSpeed) {
    writeDirections(HIGH, LOW, HIGH, LOW, leftSpeed, rightSpeed);
}

void DDBot::backward() {
    writeDirections(LOW, HIGH, LOW, HIGH);
}

void DDBot::backward(uint8_t speed) {
    writeDirections(LOW, HIGH, LOW, HIGH, speed);
}

void DDBot::backward(uint8_t leftSpeed, uint8_t rightSpeed) {
    writeDirections(LOW, HIGH, LOW, HIGH, leftSpeed, rightSpeed);
}

void DDBot::left() {
    writeDirections(LOW, HIGH, LOW, LOW);
}

void DDBot::left(uint8_t speed) {
    writeDirections(LOW, HIGH, LOW, LOW, speed);
}

void DDBot::left(uint8_t leftSpeed, uint8_t rightSpeed) {
    writeDirections(LOW, HIGH, LOW, LOW, leftSpeed, rightSpeed);
}

void DDBot::right() {
    writeDirections(LOW, LOW, LOW, HIGH);
}

void DDBot::right(uint8_t speed) {
    writeDirections(LOW, LOW, LOW, HIGH, speed);
}

void DDBot::right(uint8_t leftSpeed, uint8_t rightSpeed) {
    writeDirections(LOW, LOW, LOW, HIGH, leftSpeed, rightSpeed);
}

void DDBot::clockwise() {
    writeDirections(LOW, HIGH, HIGH, LOW);
}

void DDBot::clockwise(uint8_t speed) {
    writeDirections(LOW, HIGH, HIGH, LOW, speed);
}

void DDBot::clockwise(uint8_t leftSpeed, uint8_t rightSpeed) {
    writeDirections(LOW, HIGH, HIGH, LOW, leftSpeed, rightSpeed);
}

void DDBot::counterClockwise() {
    writeDirections(HIGH, LOW, LOW, HIGH);
}

void DDBot::counterClockwise(uint8_t speed) {
    writeDirections(HIGH, LOW, LOW, HIGH, speed);
}

void DDBot::counterClockwise(uint8_t leftSpeed, uint8_t rightSpeed) {
    writeDirections(HIGH, LOW, LOW, HIGH, leftSpeed, rightSpeed);
}

void DDBot::stop() {
    writeDirections(LOW, LOW, LOW, LOW);
}

ForwardDDBot::ForwardDDBot() {}
ForwardDDBot::~ForwardDDBot() {}

ForwardDDBot::ForwardDDBot(uint8_t maxPWM, float adjustment) {
    this->maxPWM = maxPWM;
    this->adjustment = adjustment;
}

ForwardDDBot::ForwardDDBot(uint8_t directionPins[4], uint8_t PWMPins[2]) {
    for (size_t i = 0; i < 4; i++) {
        this->directionPins[i] = directionPins[i];
    }
    for (size_t i = 0; i < 2; i++) {
        this->PWMPins[i] = PWMPins[i];
    }
}
ForwardDDBot::ForwardDDBot(uint8_t directionPins[4], uint8_t PWMPins[2], uint8_t maxPWM, float adjustment) {
    for (size_t i = 0; i < 4; i++) {
        this->directionPins[i] = directionPins[i];
    }
    for (size_t i = 0; i < 2; i++) {
        this->PWMPins[i] = PWMPins[i];
    }

    this->maxPWM = maxPWM;
    this->adjustment = adjustment;
}

void ForwardDDBot::calculateAdjustedPWM() {
    // this is the value used to "slow down" a motor
    // it is used when we want to turn, but don't want to set the speed of the other motor to 0
    _adjustedPWM = this->maxPWM * this->adjustment;
}

void ForwardDDBot::init() {
    setPinModes();
    calculateAdjustedPWM();

    this->leftActualPWM = maxPWM;
    this->rightActualPWM = maxPWM;

    // set the robot to perpetually move forward
    forward(leftActualPWM, rightActualPWM);
}

// the following methods set the direction of the robot by controlling which
// motor spins faster and which one slower
// the logic here is the same as in the conventional differential drive robot
// and you can figure it out the same way
void ForwardDDBot::left() {
    leftTargetPWM = _adjustedPWM;
    rightTargetPWM = maxPWM;
}

void ForwardDDBot::right() {
    leftTargetPWM = maxPWM;
    rightTargetPWM = _adjustedPWM;
}

void ForwardDDBot::centre() {
    leftTargetPWM = maxPWM;
    rightTargetPWM = maxPWM;
}

void ForwardDDBot::stop() {
    leftTargetPWM = 0;
    rightTargetPWM = 0;
}

void ForwardDDBot::write() {
    // update "actual" PWM values so that they get closer to the "target" values
    // this will eventually get the values to equal, as exponential decay (but
    // with integer division)
    leftActualPWM = (leftTargetPWM + leftActualPWM) / 2;
    rightActualPWM = (rightTargetPWM + rightActualPWM) / 2;

    // write the actual PWM values to the motor driver
    analogWrite(PWMPins[0], leftActualPWM);
    analogWrite(PWMPins[1], rightActualPWM);
}

#endif  // DDBot_cpp

examples/Square/Square.ino

/* Square
    This example shows how to use the DDBot library to make the robot move in a square.

    The robot will move forward for 2 seconds, then turn right for 1 second, then move forward for 2 seconds, then turn right for 1 second, and so on.

    The circuit:
    * Pin 2 to left motor forward
    * Pin 3 to left motor backward
    * Pin 4 to right motor forward
    * Pin 5 to right motor backward
    * Pin 10 to left motor speed
    * Pin 11 to right motor speed
*/

#include <Arduino.h>
#include <DDBot.h>

// define the pins used by the motors
uint8_t directionPins[4] = {2, 3, 4, 5};
uint8_t speedPins[2] = {10, 11};

// create an instance of the DDBot class
// this allows you to potentially control multiple robots at once using multiple instances
DDBot bot(directionPins, speedPins);

void setup() {
    // set the pin modes for the motor DIO pins
    bot.setPinModes();
}

void loop() {
    // move forward with full speed for 2 seconds
    bot.forward(255);
    delay(2000);

    // turn right with full speed for 1 second
    bot.right(255);
    delay(1000);
}

examples/SpeedTuner/SpeedTuner.ino

/* SpeedTuner
    This example shows how to use the DDBot library to figure out the right values for tuning
    PWM parameters for use with the Forward-biased Differential Drive robot.

    The robot will start with 0 adjustment. It will then increase the adjustment by
    0.05, attempt to forward for 3 seconds, and then increase the adjustment by 0.05 again.
    This process repeats until the adjustment is 1.0, at which point the robot will stop.

    It will then repeat the process while going backwards, increasing the adjustment from 0 to
    1.0 in 0.05 increments.

    At each stage, it will output the adjustment value and the adjusted PWM to the Serial monitor,
    so that you can determine when it has sufficient PWM to accelerate.

    The circuit:
    * Pin 2 to left motor forward
    * Pin 3 to left motor backward
    * Pin 4 to right motor forward
    * Pin 5 to right motor backward
    * Pin 10 to left motor speed
    * Pin 11 to right motor speed
*/

#include <Arduino.h>
#include <DDBot.h>

// define the pins used by the motors
uint8_t directionPins[4] = {2, 3, 4, 5};
uint8_t speedPins[2] = {10, 11};

// create an instance of the DDBot class
// this allows you to potentially control multiple robots at once using multiple instances
ForwardDDBot bot(directionPins, speedPins);

void setup() {
    // use the maximum possible PWM value
    bot.maxPWM = 255;

    // set the pin modes for the motor DIO pins
    bot.init();

    // initialize the adjustment value
    bot.adjustment = 0.0;

    // open the Serial connection
    Serial.begin(115200);

    bot.forward();

    // 1 / 0.05 per increment = 20 increments, so we have 20 iterations
    for (int counter = 0; counter < 20; counter++) {
        bot.adjustment += 0.05;
        bot.calculateAdjustedPWM();

        Serial.print("[Forward] Adjustment: ");
        Serial.print(bot.adjustment);
        Serial.print(", Adjusted PWM: ");
        Serial.println(bot.maxPWM * bot.adjustment);

        // we need to update the PWM values to allow the feedback loop to work
        // we do this by calling the write() method 300 times, with a 10 ms delay between each call,
        // which is a total of 3 seconds
        for (int i = 0; i < 300; i++) {
            delay(10);
            bot.write();
        }
    }

    // stop the robot for 5 seconds
    bot.stop();
    delay(5000);

    // reset the adjustment value
    bot.adjustment = 0.0;

    bot.backward();

    // again, we have 20 iterations, for a total of 1.0 adjustment over 3 seconds
    for (int counter = 0; counter < 20; counter++) {
        bot.adjustment += 0.05;
        bot.calculateAdjustedPWM();

        Serial.print("[Backward] Adjustment: ");
        Serial.print(bot.adjustment);
        Serial.print(", Adjusted PWM: ");
        Serial.println(bot.maxPWM * bot.adjustment);

        // again, we need to update the PWM values to allow the feedback loop to work
        // so 300 iterations * 10 ms per iteration = 3 seconds
        for (int i = 0; i < 300; i++) {
            delay(10);
            bot.write();
        }
    }

    bot.stop();
}

void loop() {}
```
\$\endgroup\$
3
  • \$\begingroup\$ Is this developed with a specific board in mind, or a more general purpose library? \$\endgroup\$
    – Linny
    Mar 26 at 11:01
  • \$\begingroup\$ @Linny it is not designed with any board in mind. It is also not expecting any particular motor driver, batteries, or motors, but it is expecting a particular structure for robots that's very common in beginner courses or projects. \$\endgroup\$ Mar 26 at 13:23
  • \$\begingroup\$ For anyone interested, much of the feedback given below is now merged into the codebase. The entire process was tracked in issue #4 on the DDBot GitHub repo. \$\endgroup\$ Mar 30 at 11:34

2 Answers 2

7
\$\begingroup\$

It's great that you have a library that works for you, but now you should think of redesigning it. I see that class DDBot is implementing the low-level functionality (controlling pins and PWM values), and class ForwardDDBot provides a more high-level interface that also takes care of scaling values and smoothly changing motor speeds. If that is the case, then I see some issues:

Keep DDBot as simple as possible

There are so many functions in DDBot, but do you really need them all? If this is supposed to be for low-level control of the motors, I think you only need one function that sets the speed of the left and right motor. By taking the parameters as signed integers, you avoid having to pass the directions:

void DDBot::setSpeed(int leftSpeed, int rightSpeed) {
    digitalWrite(directionPins[0], left > 0);
    digitalWrite(directionPins[1], left < 0);
    digitalWrite(directionPins[2], right > 0);
    digitalWrite(directionPins[3], right < 0);

    analogWrite(PWMPins[0], abs(leftSpeed));
    analogWrite(PWMPins[1], abs(rightSpeed));
}

You can do any move you want with just this function.

Think about physical units

If I would ask you "how fast can your robot go?", would your answer be "255"? No, you would say something like "X meters per second". For a higher level interface, it would be nice to provide such a physical unit, and have your library convert it to a PWM setting. The question then is, what does the PWM value control? Is it the rotation speed of the motor? In that case, you could consider using "revolutions per second", or if you know the diameter of the wheels, "meters per second" as the unit for parameters passed to the high level interface.

It could also be that the PWM value doesn't control the speed, but rather the torque. Torque is measured in newton meters, and given the mass of your robot, you could calculate the acceleration a given torque would provide. Once the robot reaches a certain speed, the torque is canceled out by friction, and the robot will then stay at that speed. This could also be measured or calculated to some degree, so that you could provide an interface where you provide speed in meters per second, and it will then convert that to the right PWM value to reach that speed.

There is no feedback control

You mention "feedback control" in your comments, however what you have implemented does not rely on feedback at all, instead it's an open-loop controller to smooth the transition from one speed to another. This will indeed help smooth the motion of the robot, but the way you implemented it is very naive.

First, consider that the smoothing depends entirely on how often write() is called. But what should the delay between calls to write() be? If you call it too often, the robot will move jerky again. If you don't call it often enough, the robot will take a long time to reach the desired speed. You could actually check in write() how long it was since the last time it was called, and take the actual delay into account. That would remove the responsibility of calling write() at exactly the right interval from the caller into your library.

But then consider that the first call to write(), you take a large step, and subsequent calls make smaller and smaller steps. So it can still make a jerky movement when you make a large speed change. A smoother way to change speeds is to just add or subtract one from the actual PWM value in each call to write(), until you reach the desired value.

Note that jerk is a mathematically well-defined concept. Since it is the derivative of the acceleration, it's easy to see that you should keep changes in acceleration as low as possible to minimize jerk.

Be careful when doing math with integers

Consider this line of code:

leftActualPWM = (leftTargetPWM + leftActualPWM) / 2;

Depending on the type of the variables, the addition could overflow before the division is performed, which would result in an unexpected result, possibly leading to badly behaving motor speed.

While it might actually be safe here due to integer promotion rules, you could explicitly convert the values to a type that you know is safe for the given operation:

leftActualPWM = (static_cast<uint16_t>(leftTargetPWM) + leftActualPWM) / 2;

Or if possible, as Toby Speight suggested, use C++20's std::midpoint():

leftActualPWM = std::midpoint(leftTargetPWM, leftActualPWM);

Alternatively, if the CPU in your Arduino has hardware support for floating point operations, I would rather store most variables as float, and only convert to uint8_t PWM values right before calling analogWrite().

\$\endgroup\$
9
  • \$\begingroup\$ Great feedback. I have some follow-up questions, and this might take more than one comment. 1 Removing the direction functions removes the essence of the library. Then the user has to think about which pins to set and in which way any time they want to set the direction of the robot. 2 That structure assumes that every user has speed control; the target audience (beginners) often don't but there is still a large number of users who do and I want to cater to both. \$\endgroup\$ Mar 26 at 13:29
  • \$\begingroup\$ 3 Physical units are difficult because they depend greatly on the users' motors, batteries, battery level, tire size etc. Most beginners are unlikely to understand how to measure these parameters and input them into the library, and there is not a standard I can assume. Moreover, many use something like a 9V battery that discharges significantly quickly. That said, I could potentially use something like percentage, but I wanted to stick to the standard system. Advise if you'd still suggest changing this. \$\endgroup\$ Mar 26 at 13:31
  • 1
    \$\begingroup\$ 1) No, my example setSpeed() function takes care of setting the direction pins for you, so the caller does not need to know about them. 2) You can have a high level class, like ForwardDDBot, hide that if you want. 3) Doing a percentage might be better than a raw 8 bit value. What if you change to 16-bit PWM for example? If your interface abstracts it away, the caller doesn't have to worry about it. 6) float is supported in the language, but not all Arduino's handle them efficiently. Especially the 8-bit CPUs will spend a lot of cycles on floating point operations. I hope this helps! \$\endgroup\$
    – G. Sliepen
    Mar 26 at 21:18
  • 1
    \$\begingroup\$ Well, setting speed for the individual motors is quite low-level. If you want something higher-level, then I suggest a setSpeed() that takes both linear speed (forward/backward) and rotational speed as parameters, again as signed values. This can be quite easily converted into speeds for the left and right motors. But in any case, I would avoid having so many different ways of setting the speed in one class as you did in DDBot. \$\endgroup\$
    – G. Sliepen
    Mar 29 at 21:19
  • 1
    \$\begingroup\$ std::uint8_t promotes to int for addition, so cannot overflow. That said, I still recommend using std::midpoint() to save the reviewer having to spot that. \$\endgroup\$ Apr 17 at 9:40
7
\$\begingroup\$

General Observations

The point of a library is to provide abstraction, reducing the amount of work that the user needs to do to accomplish what they want to do. This library does not provide any abstraction, the user needs to know the details to do the work.

The user interface is overly complicated.

Since no memory is allocated in any of the constructors you don't really need a specific destructor, in class declaration in DDBot.h you can use ~DDBot() = default;

The Use of this in C++

Generally the use of the this pointer is frowned upon in C++, there is really no reason to use it. In the DDBot constructors if the input parameter name was different from the field the values were being assigned to then the this pointer would not be necessary.

DDBot::DDBot(uint8_t directionPinsIn[4]) {
    for (size_t i = 0; i < 4; i++) {
        directionPins[i] = directionPinsIn[i];
    }
}

Too Many Constructors

There is really no reason to have 4 constructors for DDBot. Don't allow the user to directly manipulate the direction pins and you really only need 2 constructors because you can default the PWMPin value. This would allow a call with or without the PWMPin value included.

In DDBot.h:

    DDBot(uint8_t directionPins[4], uint8_t PWMPins[2]);
    DDBot(uint8_t directionPins[4], uint8_t PWMPin = 0);

And in DDBot.cpp:

DDBot::DDBot(uint8_t directionPinsIn[4], uint8_t PWMPinsIn[2]) {
    for (size_t i = 0; i < 4; i++) {
        directionPins[i] = directionPinsIn[i];
    }
    for (size_t i = 0; i < 2; i++) {
        PWMPins[i] = PWMPinsIn[i];
    }
}

DDBot::DDBot(uint8_t directionPinsIn[4], uint8_t PWMPinIn) {
    for (size_t i = 0; i < 4; i++) {
        directionPins[i] = directionPinsIn[i];
    }
    PWMPin = PWMPinIn;
}

Magic Numbers

There are Magic Numbers (4 (the number of direction pins) and 2 (the number of PWM Pins)), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Include Guards

It is nice to see include guards in the files, but it is very unusual for a C++ source file to have include guards. Why was this necessary?

#ifndef DDBot_cpp
#define DDBot_cpp

    ...

#endif  // DDBot_cpp

One Class per Header / Source File Pair

There are 2 classes defined in the header file DDBot.h, ForwardDDBot and DDBot, there are also 2 classes defined in the source file DDBot.cpp. Generally in C++ it is preferred to have only one class per header file and source file. Classes that inherit from base classes include the header file for the base class.

The Width of the Comments

The human eye can loose track of what line it should be on if the lines are too wide, try to keep line length under 80 characters and definitely under 100 characters.

\$\endgroup\$
7
  • \$\begingroup\$ 1 The include guards come from Arduino's tutorials: docs.arduino.cc/learn/contributions/… 2 How would two constructors allow the PWMPins array to be optional as well? 3 You say that this library is complicated. I should have been more transparent: Arduino is cultured around learning rather than use in production. So we expect users to first learn the technology then use an abstraction. That said, how would you suggest simplifying it? Thanks a lot! \$\endgroup\$ Mar 25 at 3:39
  • 1
    \$\begingroup\$ The width issue isn't exclusive to comments. For example writeDirections() longest arg list is weighing in at 142 columns. Fortunately there's a style to fix that. Horizontal scroll is evil. But so is word wrapped code. Make code look like it's marching in formation. \$\endgroup\$ Mar 25 at 14:11
  • \$\begingroup\$ @candied_orange Take a good look at the link you posted, there are no include guards in the Morse.cpp file. For writeDirections() you can line break after commas in the argument list. My review is based on extensive software development experience. \$\endgroup\$
    – pacmaninbw
    Mar 25 at 14:24
  • \$\begingroup\$ What Morse.cpp file? Are you talking about this? The map init code there seems to agree with what I said in the link I posted (and answer I wrote). What do include guards have to do with any of this? All I'm saying is you have a point about width. But comments aren't the only width problem here. \$\endgroup\$ Mar 25 at 14:52
  • 1
    \$\begingroup\$ Oh I see. All us orange people look the same right? ; ) \$\endgroup\$ Mar 26 at 3:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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