Clock display controlled by esp32

Question

I recently got fascinated by the world of micro controllers, and chose to do a simple clock project to see what is what. First attempt I used .Net nanoframework since I'm a C# coder by day, but I haven't been able to figure out how to make it execute fast enough.

I ported my code to C++ and used PlatformIO to build and deploy. It works and I completely eliminated the flickering problem I had, but as I haven't used C for over a decade and never really sunk my teeth into C++, I could really use some input on best practices and similar.

As my project grows I'll need to put my classes into separate .hpp files (Google suggests C++ continues C's tradition of putting the interface in a header file and the implementation in a .cpp file -- I am a little bit miffed VSCode doesn't offer me this simple refactoring, so I feel I'm missing something obvious). But apart from that, what else am I missing?

Enums felt a bit off to me. In C# I'd do a myEnum++. Porting foreach caused me some headache. I found std::for_each() but in the end I decided for's syntax was actually more readable. My std::array initialization is probably also a bit wonky.

To run this in its full glory: an ESP32 controller is required, a TI CD74HC4511E BCD decoder and a LiteOn LTC-2723Y 4 digit 7 segment display. Probably good to include seven resistors for good measure.

#include <stdio.h>
#include "freertos/FreeRTOS.h"
#include "freertos/task.h"
#include "esp_system.h"
#include "esp_spi_flash.h"
#include "driver/gpio.h"
#include <array>
#include <algorithm>

extern "C"
{
  void app_main(void);
}

// Outputs a 4-bit value to a BCD decoder
class BCDWriter
{
public:
  BCDWriter(gpio_num_t d0, gpio_num_t d1, gpio_num_t d2, gpio_num_t d3)
  {
    _bits = {d0, d1, d2, d3};
    for (auto pin = _bits.begin(); pin != _bits.end(); pin++)
    {
      gpio_pad_select_gpio(*pin);
      gpio_set_direction(*pin, GPIO_MODE_OUTPUT);
    }
  }

  void Write(short value)
  {
    for (auto pin = _bits.begin(); pin != _bits.end(); pin++)
    {
      gpio_set_level(*pin, (value & 1) > 0 ? 1 : 0);
      value >>= 1;
    }
  }

private:
  std::array<gpio_num_t, 4> _bits;
};

// Controls a four digit 7 segment display (e.g. LiteOn LTC-2723Y)
// Assumes common cathode (set digit pin low to activate that digit's display)
class QuadDigitDisplay
{
public:
  enum QuadDigit
  {
    First,
    Second,
    Third,
    Fourth,
    Indicators
  };

  QuadDigitDisplay(gpio_num_t cc1, gpio_num_t cc2, gpio_num_t cc3, gpio_num_t cc4, gpio_num_t l)
  {
    _pins = {cc1, cc2, cc3, cc4, l};
    for (auto pin = _pins.begin(); pin != _pins.end(); pin++)
    {
      gpio_pad_select_gpio(*pin);
      gpio_set_direction(*pin, GPIO_MODE_OUTPUT);
    };
  }

  void SetHigh(QuadDigit quadDigit)
  {
    gpio_set_level(_pins[quadDigit], 1);
  }

  void SetLow(QuadDigit quadDigit)
  {
    gpio_set_level(_pins[quadDigit], 0);
  }

private:
  std::array<gpio_num_t, 5> _pins;
};

// Display "12:34" test output on the display
void app_main()
{
  printf("Hello PlatformIO!\n");
  QuadDigitDisplay quadDisp(GPIO_NUM_26, GPIO_NUM_22, GPIO_NUM_18, GPIO_NUM_27, GPIO_NUM_19);
  BCDWriter bcd(GPIO_NUM_0, GPIO_NUM_17, GPIO_NUM_2, GPIO_NUM_5);

  QuadDigitDisplay::QuadDigit quadDigit = QuadDigitDisplay::QuadDigit::First;
  while (true)
  {
    uint8_t number = quadDigit == QuadDigitDisplay::QuadDigit::Indicators ? (uint8_t)2 : (uint8_t)quadDigit;
    bcd.Write(number);
    quadDisp.SetLow(quadDigit);
    vTaskDelay(5 / portTICK_PERIOD_MS);
    quadDisp.SetHigh(quadDigit);

    if (quadDigit == QuadDigitDisplay::QuadDigit::Indicators)
    {
      quadDigit = QuadDigitDisplay::QuadDigit::First;
    }
    else
    {
      quadDigit = QuadDigitDisplay::QuadDigit(quadDigit + 1);
    }
  }
}

G. Sliepen · Accepted Answer · 2021-09-04 12:29:54Z

Make `class QuadDigitDisplay` do what it says

The QuadDigitDisplay class doesn't actually handle displaying four digits, it only sets the pins that enable each digit. It would be nice if this class actually handled everything necessary to drive the quad digit display.

Ideally, your main function looks like this:

void app_main()
{
    QuadDigitDisplay quadDisp(GPIO_NUM_26, ...);

    // Display 11:11, 22:22 and so on in a loop
    while (true) {
        for (int i = 0; i < 10; i++)
            quadDisp.setDigits({i, i, i, i});
            vTaskDelay(1000 / portTICK_PERIOD_MS);
        }
    }
}

The QuadDigitDisplay class should create a new task in its constructor which takes care of displaying the digits. So it should look something like:

class QuadDigitDisplay
{
public:
  QuadDigitDisplay(gpio_num_t cc1, ...): _pins{cc1, ...}, _bcd{d0, ...} {
    // Set pins to output
    for (auto pin: _pins) {
      gpio_pad_select_gpio(pin);
      gpio_set_direction(pin, GPIO_MODE_OUTPUT);
    };

    // Create the display task
    xTaskCreate(DisplayDigits, "display digits", 100, this, 1, &_task);
  }

  ~QuadDigitDisplay() {
    // Stop the display task
    vTaskDelete(_task);

    // Set pins to input
    for (auto pin: _pins) {
      gpio_set_direction(pin, GPIO_MODE_INPUT);
    };
  }

  void SetDigits(const std::array<int, 4> &digits) {
      _digits = digits;
  }

private:
  static void DisplayDigits(void *arg) {
    QuadDigitDisplay *self = arg;

    while (true) {
      for (auto digit = 0; digit < 4; ++digit) {
        self->_bcd.Write(self->_digits[digit]);
        gpio_set_level(self->_pins[digit], 0);
        vTaskDelay(5 / portTICK_PERIOD_MS);
        gpio_set_level(self->_pins[digit], 1);
      }
    }
  }

  TaskHandle_t _task{};
  std::array<int, 4> _digits;
  std::array<gpio_num_t, 5> _pins;
  BCDWriter _bcd;
};

You could move the original functionality of QuadDigitDisplay into a new class with a name that better describes what it does, perhaps DigitSelector. Then you can use that class inside QuadDigitDisplay to abstract away the digit selection, just like it uses BCDWriter.

Consider removing the `enum`

An enum whose values are mostly just ONE, TWO, THREE or FIRST, SECOND, THIRD is not very useful. Just use a regular integer for this, they are great for counting! Also consider using a separate variable to hold the pin number for the indicators, as its purpose is really different from those of the digits themselves.

Use range-for

Porting foreach caused me some headache. I found std::for_each() but in the end I decided for's syntax was actually more readable.

Since C++11 you can do "foreach" with a regular for statement, like I already showed in the code above. This is called a range-based for loop, and you use it like so:

std::array<gpu_num_t, 5> _pins;
...
for (auto pin: _pins) {
  gpio_pad_select_gpio(pin);
  gpio_set_direction(pin, GPIO_MODE_OUTPUT);
};

For me the enum is a constraint. Note that it contains a member called "Indicator". It isn't just a number. Ideally I would want to say "initialize to the first member of my enum" and ask "is the current value equal to the last member of the enum?". Of course an int would work, but there are only five valid numbers. It would be neat if the language/compiler would help me guard this automatically. In Pascal I'd use Low() and High() (with a set) IIRC. — 9Rune5
– 9Rune5, Commented Sep 4, 2021 at 13:26
Setting pins to input in the destructor -- is that to help the board function a bit better when it is being programmed? I've experienced some problems with the nano framework as I've been forced to disconnect the ground pins when programming my board using USB. — 9Rune5
– 9Rune5, Commented Sep 4, 2021 at 13:32
It's good practice in general to set the pins to a high impedance mode when you are no longer needing them, and if you can choose whether a pin has a pull-up or pull-down resister, set it to the one that causes the device to go idle. It might not be relevant though if your program runs in an infinite loop and never quits, as the destructors are then never called. — G. Sliepen
– G. Sliepen, Commented Sep 4, 2021 at 20:27

Edward · Accepted Answer · 2021-09-04 15:42:47Z

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

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

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.

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

Stack Exchange Network

Clock display controlled by esp32

2 Answers 2

Make `class QuadDigitDisplay` do what it says

Consider removing the `enum`

Use range-for

Prefer modern initializers for constructors

Use "range `for`" and simplify your code

Use logical expressions logically

Rethink the interface

Consider further abstraction

You must log in to answer this question.

Clock display controlled by esp32

2 Answers 2

Make class QuadDigitDisplay do what it says

Consider removing the enum

Use range-for

Prefer modern initializers for constructors

Use "range for" and simplify your code

Use logical expressions logically

Rethink the interface

Consider further abstraction

You must log in to answer this question.

Related

Make `class QuadDigitDisplay` do what it says

Consider removing the `enum`

Use "range `for`" and simplify your code