Skip to main content
Code Review

Return to Answer

added GPIO_Outpin implementation
Source Link
Edward
Edward
  • 67.2k
  • 4
  • 120
  • 284

Note that digit and value would both need range checking in real code. This is just a sample.

Note that digit and value would both need range checking in real code. This is just a sample.

added GPIO_Outpin implementation
Source Link
Edward
Edward
  • 67.2k
  • 4
  • 120
  • 284

Here's how that might look:

class GPIO_Outpin {
public:
    GPIO_Outpin(gpio_num_t pin) : pin{pin} {
      gpio_pad_select_gpio(pin);
      gpio_set_direction(pin, GPIO_MODE_OUTPUT);
    }
    void operator=(bool value) {
        gpio_set_level(pin, value);
    }
private:
    gpio_num_t pin;    
};

Here's how that might look:

class GPIO_Outpin {
public:
    GPIO_Outpin(gpio_num_t pin) : pin{pin} {
      gpio_pad_select_gpio(pin);
      gpio_set_direction(pin, GPIO_MODE_OUTPUT);
    }
    void operator=(bool value) {
        gpio_set_level(pin, value);
    }
private:
    gpio_num_t pin;    
};
Source Link
Edward
Edward
  • 67.2k
  • 4
  • 120
  • 284

Here are some things that may help you improve your program.

Prefer modern initializers for constructors

The constructors can both be slightly rewritten to use a more modern style:

BCDWriter(gpio_num_t d0, gpio_num_t d1, gpio_num_t d2, gpio_num_t d3)
: _bits{d0, d1, d2, d3}
{ /* the rest of the constructor */ }

Use "range for" and simplify your code

The code includes a number of lines like this:

for (auto pin = _bits.begin(); pin != _bits.end(); pin++)

The much simpler way to express that in modern C++ is this:

for (auto pin : _bits)

Use logical expressions logically

The code for Write contains this line:

gpio_set_level(*pin, (value & 1) > 0 ? 1 : 0);

This could be much, much simpler:

gpio_set_level(pin, value & 1);

Rethink the interface

These classes aren't really doing much except acting as placeholders for pin numbers. It would be much nicer if we could use a more user-friendly interface. I'd suggest it would make sense for the QuadDigitDisplay to contain both the cathodes and the digits and the currently displayed digit. Then it could have a method diplay which would take the digit and the value as arguments. With such a class, we could rewrite app_main:

void app_main()
{
    printf("Hello PlatformIO!\n");
    QuadDigitDisplay quadDisp({GPIO_NUM_26, GPIO_NUM_22, GPIO_NUM_18, GPIO_NUM_27, GPIO_NUM_19}, {GPIO_NUM_0, GPIO_NUM_17, GPIO_NUM_2, GPIO_NUM_5});
    std::array<short, 5> values{0, 1, 2, 3, 2 /* colon indicator */};

    while (true)
    {
        for (short i = 0; i < 5; ++i) {
            quadDisp.display(i, values[i]);
            vTaskDelay(5 / portTICK_PERIOD_MS);
        }
    }
}

The class could be written like this:

class QuadDigitDisplay
{
public:
  QuadDigitDisplay(std::array<gpio_num_t, 5> cathodes, std::array<gpio_num_t, 4> digits)
  : cathodes{cathodes}
  , digits{digits}
  {
    for (auto pin : cathodes) {
      gpio_pad_select_gpio(pin);
      gpio_set_direction(pin, GPIO_MODE_OUTPUT);
    }
    for (auto pin : digits) {
      gpio_pad_select_gpio(pin);
      gpio_set_direction(pin, GPIO_MODE_OUTPUT);
    }
  }
  
  void display(unsigned short digit, short value) {
      // undisplay currently active digit
      gpio_set_level(cathodes[active_digit], 1);
      // set new active digit
      active_digit = digit;
      // set digit outputs
      for (auto pin : digits) {
          gpio_set_level(pin, value & 1);
          value >>= 1;
      }
      // now display new active digit
      gpio_set_level(cathodes[active_digit], 0);
  }

private:
  std::array<gpio_num_t, 5> cathodes;
  std::array<gpio_num_t, 4> digits;
  unsigned short active_digit = 0;
}

Consider further abstraction

Even as refactored above, there is some duplication. One could create a GPIO_Outpin class that would handle the setup, store the pin number and provide an operator= to set the value of the pin.