Add -Wconversion and -Wsign-conversion and fix resulting issues

Signed-off-by: Javier Balloffet <javier.balloffet@gmail.com>

Author: jballoffet

andino_firmware/include/andino/app/app.h

@@ -29,7 +29,14 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #pragma once
 
+#if defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wconversion"
+#endif
 #include <Adafruit_BNO055.h>
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
 
 #include "andino/app/constants.h"
 #include "andino/app/pid.h"

andino_firmware/include/andino/app/hw.h

@@ -29,42 +29,44 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #pragma once
 
+#include <stdint.h>
+
 namespace andino {
 
 /// @brief Hardware configuration.
 struct Hw {
   /// @brief Left encoder channel A pin. Connected to PD2 (digital pin 2).
-  static constexpr int kLeftEncoderChannelAGpioPin{2};
+  static constexpr uint8_t kLeftEncoderChannelAGpioPin{2};
   /// @brief Left encoder channel B pin. Connected to PD3 (digital pin 3).
-  static constexpr int kLeftEncoderChannelBGpioPin{3};
+  static constexpr uint8_t kLeftEncoderChannelBGpioPin{3};
 
   /// @brief Right encoder channel A pin. Connected to PC2 (digital pin 16, analog pin A2).
-  static constexpr int kRightEncoderChannelAGpioPin{16};
+  static constexpr uint8_t kRightEncoderChannelAGpioPin{16};
   /// @brief Right encoder channel B pin. Connected to PC3 (digital pin 17, analog pin A3).
-  static constexpr int kRightEncoderChannelBGpioPin{17};
+  static constexpr uint8_t kRightEncoderChannelBGpioPin{17};
 
   /// @brief Left motor driver backward pin. Connected to PD6 (digital pin 6).
-  static constexpr int kLeftMotorBackwardGpioPin{6};
+  static constexpr uint8_t kLeftMotorBackwardGpioPin{6};
   /// @brief Left motor driver forward pin. Connected to PB2 (digital pin 10).
-  static constexpr int kLeftMotorForwardGpioPin{10};
+  static constexpr uint8_t kLeftMotorForwardGpioPin{10};
   /// @brief Left motor driver enable pin. Connected to PB5 (digital pin 13).
   /// @note The enable input of the L298N motor driver may be directly jumped to 5V if the board has
   /// a jumper to do so.
-  static constexpr int kLeftMotorEnableGpioPin{13};
+  static constexpr uint8_t kLeftMotorEnableGpioPin{13};
 
   /// @brief Right motor driver backward pin. Connected to PD5 (digital pin 5).
-  static constexpr int kRightMotorBackwardGpioPin{5};
+  static constexpr uint8_t kRightMotorBackwardGpioPin{5};
   /// @brief Right motor driver forward pin. Connected to PB1 (digital pin 9).
-  static constexpr int kRightMotorForwardGpioPin{9};
+  static constexpr uint8_t kRightMotorForwardGpioPin{9};
   /// @brief Right motor driver enable pin. Connected to PB4 (digital pin 12).
   /// @note The enable input of the L298N motor driver may be directly jumped to 5V if the board has
   /// a jumper to do so.
-  static constexpr int kRightMotorEnableGpioPin{12};
+  static constexpr uint8_t kRightMotorEnableGpioPin{12};
 
   /// @brief IMU sensor I2C SCL pin. Connected to PC5 (digital pin 19, analog pin A5).
-  static constexpr int kImuI2cSclPin{19};
+  static constexpr uint8_t kImuI2cSclPin{19};
   /// @brief IMU sensor I2C SDA pin. Connected to PC4 (digital pin 18, analog pin A4).
-  static constexpr int kImuI2cSdaPin{18};
+  static constexpr uint8_t kImuI2cSdaPin{18};
 };
 
 }  // namespace andino

andino_firmware/include/andino/app/pid.h

@@ -49,7 +49,7 @@ class Pid {
   /// @brief Resets the PID controller.
   ///
   /// @param encoder_count Current encoder value.
-  void reset(int encoder_count);
+  void reset(long encoder_count);
 
   /// @brief Returns if the PID controller is enabled or not.
   bool enabled();
@@ -64,7 +64,7 @@ class Pid {
   ///
   /// @param encoder_count Current encoder value.
   /// @param computed_output Computed output value.
-  void compute(int encoder_count, int& computed_output);
+  void compute(long encoder_count, int& computed_output);
 
   /// @brief Sets the setpoint.
   ///

andino_firmware/include/andino/bsp/digital_out_arduino.h

@@ -29,6 +29,8 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #pragma once
 
+#include <stdint.h>
+
 #include "andino/hal/digital_out.h"
 
 namespace andino {
@@ -39,16 +41,16 @@ class DigitalOutArduino : public DigitalOut {
   /// @brief Constructs a DigitalOutArduino using the specified GPIO pin.
   ///
   /// @param gpio_pin GPIO pin.
-  explicit DigitalOutArduino(const int gpio_pin) : gpio_pin_(gpio_pin) {
+  explicit DigitalOutArduino(const uint8_t gpio_pin) : gpio_pin_(gpio_pin) {
   }
 
   void begin() const override;
 
-  void write(int value) const override;
+  void write(bool value) const override;
 
  private:
   /// GPIO pin.
-  const int gpio_pin_;
+  const uint8_t gpio_pin_;
 };
 
 }  // namespace andino

andino_firmware/include/andino/bsp/interrupt_in_arduino.h

@@ -29,6 +29,8 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #pragma once
 
+#include <stdint.h>
+
 #include "andino/hal/interrupt_in.h"
 
 namespace andino {
@@ -39,7 +41,7 @@ class InterruptInArduino : public InterruptIn {
   /// @brief Constructs a InterruptInArduino using the specified GPIO pin.
   ///
   /// @param gpio_pin GPIO pin.
-  explicit InterruptInArduino(const int gpio_pin) : gpio_pin_(gpio_pin) {
+  explicit InterruptInArduino(const uint8_t gpio_pin) : gpio_pin_(gpio_pin) {
   }
 
   void begin() const override;
@@ -50,7 +52,7 @@ class InterruptInArduino : public InterruptIn {
 
  private:
   /// GPIO pin.
-  const int gpio_pin_;
+  const uint8_t gpio_pin_;
 };
 
 }  // namespace andino

andino_firmware/include/andino/bsp/pwm_out_arduino.h

@@ -29,6 +29,8 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #pragma once
 
+#include <stdint.h>
+
 #include "andino/hal/pwm_out.h"
 
 namespace andino {
@@ -39,7 +41,7 @@ class PwmOutArduino : public PwmOut {
   /// @brief Constructs a PwmOutArduino using the specified GPIO pin.
   ///
   /// @param gpio_pin GPIO pin.
-  explicit PwmOutArduino(const int gpio_pin) : gpio_pin_(gpio_pin) {
+  explicit PwmOutArduino(const uint8_t gpio_pin) : gpio_pin_(gpio_pin) {
   }
 
   void begin() const override;
@@ -48,7 +50,7 @@ class PwmOutArduino : public PwmOut {
 
  private:
   /// GPIO pin.
-  const int gpio_pin_;
+  const uint8_t gpio_pin_;
 };
 
 }  // namespace andino

andino_firmware/include/andino/hal/digital_out.h

@@ -40,10 +40,10 @@ class DigitalOut {
   /// @brief Initializes the digital output.
   virtual void begin() const = 0;
 
-  /// @brief Sets the digital output value (0 or 1).
+  /// @brief Sets the digital output value.
   ///
-  /// @param value Digital output value.
-  virtual void write(int value) const = 0;
+  /// @param value True to set the output high, false to set it low.
+  virtual void write(bool value) const = 0;
 };
 
 }  // namespace andino

andino_firmware/platformio.ini

@@ -23,6 +23,8 @@ build_src_flags =
   -Wcast-qual
   -Wuseless-cast
   -Wnull-dereference
+  -Wconversion
+  -Wsign-conversion
   -Werror
 check_tool = cppcheck
 check_skip_packages = yes

andino_firmware/src/app/app.cpp

@@ -119,7 +119,7 @@ void App::loop() {
   shell_.process_input();
 
   // Compute PID output at the configured rate.
-  if ((clock_.millis() - last_pid_computation_) > Constants::kPidPeriod) {
+  if (static_cast<double>(clock_.millis() - last_pid_computation_) > Constants::kPidPeriod) {
     last_pid_computation_ = clock_.millis();
     adjust_motors_speed();
   }
@@ -145,7 +145,7 @@ void App::cmd_read_analog_gpio_cb(void*, int argc, char** argv) {
     return;
   }
 
-  const int pin = atoi(argv[1]);
+  const uint8_t pin = static_cast<uint8_t>(atoi(argv[1]));
   Serial.println(analogRead(pin));
 }
 
@@ -154,7 +154,7 @@ void App::cmd_read_digital_gpio_cb(void*, int argc, char** argv) {
     return;
   }
 
-  const int pin = atoi(argv[1]);
+  const uint8_t pin = static_cast<uint8_t>(atoi(argv[1]));
   Serial.println(digitalRead(pin));
 }
 

andino_firmware/src/app/pid.cpp

@@ -71,7 +71,7 @@ namespace andino {
 //  - http://brettbeauregard.com/blog/2011/04/improving-the-beginner%E2%80%99s-pid-derivative-kick/
 //  - http://brettbeauregard.com/blog/2011/04/improving-the-beginner%E2%80%99s-pid-tuning-changes/
 
-void Pid::reset(int encoder_count) {
+void Pid::reset(long encoder_count) {
   // Since we can assume that the PID is only turned on when going from stop to moving, we can init
   // everything on zero.
   setpoint_ = 0;
@@ -96,7 +96,7 @@ void Pid::disable() {
   enabled_ = false;
 }
 
-void Pid::compute(int encoder_count, int& computed_output) {
+void Pid::compute(long encoder_count, int& computed_output) {
   if (!enabled_) {
     // Reset PID once to prevent startup spikes.
     if (last_input_ != 0) {
@@ -105,7 +105,9 @@ void Pid::compute(int encoder_count, int& computed_output) {
     return;
   }
 
-  int input = encoder_count - last_encoder_count_;
+  // The tick count itself is unbounded, but the delta between two consecutive PID cycles is
+  // always small (a few dozen ticks at most), so it safely fits back into an int.
+  int input = static_cast<int>(encoder_count - last_encoder_count_);
   long error = setpoint_ - input;
 
   long output = (kp_ * error - kd_ * (input - last_input_) + integral_term_) / ko_;
@@ -117,11 +119,14 @@ void Pid::compute(int encoder_count, int& computed_output) {
   } else if (output <= output_min_) {
     output = output_min_;
   } else {
-    integral_term_ += ki_ * error;
+    // ki_ * error safely fits into an int: it is only accumulated while output (which is
+    // derived from the same magnitudes) is within the int-sized output_min_/output_max_ bounds.
+    integral_term_ += static_cast<int>(ki_ * error);
   }
 
-  // Set the computed output accordingly.
-  computed_output = output;
+  // output is clamped to output_min_/output_max_ above, both of which are int, so this narrowing
+  // is safe.
+  computed_output = static_cast<int>(output);
 
   // Store obtained values.
   last_encoder_count_ = encoder_count;