I know this request already has several reviews, and one has even been accepted. I actually waited to post this review because it is probably not the kind of review the requester wants.
This review is for people who are targeting embedded platforms with larger, more complex projects. Few people realize how amazingly efficient modern C++ code can be when run through a modern optimizing compiler. It’s almost magical how clear, safe, and high-level code can be, and it still compiles down to a few simple arithmetic, logical, or move instructions on a bare machine word. I guarantee that you… yes you, the reader, whoever you may be… underestimate just how much high-level C++ you can use in an embedded project, because everyone does—even experienced embedded developers with decades of experience. I’ve recently been following the work of Khalil Estell, who is using exceptions on embedded ARM. Yes. You read that right. Exceptions. On embedded. And they may be more efficient than manual error handling. I bet that blew the minds of 99% of the people reading this. I say again, you will be surprised at just how much high-level C++ you can use on embedded, and just how much gains you can get from it.
This does come at a cost, though. The magic is that all of the cost is at development time, not run-time, and almost all of it is a one-time cost: you put a lot of effort into writing a high-quality library once, then it can be used for multiple embedded projects. If you’re just writing a simple, one-off project, like the requester appears to be, then sure, all this work is probably not worth it; it’s probably fine to just use a bunch of bare-int global variables, whatever. (Maybe!) But if you are making multiple embedded projects, or your projects are more complex, then the extra work may be well worth it. This review is for those people.
Before we get into the actual code, I want to take a moment to hammer in something that anyone who has learned C++ from me has heard a thousand times: C++ is a strongly-typed language, and is very probably the most strongly type language you have ever used. Using the type system is the single most important skill for extracting maximal power from C++, both in terms of efficiency, safety, and ergonomics. If you get the types right, everything else Just Works™.
Strong types for times
So let’s look at the first bit of the code, and see where things are not being done optimally:
#include <HT16K33.h>
using namespace std;
// !!
// Pin numbers start. Modify to ensure numbers line up with wires.
// !!
const int P1L = 29; // Player 1 (left) indicator LED.
const int P2L = 37; // Player 2 (right) indicator LED.
const int P1B = 2; // P1s button.
const int P2B = 13; //P2s button.
const int PR = 12; // Pause/Reset button.
// !!
// Pin numbers end. Do not modify anything outside of this.
// !!
bool turn = false; // False for P1s turn, true for P2s.
bool gameActive = false; // Controls if timers act or not.
int P1T = 1800; // P1s timer. Starts at 30:00, in seconds.
int P2T = 1800; // Ditto, for P2.
bool GO = false; // Game Over bool.
HT16K33 P1(0x71); //P1s clock. (Sodder the bottom address jumper to set the address to 0x71.)
HT16K33 P2(0x70); //P2s clock. (Leave address jumpers untouched.)
int secTime = 0; // A built in delay mechanism.
int PRH = 0; // How long the Pause/Reset button has been held for.
int GOT = 0; //Timer used in game over routine.
bool GOI = false; //Bool used in game over routine.
unsigned long sysTime = millis(); // Delay tracker.
uint8_t G[4] = { 125, 119, 55, 121 }; // Raw bytes for GAME OVER display.
uint8_t O[4] = { 63, 62, 121, 49 }; // Ditto.
Set aside the using namespace std; for the moment. That is a cardinal sin, but it is a sin that every competent C++ programmer already knows about.
Also set aside that these are all global variables. This is generally a bad idea, but for a really small program, it might make sense.
The problem I want to focus on here is that we have a long list of (global!!!) values—some constants, some variables—that are all—with only two exceptions—just naked, raw types.
This is bad. Very bad.
Just consider all the time-related variables: All of them are either bare int or (in one case) unsigned long. Two of them say… in comments…that the values are in seconds, and one is named millis, which implies milliseconds. But what about these?:
int secTime = 0; // A built in delay mechanism.
int PRH = 0; // How long the Pause/Reset button has been held for.
int GOT = 0; //Timer used in game over routine.
You know that kind of thing—not encoding the units—was famously the cause of a half-billion-dollar Mars mission disaster, right? You’d think the software development community would have learned the lesson by now.
This one is a cheap and easy fix, because the standard library already comes with typed time units. (For other types of units, have faith, the future looks bright.) Look how much clearer this is:
// "using namespace std;" is bad... but only using the *literals* is fine.
using namespace std::literals::chrono_literals;
auto P1T = 1800s; // P1s timer. Starts at 30:00.
auto P2T = 1800s; // Ditto, for P2.
// ...
auto secTime = 0s; // A built in delay mechanism.
auto PRH = 0s; // How long the Pause/Reset button has been held for.
auto GOT = 0s; //Timer used in game over routine.
// ...
auto sysTime = std::chrono::milliseconds{millis()}; // Delay tracker.
Note that you no longer need to specify that the time is in seconds in the comment. Now it is clear, and also enforced by the compiler.
But you might say: “Hol’ up, Indi… 1800s will produce a std::chrono::seconds, which is std::chrono::duration<T>, where T has at least 35 bits. So T might not be int; it might be long (or something else). (And a similar problem exists for std::chrono::milliseconds.) What if I really, really want the time in seconds to be an int?”
Not a problem. Just do using seconds = std::chrono::duration<int>; (in an appropriate namespace! don’t just pollute the global namespace!), define your own operator""_s, and then change the declarations using the standard operator""s by adding an underscore (for example: auto P1T = 1800_s; (need the underscore, because that is mandatory for user-defined literals)).
I want to stress that all this high-level clarity and safety is completely free. See for yourself; the wrapper and any associated wrapper functions are compiled to effectively nothing, even on the most basic optimization level.
But wait, there’s more!
Here’s the actual line with comment: int P1T = 1800; // P1s timer. Starts at 30:00, in seconds. Okay, so you don’t really want 1800 seconds… you want 30 minutes. They’re the same, sure, but one much more clearly matches the intent. So… say what you mean:
auto P1T = std::chrono::seconds{30min};
// or to force int for seconds:
auto P1T = std::chrono::duration<int>{30min};
// or:
auto P1T = indi::seconds{30min};
And with appropriately named variables:
auto player_1_timer = std::chrono::seconds{30min};
auto player_2_timer = std::chrono::seconds{30min};
// or even:
constexpr auto player_starting_time = std::chrono::seconds{30min};
auto player_1_timer = player_starting_time;
auto player_2_timer = player_starting_time;
… the comments become wholly unnecessary. Yes, even the “ditto”s. The code now speaks for itself. And, more importantly, everything will be type-checked at compile time, so you won’t screw up and mixing minutes, seconds, or milliseconds. Everything Just Works™.
(I find the argument that it’s okay to give variables short, meaningless names because it means less typing to be profoundly silly. You save a few keystrokes in the variable name… at the cost of a much longer freaking comment!!! Not to mention the massively increased probability of errors, because variables are so cryptic. It ain’t worth it. Even if you’re using a shitty IDE like the Arduino one.)
One last thing before moving on:
int timesub(int timer, HT16K33 player) {
timer = timer - 1;
int tempMin = timer / 60;
int tempSec = timer % 60;
player.displayInt((tempMin * 100) + tempSec);
return timer;
}
This is a terrible function, for a number of reasons.
First, the name. “timesub”? Time substitute? Time-travelling submarine? Code doesn’t run faster if you use shorter variable names. Be clear.
More importantly, this function is bad because it does two jobs: it decrements the timer and it displays the time. A function should have a single job. And in this case, it would be far too easy to forget to reassign the return value to the timer variable (or assign it to the wrong variable). So instead of:
P1T = timesub(P1T, P1); // Subtract 1 unit (second).
You could do:
--P1T;
display_time(P1T, P1);
Which is already clearer, but could even clearer still:
player_1_time -= 1s;
display_time_for(player_1, player_1_time);
With the time display function being:
auto display_time_for(HT16K33 player, std::chrono::seconds time)
{
auto const minutes = time.count() / 60;
auto const seconds = time.count() % 60;
player.displayInt(static_cast<int>((minutes * 1000) + seconds));
}
You could also call player.displayColon(1); in there, too, so that even if the colon has been turned off, it will be correctly turned back on. And maybe even call displayOn() to ensure the display is actually on, in the case that it might have been turned off. And then this function could also be used in setup(), to eliminate all the duplicate code; you’d just do display_time_for(player_1, 30min);, and the same for player 2. Six lines would collapse to two, and be much clearer.
That should probably do for the time stuff. Let’s move on to something more interesting.
Self-registering, self-validating pins
const int P1L = 29; // Player 1 (left) indicator LED.
const int P2L = 37; // Player 2 (right) indicator LED.
const int P1B = 2; // P1s button.
const int P2B = 13; //P2s button.
const int PR = 12; // Pause/Reset button.
So what’s the problem here? Well, consider this:
// !!
// Pin numbers start. Modify to ensure numbers line up with wires.
// !!
const int P1L = 29; // Player 1 (left) indicator LED.
const int P2L = 37; // Player 2 (right) indicator LED.
const int P1B = 2; // P1s button.
const int P2B = 13; //P2s button.
const int PR = 13; // Pause/Reset button.
// !!
// Pin numbers end. Do not modify anything outside of this.
// !!
// ...
// a dozen lines later
// ...
void setup() {
// Setting pin types.
pinMode(P1L, INPUT);
pinMode(P2L, INPUT);
pinMode(P1B, OUTPUT);
pinMode(P2B, OUTPUT);
// Begin LED clock startup.
See the problem? Did you notice that there were multiple problems? Did you spot all of them?
As always, the first thing to do is to use proper types. You need an actual pin class, at least. But let’s see if we can do better.
What if we define the pin class something like this:
class pin
{
public:
enum class mode
{
input,
output
};
static consteval auto register_pin(int, mode) -> pin;
// No default constructor or other public constructors
// except for a copy constructor.
The idea is this: pin is a handle to a physical pin. You can’t simply conjure physical pins out of thin air. So you can’t simply construct pin objects.
Instead, when you want to assign a function to a pin, you use pin::register_pin(), which is a compile-time-only (consteval) function.
So instead of:
const int P1L = 29; // Player 1 (left) indicator LED.
const int P2L = 37; // Player 2 (right) indicator LED.
const int P1B = 2; // P1s button.
const int P2B = 13; //P2s button.
const int PR = 12; // Pause/Reset button.
… you do:
constexpr auto player_1_indicator = pin::register_pin(29, pin::mode::output);
constexpr auto player_2_indicator = pin::register_pin(37, pin::mode::output);
constexpr auto player_1_button = pin::register_pin(2, pin::mode::input);
constexpr auto player_2_button = pin::register_pin(13, pin::mode::input);
constexpr auto pause_reset_button = pin::register_pin(12, pin::mode::input);
Or, alternatively:
class input_pin
{
public:
static consteval auto register_pin(int) -> input_pin;
// No default constructor or other public constructors
// except for a copy constructor.
// ...
};
class output_pin
{
public:
static consteval auto register_pin(int) -> output_pin;
// No default constructor or other public constructors
// except for a copy constructor.
// ...
};
constexpr auto player_1_indicator = output_pin::register_pin(29);
constexpr auto player_2_indicator = output_pin::register_pin(37);
constexpr auto player_1_button = input_pin::register_pin(2);
constexpr auto player_2_button = input_pin::register_pin(13);
constexpr auto pause_reset_button = input_pin::register_pin(12);
Okay, but what do we gain from this?
Here’s a crazy thought. Since register_pin() is a compile-time function, what if it checked… at compile-time… if the pin number was valid? So if you accidentally used the same pin number for two functions, that would be detected, and the program would fail to compile, saying, for example: “pin number 13 is already in use”. Not only that, it could check that the pin number given is valid, and not negative or out of range, or the number of a pin that cannot be used for one purpose or another.
That’s not all. What if you could write setup() like this:
void setup()
{
setup_pins();
// ...
… and every registered pin would be automatically registered with the correct I/O mode? So you can’t forget to initialize a pin, or initialize it to the wrong mode, even if you add/remove pins later, multiple times. Everything Just Works™.
I’m not going to work out all the code for how to get all of this done. I’m hardly going to write an entire Arduino library here in a code review. And of course there’s a lot of complexity I’m hand-waving over, like analog versus digital, INPUT_PULLUP, and more. Let’s call it an exercise for the reader.
Once again, everything I am describing is absolutely zero-cost at run-time. All the checking can be done at compile-time. Or, perhaps, depending how you structure everything, you can have a debug mode, where some checks are optionally done at run-time. Anything is possible.
Gross program structure
Let’s pull back and take a really high-level look at things.
Large chunks of your program look basically like this:
if (turn)
{
// Big chunk of code for player 1
}
else
{
// Almost identical big chunk of code for player 2
}
(Although I see a few subtle bugs from cases where there are differences between the two branches!)
Similar patterns show up in other places. Basically, you have two players, and the game alternates between them, but otherwise the behaviour is identical between the two players.
Consider instead if you made a player class that held all of the information relevant to a player:
// VERY rough example; not good code; just illustrative of the concept
struct player
{
HT16K33 display;
output_pin indicator;
input_pin button;
std::chrono::seconds remaining_time;
};
With that, you create an array of all the players, with an index pointing to the current player:
auto players = std::array{
player{HT16K33{0x71}, output_pin::register_pin(29), input_pin::register_pin(2)},
player{HT16K33{0x70}, output_pin::register_pin(37), input_pin::register_pin(13)}
};
auto current_player_index = 0uz;
Naturally, you can do some compile-time checks to make sure the number of players is correct, and that pin numbers and display numbers and so on aren’t reused. (With the pin type described earlier, that won’t be a problem for the pins, at least.)
With just that, all of that duplication goes away. For example:
if (gameActive = true) // Is game active?
{ // Yes.
if (secTime <= 0) // Has a second passed?
{ // Yes.
if (turn = false) // Is it P1s turn?
{ // Yes.
if (P1T = 0) // Is P1 out of time?
{ // Yes.
GO = true; // Set GAME OVER to true.
gameActive = false; // Stop the game.
} else { // No, P1 still has time.
P1T = timesub(P1T, P1); // Subtract 1 unit (second).
secTime = 1000; // Set the delay timer to 1 second.
}
} else { // No, it's P2s turn.
if (P2T = 0) // Is P2 out of time?
{ // Yes.
GO = true; // Set GAME OVER to true.
} else { // No, P2 still has time.
P2T = timesub(P2T, P2); // Subtract 1 second.
secTime = 1000; // Set the delay timer to 1 second.
}
}
}
}
… becomes:
if (gameActive)
{
if (secTime <= 0)
{
if (players[current_player_index].remaining_time == 0s)
{
GO = true;
gameActive = false;
}
else
{
players[current_player_index].remaining_time -= 1s;
display_time_for(players[current_player_index], players[current_player_index].time);
secTime = 1s;
}
}
}
… and a bug is automatically fixed!
Or, with some even better ergonomics:
if (gameActive)
{
if (time_elapsed > 1s)
{
auto& current_player = players[current_player_index];
current_player.remaining_time -= duration_cast<std::chrono::seconds>(time_elapsed);
if (current_player.remaining_time <= 0s)
{
game_over = true;
game_active = false;
}
else
{
current_player.display_time();
}
}
}
Once again, the code is so clear, you don’t even need comments.
But it’s not only that it’s clearer and shorter. By using an array of players instead of hard-coding player 1 and player 2, now you can have any number of players from 1 to, well, the limit of std::size_t. Obviously it doesn’t make sense for the number to be anything other than two for conventional chess… but by simply refactoring the code this way, the exact same program can be used for games with more players, or even for self-timing something. All you need to do is add/remove players from the players array, with the appropriate pin and display settings.
And we’re still just scratching the surface.
Because the code, as written, well, it’s kind of a spaghetti mess. It’s not clear exactly what is supposed to be going on. And if you’re not very careful about all your top-level if checks, you may find random “game over”s occurring… or maybe “game over” happens only when one player’s time zeros, but not the other… or maybe pressing the pause button restarts the game completely, or resets the timer for one or both players, or… who knows what else. I assume the code works “correctly” now (the bugs I and everyone else here spotted notwithstanding). But I can’t be sure of that, and I don’t think anyone can, without a very, very, VERY careful inspection of the code.
And heaven help you if you ever want to change anything. Like, say you wanted to add a feature where every time a player pauses they get a 10-second penalty. Or maybe when there are less than 10 seconds left, the display changes from MM:SS to SS.SS counting down fractions of a second for more excitement. Making those types of change is effectively a total rewrite.
If you need a small army of experienced coders to pore over your code to (hopefully!) be sure it works as expected… you’ve really failed as a programmer. I love coding and I’ve been doing it for fun for literally decades… but I am still lazy AF. When I write code, I write it along with automated tests (actually, I often write the tests before the actual code), that run it through the wringer, inside-out and sideways, so I know it works… without the effort of poring over code and carefully keeping track of the state of system memory in my easily-distracted brain.
Test. Your. Code. Untested code is garbage code. By that I don’t imply anything about the quality of the code: it may be the most beautiful and most brilliant code ever written. But if it has not been tested, it is absolutely useless. No sane software engineer would put code in their program where, if they ask the coder if it works, the coder just shrugs and goes “I think so, LOL!”.
Code should be tested, and it should be written in a way that allows it to be tested. How would you test if that big spaghetti mess inside loop() actually works as expected?
Let’s pull back, and take a higher view of the program. Fundamentally, it’s just a state machine. It starts in a “READY” state. When the pause/reset button is pressed, it switches to the “PLAYING” state. (Here I think you have a logic bug, in that the game won’t start at all if you hold down the button for too long. That might be what you want; if you press the button accidentally you can keep holding it to abort the game start. Or it might not be what you want; you can press the button and hold it until all players say they’re ready, then release it to start. As I say, it might be a logic bug, but with the code written as is it is just impossible to tell.) It stays in the play state until either the pause button is pressed—in which case it goes to the “PAUSE” state—or the player hits their button—in which case it enters the “NEXT PLAYER” state (which changes the active player, resets their time, and then re-enters the “PLAY” state)—or the time runs out—in which case it enters the “GAME OVER” state.
Mind you, I’m just guessing at what the states are supposed to be, because it is impossible to determine from the code… which is not good. The code as written just basically describes a disjointed set of individual actions that you (hopefully) want to happen (and hopefully don’t forget any) when some set of facts is true. Even describing it like that you can see why it is dangerous and buggy to code that way. Instead, you want to be doing is describing what should be happening in just the current program state. That way you don’t need to reason about… well, the universe; as in every possible set of inputs and internal state the program might currently be in. You just need to know what to do in one single, specific state, including when you might want to switch to another (encapsulated, self-contained, easy-to-reason-about) state.
Here’s just one way to go about it… maybe not the best, but perhaps the simplest.
First, we start with an abstract base class for states:
struct state
{
virtual ~state() = 0;
virtual auto enter() -> void {};
virtual auto exit() -> void {};
virtual auto run(std::chrono::milliseconds elapsed) -> state* = 0;
};
state::~state() = default;
And a function to switch states:
auto change_state(state* before, state* after)
{
before.exit();
after.enter();
return after;
}
Now the main function is just:
auto loop()
{
static auto current_state = /*???*/;
static auto last_time = std::chrono::steady_clock::now();
auto current_time = std::chrono::steady_clock::now();
auto elapsed = current_time - last_time;
if (auto next_state = current_state->run(elapsed); next_state)
current_state = change_state(current_state, next_state);
last_time = std::chrono::steady_clock::now();
}
Now, you may be thinking: “Virtual functions? Abstract classes? In an embedded program? Surely not!” Yes, virtual functions are more expensive than regular functions because they are indirect. But that cost is literally a single indirection; it is on par with an if statement… on some platforms and in some situations it can be slightly slower, or even slightly faster. A class with virtual functions adds the cost of a vtable for the class, and an extra pointer for each instance. Given that we will only be making a small number of states, that is peanuts. And again, that spaghetti mess of if statements might even be slower, because there are multiple checks done on each iteration of the loop function… whereas here, only a single indirect call is done (unless the state is being changed, in which case up to three indirect calls are done, which is still peanuts, and is not happening every loop anyway). We are using virtual functions, but we are not using dynamic memory allocation.
Now we can just focus on the ready state:
class ready_state : public state
{
std::chrono::milliseconds pause_reset_button_hold_time = {};
// ...
};
What do we need to do when entering the ready state? Well, we probably want to ensure the displays for all the players are turned on, and displaying the correct time:
auto ready_state::enter() -> void
{
for (auto&& p : players)
{
p.display.displayOn();
p.remaining_time = 30min;
p.display_time();
p.indicator = false;
}
// Reset the current player to the first player.
current_player_index = 0uz;
}
That’s probably all we need to do, but we should think holistically about the system state. We should not assume that we are entering the ready state with all the buttons unpressed. Maybe someone is holding down the pause/reset button, or one of the player buttons. What to do then? Well… that’s up to you; that’s the kind of thing you really should be thinking about when designing this kind of device. Maybe you should set flags for all the buttons that are being pressed when you enter the ready state, and refuse to leave unless/until all buttons are released. Maybe if a button is never released but someone presses the button to start, you could blink the display a few times: that could indicate that someone is unknowingly leaning on a button, or maybe the button is broken. You see all the things you need to think about, but don’t become clear until you break the problem into smaller states?
Anywho, let’s assume that we enter the ready state with all the buttons not pressed. Now what. Well, now we wait until someone starts the game. Your current design starts the game with the pause/reset button, so:
auto ready_state::run(std::chrono::milliseconds elapsed) -> state*
{
if (pause_reset_button.is_high())
{
pause_reset_button_hold_time += elapsed;
}
// Button *was* pressed, but is now released.
else if (pause_reset_button_hold_time != 0ms)
{
if (pause_reset_button_hold_time < 2s)
return playing_state;
else
return nullptr;
}
}
And that’s pretty much it for the ready state, because there is probably nothing we need to do to “clean things up” when exiting the ready state. Though we should consider the case of what to do if someone presses other buttons, or holds other buttons down while pressing pause/reset, and so on.
But consider two things. First, by breaking the ready state into its own, isolated problem, we spotted a number of complications that weren’t obvious in the original program.
Second, suppose we want to change things. Suppose that instead of always automatically starting with player 1 every time, we use the player buttons to determine who is the starting player. If player 1 presses (and releases) their button first, we set current_player_index to their index and move to the playing state… but if player 2 presses (and releases) their button first, then we set current_player_index to their index (and so on for other players as well). That way if two people are playing multiple games in a row, they don’t need to switch seats or move the clock around to alternate who goes first.
For the playing state, we follow the same process. What do we need to set up when entering the playing state? Well, the currently active player needs their display needs to be turned on. That’s pretty much it, I think (other than checking what’s up with the buttons to avoid weirdness happening). What about when running the state? Well, we increase the player’s elapsed time by elapsed, and we monitor both the pause/reset button, the player button, and the player’s elapsed time. Then we may enter the pause state—or, if the button is held down too long before being released, the ready state—or we may change the player then re-enter the playing state, or we may enter the game-over state.
And so on. Each state becomes an isolated problem that we can reason about more easily, and spot edge cases and complications (like shenanigans with pressing/holding multiple buttons). Once a state is solid, then nothing that happens in any other state should affect it. That way, bugs remain isolated, and are much easier to narrow down.
I’ve glossed over quite a bit. You may need/want intermediate states; for example, when in the ready state, if the start button is pressed, immediately jump to a “start new game” state that sets up the new game, and stay in that state until the button is released. If it was held for too long, go back to ready, otherwise go to playing… but check the state of the other buttons first. You might want to enter a “WTF” state if someone is holding an unexpected button down, or add other features (like a way to configure the time each player gets: maybe instead of just going back to ready if the start button was held too long, you go to a “configure” state, where one player’s button increases the time allotted, and the other’s decreases, or something).
And of course I haven’t even touched on the topic of testability; it should be possible to create the states in isolation, and mock all types of button presses, in any order for any length of time, and make sure states transition the way you expect them to. That should be done as part of an automated testing suite, which you can run on the development machine with mocked I/O and timers. (Everything else should be rigorously tested, too, like the pin classes. Untested code is garbage code.)
Even though most of what I’ve described probably feels like overkill for a “small” project with a single developer, it’s really not. Reasoning about all the potential complications of button pressing/holding/mashing for each state of operation is damn near impossible without some form of state isolation. And all the strong typing stuff not only massively simplifies the actual business logic, it prevents silly errors like reusing pin numbers. Good programming practices and high-level thinking and design take more work than just farting out step-by-step procedural code, but they do pay off, even for “small” projects, and even for just a single developer.
