2
\$\begingroup\$

The code below I've made is supposed to act as a chess clock when uploaded to a compatible Arduino. I've written everything, but I just want another set of eyes to look over the code in case I've missed something obvious and/or forgotten a certain feature before I solder all the wires and hardware together. There's no errors when I verify in the Arduino IDE, but that doesn't mean there's no bugs.

There's supposed to be two lights to indicate the turn, two HT16K33 4 digit 7 segment clocks for time display, and three buttons for various operations.

Used libraries:

HT16K33 - https://github.com/RobTillaart/HT16K33/tree/master

Code:

#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.
void setup() {
  // Setting pin types.
  pinMode(P1L, OUTPUT);
  pinMode(P2L, OUTPUT);
  pinMode(P1B, INPUT);
  pinMode(P2B, INPUT);
  pinMode(PR, INPUT);

  // Begin LED clock startup.
  Wire.begin();
  Wire.setClock(100000);
  P1.begin();
  P2.begin();
  P1.displayOn();
  P2.displayOn();
  P1.displayInt(3000);
  P2.displayInt(3000);
  P1.displayColon(1);
  P2.displayColon(1);
  // End LED clock startup.
}

void loop() {
  secTime = (millis() - sysTime);            // Decrement sectime based on how many MS the last loop took.
  sysTime = millis();                        // Adjust sysTime after secTime is adjusted.
  if (digitalRead(P1B) or digitalRead(P2B))  // Are either of the player buttons pressed?
  {
    secTime = 1000;        //Reset secTime.
    if (digitalRead(P1B))  //Is P1s button pressed?
    {
      turn = false;             // false = P1s turn
      digitalWrite(P1L, HIGH);  // Turn on P1s LED
      digitalWrite(P2L, LOW);   // Turn off P2s LED.
    } else                      //No, P2s button is pressed.
    {
      turn = true;  // Do the opposite of above.
      digitalWrite(P2L, HIGH);
      digitalWrite(P1L, LOW);
    }
  }
  // No? Than is the pause/reset button pressed?
  if (digitalRead(PR)) {                 // Yes.
    secTime = 1000;                      // Reset secTime.
    PRH = (PRH + (millis() - sysTime));  //Increment PRH.
    if (PRH >= 2000)                     // Has PR been held for 2 seconds or longer?
    {                                    // Yes.
      gameActive = false;                // Stop the game.
      P1T = 1800;
      P2T = 1800;  // Reset timers.
      P1.displayInt(3000);
      P2.displayInt(3000);  // Reset displays.
      GO = false;           //Reset GAME OVER.
    }
  }
  if (!digitalRead(PR) && (PRH > 0))  // Has PR been released?
  {                                   // Yes.
    if (PRH >= 2000)                  //Was PR held for two seconds or longer?
    {                                 // Yes.
      PRH = 0;                        // Reset PRH and do nothing more.
    } else {                          // No, PR was held for less than 2 seconds.
      secTime = 1000;                 //Reset secTime.
      gameActive = !gameActive;       // Toggle gameActive.
    }
  }
  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.
        }
      }
    }
  }
  if (GO = true)  // Has someone ran out of time?
  {
    if (secTime <= 0)  //Is secTime 0 or less?
    {
      if (turn = false)  //Is it P1's turn?
      {
        if (GOI = false) {
          P1.displayRaw(G);        // GAME
          P1.displayColon(false);  // disable colon
          GOI = true;
          secTime = 750;
        }
        if (GOI = true) {
          P1.displayRaw(O);  // OVER
          GOI = false;
          secTime = 750;
        }
      } else {
        if (GOI = false) {
          P2.displayRaw(G);        // GAME
          P2.displayColon(false);  // disable colon
          GOI = true;
          secTime = 750;
        }
        if (GOI = true) {
          P2.displayRaw(O);  // OVER
          GOI = false;
          secTime = 750;
        }
      }
    }
  }
}

int timesub(int timer, HT16K33 player) {
  timer = timer - 1;
  int tempMin = timer / 60;
  int tempSec = timer % 60;
  player.displayInt((tempMin * 100) + tempSec);
  return timer;
}

// -- END OF FILE --
New contributor
Beans is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$

3 Answers 3

Reset to default
3
\$\begingroup\$

Personal comments:

Indentation of only two spaces is real hard to read. Please consider using 4.

Recommendation

Put code in a class for testability.

Use a couple more functions to remove repeated code.

Some bugs need to be removed:

if (GO = true)
      ^^^ This is an assignment not a test.

Maybe increase the compiler warning level to get the compiler to give you an idea of what it thinks may be a potential issue.

My advice the absolute minimum yuo need on a new project are:

-Wall -Wextra -Werror

But there are many more.

Code Review

This is considered bad practice.

using namespace std;

There are a lot of situations where you can accidently change the meaning of the code becuase of the vast number of things in the standard namespace.

See this question for details What's the problem with "using namespace std;"?

If these constants align with actual PIN names on the board then fine.

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.

But otherwise why not change the comment into the variable name?
More expressive names make things easier.

Also why

const int P1B = 2;   // P1s button.
const int P2B = 13;  //P2s button.
                       ^ Missing space.

Putting all your variables into the global namespace is problematic for testing.

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.

If you put everything into a class with appropriate constructors, it makes writing some tests so much easier.

Sure.

bool turn = false;                     // False for P1s turn, true for P2s.

But would it not be easier to use named things so the code it is easy to read.

enum Turn {Player_1_Turn, Player_2_Turn};
Turn turn = Player_1_Turn;

Now in the code you explicitly test for which player:

if (turn == Player_2_Turn) {
     <Do Stuff>
}

But if I look further into the code I see you have more state that indicates if the games is running etc. Why not combine this into a single variable..

enum GameState {NotStarted, Player_1_Turn, Player_2_Turn, Player_1_Won, Player_2_Won/*, Draw ??*/};

int P1T = 1800;

or you could write:

Int Player_1_Time_Seconds = 60 * 30;  // 1800 seconds.

First. Single letter variable name! Second why not a single array, or a two D array?

uint8_t G[4] = { 125, 119, 55, 121 };  // Raw bytes for GAME OVER display.
uint8_t O[4] = { 63, 62, 121, 49 };    // Ditto.

Please some vertical space between sections:

uint8_t O[4] = { 63, 62, 121, 49 };
void setup() {

void setup() {

  <Removed>

  // Begin LED clock startup.
  Wire.begin();
  Wire.setClock(100000);
  P1.begin();
  P2.begin();
  P1.displayOn();
  P2.displayOn();
  P1.displayInt(3000);
  P2.displayInt(3000);
  P1.displayColon(1);
  P2.displayColon(1);
  // End LED clock startup.
}

I don't see declarations for Wire, P1 or P2. Is this an ardino thing?

This screams for a function.

  P1.begin();
  P2.begin();
  P1.displayOn();
  P2.displayOn();
  P1.displayInt(3000);
  P2.displayInt(3000);
  P1.displayColon(1);
  P2.displayColon(1);

You are repeating code. If you repeat code and find a bug that means you need to fix the bug in multiple places. To prevent this have the code in one function and call it twice (or better wrap it in a class).

  void setupPlayer(Player& player, int initTime)
  {
      player.begin();
      player.displayOn();
      player.displayInt(initTime);
      player.displayColon(1);
  }

Then your code becomes:

  setupPlayer(P1, 3000); // Why 30000?
  setupPlayer(P2, 3000); // Is this different from P1T/P2T
                         // If not you should be using these variables.
\$\endgroup\$
3
  • \$\begingroup\$ (1/3?) Let me address this piece by piece, because while some of these points seem great, others seem.. Eh. Personal Comments This is how the Arduino IDE does autoformatting. Looks good in the program, seemingly not good here. Sorry. Recommendation Each of those will indeed be fixed, good catch. (+1) Code Review - Bad Practice Once again, yep, that's my bad. I grabbed std thinking I would need it to turn my one number into mins and secs, but never did need it. \$\endgroup\$ Commented 10 hours ago
  • \$\begingroup\$ (2/3) Undeclared Variables Wire is included as a part of the library, and P1 and P2 are indeed declared, between the end of the pin numbers and setting pin types. Short Variables The variable names were designed like this for faster typing. The Arduino IDE doesn't seem to have auto-complete, so the difference between say Player_1_Turn and false/true will start to add up. Should be noted that there's only a single dev, me, working on this. Missing Spaces Simple typing error. \$\endgroup\$ Commented 10 hours ago
  • \$\begingroup\$ (3/3) enum This is my first time hearing of this, apologies. I've only taken a python and unity class, not a C++ class, but I'm the only person who knows ANY amount of code on this project. ` // Is this different from P1T/P2T` Yes. Because of how displayInt() works, displayInt(1800) will not yield the desired results, appearing as 18:00 on the physical display, while displayInt(3000) will work as desired (30:00). This discrepancy is handled in the timeSub function. @toolic This also addresses some of your concerns. \$\endgroup\$ Commented 10 hours ago
1
\$\begingroup\$

Comparison

In this expression:

if (P1T = 0)

= is an assignment operator, but it seems like you intended to use a comparison operator, such as ==. That line is worth reviewing for correctness. There are other similar lines, such as:

if (turn = false)            // Is it P1s turn?

if (P2T = 0)               // Is P2 out of time?

There's no errors when I verify in the Arduino IDE

I'm not sure what that means, but you should review how you check your code for intended behavior.

Layout

The code uses 2 spaces per indentation level:

void loop() {
  secTime = (millis() - sysTime);            // Decrement sectime based on how many MS the last loop took.
  sysTime = millis();                        // Adjust sysTime after secTime is adjusted.
  if (digitalRead(P1B) or digitalRead(P2B))  // Are either of the player buttons pressed?
  {
    secTime = 1000;        //Reset secTime.
    if (digitalRead(P1B))  //Is P1s button pressed?
    {
      turn = false;             // false = P1s turn

It is easier to read and understand the code using 4 spaces per level:

void loop() {
    secTime = (millis() - sysTime);            // Decrement sectime based on how many MS the last loop took.
    sysTime = millis();                        // Adjust sysTime after secTime is adjusted.
    if (digitalRead(P1B) or digitalRead(P2B))  // Are either of the player buttons pressed?
    {
        secTime = 1000;        //Reset secTime.
        if (digitalRead(P1B))  //Is P1s button pressed?
        {
            turn = false;             // false = P1s turn

Naming

Consider giving the GO variable a more meaningful name, such as game_over:

bool GO = false;                       // Game Over bool.

Then you don't even need the comment.

Typo

"Sodder" should be "Solder".

\$\endgroup\$
1
  • \$\begingroup\$ To answer the question only raised in your answer, verifying in the Arduino IDE will attempt to compile your code, so any big errors will be spotted with that. \$\endgroup\$ Commented 10 hours ago
1
\$\begingroup\$

The pin numbers made sense to me. But maybe we could spring for more than 3-character identifiers?

enum

bool turn = false;  // False for P1s turn, true for P2s.

We're writing in C++ here, not assembler. The turn = false seems more clunky than isP2Turn, and we shouldn't need to write a comment here.

Consider defining a pair of enum values, and storing them in a new turn or currentPlayer variable.

magic constant

int P1T = 1800;

Please write that as 30 * 60, so the comment just says "// seconds".

Many words start with "t". Couldn't we spell out "timer" here?

Similarly for variables like gameOver.

nit, typo: "sodder" --> "solder"

a boolean variable is a boolean expression

  if (GO = true)
  {
        ...
        if (GOI = false) {

Those should simply be if (GO), and if (!GOI).

I won't even ask why your pretty-printer decided that { brace sometimes goes on its own line and sometimes not.

local vars

  int tempMin = timer / 60;
  int tempSec = timer % 60;

These temp vars are local to timesub(). No need to give them a temp... prefix.

name

int timesub(int timer, HT16K33 player) {
  timer = timer - 1;

Maybe name it decrementTimer()? I wasn't reading "subtract" as producing a side effect.

And I thought timer-- was the conventional way to express that.

manifest constants

    PRH = (PRH + (millis() - sysTime));
    ...
    GO = true;

The ALL_CAPS is telling me that this is a constant. But then we assign a new value to it?!?

Please adopt more conventional spellings for identifiers. That gameActive flag, for example, looks great.

extract helper

There's a pair of if (secTime <= 0) clauses that are a bit long, and might benefit from being pushed down into a pair of helper functions.

Generally we try to ensure a function can be viewed in a single screenful without vertical scrolling. The loop() body is waaay past that, at this point.

units

Some times are in terms of seconds, others in milliseconds. Adopting a uniform naming convention that reveals that to maintenance engineers would be helpful.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.