Clean Code Review: An NVIDIA Jetson Nano GPIO Wheel Encoder Message Publisher using ROS2

edit

Although I am not entirely new to ROS nor C++ I still struggle when it comes to making good design decisions with regards to single responsibility, dependency injection, and callbacks. Also worried about which objects to use in callbacks in terms of race conditions (which in this case is not really applicable but it's something I worry about getting wrong).

The below code example is very simple. There's a wheel encoder connected to the GPIO pin on an NVIDIA Jetson Nano board, and a ROS2 Node that publishes the encoder. I've tested this code on the Jetson Nano and it is functional.

I already tested this code on the Jetson Nano and it is fully functional.

The wheel_encoder_sensor.h file

#pragma once

#include <iostream>
#include <memory>
#include <functional>
#include <JetsonGPIO.h>

namespace xiaoduckie {
namespace sensors {

/*!
 * @brief This class manages the WheelEncoder sensor connected to the GPIO
 * pins in the NVIDIA Jetson Nano Board
 * @ingroup Sensors
*/
class WheelEncoderSensor {

  /*!
    * @brief ENUM that represents the direction of motion for the robot.
    * This value is updated when the controller makes a decision about
    * direction.
    * The value is used to update the global tick count on the encoder.
    * @ingroup Sensors
    */
  enum Direction {
    FORWARD = 1,
    BACKWARD = -1
  };

  /*!
    * @brief This class is used by the Jetson GPIO c++ library as a callback
    * for the event detect on a GPIO pin.
    * We cannot use a regular lambda bcause this Jetson GPIO library requires
    * the callbacks must be equaility-comparable.
    * More details: https://github.com/pjueon/JetsonGPIO/issues/55#issuecomment-1059888835
    * @ingroup Sensors
    */
  class EncoderCallback
  {
  public:
    /*!
      * @brief Default constructor.
      * @param name Reference to the name assigned to this callback.
      * @param ticks Pointer referring to total ticks in the WheelEncoderSensor.
      * @param callback The external callback passed to WheelEncoderSensor.
      * @param direction Pointer to the DIRECTION enum in WheelEncoderSensor.
      */
    EncoderCallback(
      const std::string& name,
      std::shared_ptr<int> ticks,
      std::function<void(int)> callback,
      std::shared_ptr<WheelEncoderSensor::Direction> direction)
      : name(name),
      callback_(std::move(callback)),
      ticks_(ticks),
      direction_(direction)
    {};

    /*!
      * @brief Default copy constructor
      */
    EncoderCallback(const EncoderCallback&) = default; // Copy-constructible

    /*!
      * @brief Callable executed by the GPIO library upon event detection.
      * Used to update the global tick count and execute the callback provided
      * to the Encoder through its constructor.
      * @param channel Reference to the GPIO pin passed in by the library.
      */
    void operator()(const std::string& channel) // Callable with one string type argument
    {
      std::cout << "A callback named " << name;
      std::cout << " called from channel " << channel << std::endl;
      *ticks_ += *direction_;
      callback_(*ticks_);
    }

    /*!
      * @brief equality operator used by GPIO library to remove callbacks.
      * @param other Reference to the object being compared.
      */
    bool operator==(const EncoderCallback& other) const // Equality-comparable
    {
        return name == other.name;
    }

    /*!
      * @brief in-equality operator used by GPIO library to remove callbacks.
      * @param other Reference to the object being compared.
      */
    bool operator!=(const EncoderCallback& other) const 
    {
        return !(*this == other);
    }

  private:
    std::string name;
    std::function<void(int)> callback_;
    std::shared_ptr<int> ticks_ = std::make_shared<int>(0);
    std::shared_ptr<WheelEncoderSensor::Direction> direction_ = std::make_shared<WheelEncoderSensor::Direction>();
  };

  public:
    /*!
      * @brief Default constructor.
      */
    WheelEncoderSensor() = default;

    /*!
      * @brief The Init function sets the GPIO ...