Incorporate CRSE Feedback

Question

Incorporate CRSE Feedback

eccentricOrange opened this issue 2 years ago · 0 comments

I recently submitted the code up to 5e25311 to the Code Review Stack Exchange: https://codereview.stackexchange.com/questions/284152/arduino-library-to-simplify-differential-drive-robots

This issue will tackle changes based on the two answers received up till the time of writing.

Based on Answer 1

#5
This would allow controlling both speed and direction with just two parameters.

  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));
  }

#6
Check the possibility of converting the speed parameters to physical units, such as a percentage instead of the 0-255 value enforced by the Arduino's analogWrite() function.
#7
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 naïve.
At the very least, the documentation should communicate accurately that it is an open-loop system, and that it is not a closed-loop system.
#8
The mathematics in the ForwardDDBot::write() method is done with 8-bit integers, and there is significant chance of overflow. So it would be better to use a float or a 16-bit integer instead.

Based on Answer 2

#9
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];
      }
  }
```

#10
Some values can be defaulted, for example:

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

#11
Some values like the number of pins (4 DIO pins, 2 PWM pins) might be better as constants, so they can be changed from one place.
This is the concept of a magic number.

#12
These are not necessary in the .cpp files.

  #ifndef DDBot_cpp
  #define DDBot_cpp

      ...
      
  #endif  // DDBot_cpp

#13
Each of DDBot and ForwardDDBot should be in their own .cpp and .h files. Inheritance is a good should be handled by including the base class header file in the derived class header file.
#14
This improves readability, and is a good practice.