Trying to make code beginner-friendly -- how's this for style?

Projects General Guidance
1

Some time ago, I made a two-button clock project:
https://forum.arduino.cc/index.php?topic=408565.0
I used a coding style (numbers in BCD, lots of bit-masks) which "feels" right to me, but is very beginner-unfriendly. So, now I wish to build up essentially the same project, in smaller steps, and with a simpler, more beginner-friendly coding style.

This first stage just counts time since the Arduino was most recently powered on (or reset).

// robs_clk_pt1.ino
// by odometer  2018-12-06

#include <LiquidCrystal.h>

// declare and initialize variables
uint16_t dd = 0; // number of days
uint8_t hh = 0; // number of hours
uint8_t mi = 0; // number of minutes
uint8_t ss = 0; // number of seconds
uint8_t ticks = 0; // number of ticks
uint32_t lastTickMicros = 0UL; // value of micros() at most recent tick
uint8_t updateFlag = 1; // whether to update the display (0 means no, 1 means yes)
char buf[30]; // buffer for storing a string

// a pretty important constant
const uint32_t ONE_TICK = 100000UL; // this number controls the speed of our clock

// here we specify what pins our LCD is on
//                RS  EN  D4  D5  D6  D7
LiquidCrystal lcd(12, 11,  5,  4,  3,  2);

void setup() {
  // start the LCD going
  lcd.begin(16, 2);
}

void loop() {
  // check if it is time for the clock to tick
  if ((micros() - lastTickMicros) >= ONE_TICK) {
    // make the clock tick forward
    ticks++;
    // space the clock ticks exactly 1/10 second apart
    lastTickMicros += ONE_TICK;
    // take care of all of the timekeeping math
    if (ticks >= 10) {
      // trade 10 ticks for 1 second
      ticks -= 10;
      ss++;
    }
    if (ss >= 60) {
      // trade 60 seconds for 1 minute
      ss -= 60;
      mi++;
    }
    if (mi >= 60) {
      // trade 60 minutes for 1 hour
      mi -= 60;
      hh++;
    }
    if (hh >= 24) {
      // trade 24 hours for 1 day
      hh -= 24;
      dd++;
    }
    // remind ourselves that it is time to update the display 
    updateFlag = 1;
  }
  
  // check if it is time to update the display
  if (updateFlag == 1) {
    // update the display
    //
    //   Position: 01234567890123456
    //     Line 0: robs_clk_pt1.ino
    // or, Line 0: So far it's been
    //     Line 1: 00000d00h00m00s0
    lcd.setCursor(0,0);
    // top line of display: alternate messages every 5 seconds
    if ((ss % 10) < 5) {
      lcd.print("robs_clk_pt1.ino");
    }
    else {
      lcd.print("So far it\'s been");
    }
    lcd.setCursor(0,1);
    // bottom line of display: show elapsed time
    sprintf(buf, "%5dd%02dh%02dm%02ds%d", dd, hh, mi, ss, ticks);
    lcd.print(buf);
    updateFlag = 0;
  }  
}

I am not 100% sure about my explanatory comments, though. I want them to be short enough to hold the reader's attention, but not so short as to leave her scratching her head.

The main thing I am having trouble with is that big ol' sprintf() statement. I am not sure how to comment it, or even whether to try.

I am aware that, instead of the sprintf() statement, I could use lcd.print() to print one number at a time. But, let's face it: lcd.print() leaves something to be desired. More details on that "something" here: Support for leading zeros in Serial.print, etc. - Suggestions for the Arduino Project - Arduino Forum

What should I do?

2

odometer:
What should I do?

The very first thing is to click Report to Moderator and ask to have your Thread moved to a more appropriate section. The Introductory Tutorials section clearly says that questions should NOT be posted in it.

...R

3

Seeing how sprintf is a core function, are you writing a demo program or are you explaining core functionality? Make a decision and stick to it. Trying to be all things to all people is a receipe for ineffective tutorials.

There are tons of tutorials available for core functions. Provide some links and leave it there.

uint16_t dd = 0; // number of days
uint8_t hh = 0; // number of hours
uint8_t mi = 0; // number of minutes
uint8_t ss = 0; // number of seconds

IMO, that’s not a good way to introduce newbies to variable naming...

4

It needs an introductory big comment at the top saying what the purpose of the program is.

I also don’t like your variable names.

5

odometer:
Some time ago, I made a two-button clock project:

I used a coding style (numbers in BCD, lots of bit-masks) which "feels" right to me, but is very beginner-unfriendly. So, now I wish to build up essentially the same project, in smaller steps, and with a simpler, more beginner-friendly coding style.

What exactly do you want the student to gain from your tutorial?

Is it "how to use an LCD"?

OR,

Is it "how to write code to calculate time"

Whichever it is, it seems to me that the other part is just a confusion.

And I, also, don't like your abbreviated variable names. IMHO long descriptive names make it possible to read a program like story. For example secondsValue rather than ss

...R

6

Robin2:
What exactly do you want the student to gain from your tutorial?

Is it "how to use an LCD"?

OR,

Is it "how to write code to calculate time"

Whichever it is, it seems to me that the other part is just a confusion.

It's "how to make a rudimentary clock". It's not much of a clock if you can't see what time it is.
Actually, now that I think about it, maybe I should pull out the "display the time" part into its own function.

I know that the code I posted is for a count-up timer rather than a clock. The first step in my tutorial will be to make a count-up timer. In the next step, I will add buttons to allow the time to be set, thus turning it into a clock.

And I, also, don't like your abbreviated variable names. IMHO long descriptive names make it possible to read a program like story. For example secondsValue rather than ss

...R

I see your point. But I don't want my code to end up like this, either: https://www.youtube.com/watch?v=nJ8Tq5EotaE

Would day, hour, minute, and second work? Or would they conflict with some reserved words or something? I know that min for minutes wouldn't work, because min is used for something else in Arduino-land.

7

odometer:
I see your point. But I don't want my code to end up like this, either:

Isn't that a bit of an exaggeration?

I see no value in trying to make variable names short - especially as programs get longer.

...R

8

I usually try and not reserve separate lines for comments but put them after the condition or the statement

    if (mi >= 60) {      // trade 60 minutes for 1 hour
      mi -= 60;
      hh++;
    }
    if (hh >= 24) {            // trade 24 hours for 1 day
      hh -= 24;
      dd++;
    }    
    updateFlag = 1;            // remind ourselves that it is time to update the display 
  }

as to make it clear what is the code and that the comments are just that.

9

Reply #7 brings to mind a preference of mine for indenting comments more than the code - like this

     // space the clock ticks exactly 1/10 second apart
lastTickMicros += ONE_TICK;
    // take care of all of the timekeeping math
if (ticks >= 10) {

because then I can easily read the code while ignoring the comments.

...R

10

OK, this is starting to turn into something like this: The Man, the Boy, and the Donkey - Collection at Bartleby.com

Which is why I've settled on this code:

// how_long_running.ino
// by odometer  2018-12-10

// This sketch answers the question: "How long have I been running?"
// The answer is shown on a 16x2 LCD character display.

#include <LiquidCrystal.h>

// declare and initialize variables
uint16_t nowDay = 0; // "days" part of elapsed time
uint8_t  nowHr  = 0; // "hours" part of elapsed time
uint8_t  nowMin = 0; // "minutes" part of elapsed time
uint8_t  nowSec = 0; // "seconds" part of elapsed time
uint8_t  nowTick = 0; // "ticks" part of elapsed time
uint32_t lastTickMicros = 0UL; // value of micros() at most recent tick
char buf[30]; // buffer for storing a string

// a pretty important constant
const uint32_t ONE_TICK = 100000UL; // this number controls the speed of our clock

// here we specify what pins our LCD is on
//                RS  EN  D4  D5  D6  D7
LiquidCrystal lcd(12, 11,  5,  4,  3,  2);

void setup() {
  // start the LCD going
  lcd.begin(16, 2);
}

void loop() {
  // check if it is time for the clock to tick
  if ((micros() - lastTickMicros) >= ONE_TICK) {
    // make the clock tick forward
    nowTick++;
    // space the clock ticks exactly 1/10 second apart
    lastTickMicros += ONE_TICK;
    // take care of all of the timekeeping math
    if (nowTick >= 10) {
      // trade 10 ticks for 1 second
      nowTick -= 10;
      nowSec++;
    }
    if (nowSec >= 60) {
      // trade 60 seconds for 1 minute
      nowSec -= 60;
      nowMin++;
    }
    if (nowMin >= 60) {
      // trade 60 minutes for 1 hour
      nowMin -= 60;
      nowHr++;
    }
    if (nowHr >= 24) {
      // trade 24 hours for 1 day
      nowHr -= 24;
      nowDay++;
    }
    // update the display so we can see the new time
    updateTimeDisplay();
  }
}


void updateTimeDisplay () {
  // function to update the display to show the current elapsed time
  // 
  // we want the display to look like this
  //   Position: 01234567890123456
  //     Line 0:  days  h  m  s  
  //     Line 1:     0 00:00:00.0
  //
  // on the top line of display, show the units
  lcd.setCursor(0,0);
  lcd.print(" days  h  m  s  ");
  // on the bottom line of the display, we show the elapsed time
  // first, we compose a string showing the elapsed time
  sprintf(buf, "%5d %02d:%02d:%02d.%1d", nowDay, nowHr, nowMin, nowSec, nowTick);
  // then we display that string on the bottom line of the display    
  lcd.setCursor(0,1);    
  lcd.print(buf);
}

(Maybe I'll put in one or two more lines of comments explaining that sprintf statement, but that's about it.)

In which sub-forum should this project go? This sketch is going to be the first in a series in which I build up to making a clock with calendar, hourly chime, and buttons for setting the time, as well as an option to use an external RTC.

11

odometer:
Which is why I've settled on this code:

You are quite correct to realize that YOU must the final decision.

Nevertheless I find it strange that an item named ONE_TICK actually has a value of 100,000 :slight_smile:

...R

12

Robin2:
You are quite correct to realize that YOU must the final decision.

Nevertheless I find it strange that an item named ONE_TICK actually has a value of 100,000 :slight_smile:

...R

That's because the clock is to tick 10 times per second. Actually, I might be able to justify making it 100 times per second (and changing ONE_TICK to 10000), but well, 10 is what I settled on.

At the very least, the finished clock will need to keep track of half-seconds. When I get to the code for making it chime, you will see why.

13

odometer:
That's because the clock is to tick 10 times per second. Actually, I might be able to justify making it 100 times per second (and changing ONE_TICK to 10000), but well, 10 is what I settled on.

You missed my point completely - which probably reinforces my point.

It was the name of the constant that I was commenting on, not the chosen value. In my universe calling it millisForOneTick would be more meaningful.

...R

14

Why is buf global? Shouldn't it be a local in updateTimeDisplay?

15

This just doesn't look like good beginner's code.

"Unix style" types such as uint8_t are more intimidating than byte, and unsigned long. Any abbreviations in variable or function names can be ambiguous, especially for non-English as a first people.

And maybe make the entire sprintf thing something for lesson 2. The F macro might be a good thing to introduce if you do explicit print statements. (Memory management is a beggining Arduino topic.)

And a bit of white space always seems to friendly the structure up a bit. For example:

void loop() {

  // check if it is time for the clock to tick
  if ((micros() - lastTickMicros) >= ONE_TICK) {

    // make the clock tick forward
    nowTick++;

    // space the clock ticks exactly 1/10 second apart
    lastTickMicros += ONE_TICK;

    // take care of all of the timekeeping math
    if (nowTick >= 10) {
      // trade 10 ticks for 1 second
      nowTick -= 10;
      nowSec++;
    }

    if (nowSec >= 60) {
      // trade 60 seconds for 1 minute
      nowSec -= 60;
      nowMin++;
    }

    if (nowMin >= 60) {
      // trade 60 minutes for 1 hour
      nowMin -= 60;
      nowHr++;
    }

    if (nowHr >= 24) {
      // trade 24 hours for 1 day
      nowHr -= 24;
      nowDay++;
    }

    // update the display to see the new time
    updateTimeDisplay();
  }

This allows the student to see every part of the code independently from the rest, while maintaining the sequential nature of the flow. I also am not a big fan of using "we" in comments. It's kitschy.

16

ChrisTenone:
This just doesn't look like good beginner's code.

"Unix style" types such as uint8_t are more intimidating than byte, and unsigned long.

Maybe I should just give in and say int. I know I'd be using up an extra byte of RAM per variable, but at least that way I will be able to catch underflow. Useful in case I need to subtract time.

Any abbreviations in variable or function names can be ambiguous, especially for non-English as a first people.

If I give in and just use hour, minute, and second as variable names, will that collide with anything?

And maybe make the entire sprintf thing something for lesson 2.

The reason I use sprintf() is because the way lcd.print() handles numbers is like a banana peel on the floor.
Someone slipping on the banana peel: integer 1 written as 01 - Programming Questions - Arduino Forum
"So, walk around the peel!" integer 1 written as 01 - #13 by GypsumFantastic - Programming Questions - Arduino Forum

The F macro might be a good thing to introduce if you do explicit print statements. (Memory management is a beggining Arduino topic.)

Maybe if later I decide to add a lot of strings (the time in words, for example), then I will use the F() macro. But not yet.

And a bit of white space always seems to friendly the structure up a bit.

Sure, if you like scrolling.
I could go back and forth all day on how much whitespace to use, but really, I just want to get this done. I don't want to be like someone who shows up an hour late for a date because they couldn't make up their mind what to wear.
I think there's a saying, "Perfect is the enemy of finished", or something like that. And now I have to finish up this post, because "real life" calls.

17

odometer:
really, I just want to get this done.

I think there's a saying, "Perfect is the enemy of finished",

I understand the conflict.

However I'm not convinced that you have given sufficient thought to the capabilities (or lack of them) of your target audience.

I have seen textbooks with a preface that says "this book is written for this [type of audience] and it assumes that the reader already knows [this stuff]

It may be useful if you write the equivalent just for your own information - I'm not expecting you to publish it here if you don't want to.

...R

18

Actually counters and timing calculations are my favorite things.

Maybe I should just give in and say int.

Beginners like readability, and they appreciate explanation of abbreviations and clear coding - uint8_t means the same as byte. I find byte to be readily understood by anyone. Using int would be introducing bad coding practice - if you only need an 8 bit quantity, then use an 8 bit container. uint8_t looks a lot like uint16_t and uint32_t unless you read slowly to dig out the numerals in the word, so I don't use them, there is no difference in the compiled code. Bytes are always unsigned, so you don't need to specify. 16 and 32 bit integers need the unsigned, so the uint#_t (unsigned integer # bits type) nomenclature are used to save programmers a few keystrokes. But beginners don't need that.

If I give in and just use hour, minute, and second as variable names, will that collide with anything?

Those words turn a color, so I use minute[i]s[/i], second[i]s[/i], etc.

Sure, if you like scrolling. ...

You can reduce scrolling (well, vertical scrolling) by eliminating all the white space and comments ...
#include <LiquidCrystal.h> uint16_t nowDay = 0;uint8_t nowHr = 0; uint8_t nowMin = 0; uint8_t nowSec = 0; uint8_t nowTick = 0; uint32_t lastTickMicros = 0UL; char buf[30]; const uint32_t ONE_TICK = 100000UL; LiquidCrystal lcd(12, 11,  5,  4,  3,  2); void setup(){lcd.begin(16, 2);} void loop(){if ((micros() - lastTickMicros) >= ONE_TICK){nowTick++; lastTickMicros += ONE_TICK; if(nowTick >= 10){nowTick -= 10;nowSec++;} if (nowSec >= 60) {nowSec -= 60;nowMin++;} if (nowMin >= 60) {nowMin -= 60;nowHr++;}if (nowHr >= 24){nowHr -= 24;nowDay++;} updateTimeDisplay();}} void updateTimeDisplay(){lcd.setCursor(0,0); lcd.print(" days  h  m  s  "); sprintf(buf, "%5d %02d:%02d:%02d.%1d", nowDay, nowHr, nowMin, nowSec,nowTick); lcd.setCursor(0,1); lcd.print(buf);}Heh. :wink:

Perfect is the enemy of finished"

I heard it was "Perfect is the enemy of meh."

All in all though, thanks for working on this topic. Despite my critique, it's looking good.

19

ChrisTenone:
uint8_t looks a lot like uint16_t and uint32_t unless you read slowly to dig out the numerals in the word, so I don't use them, there is no difference in the compiled code.

I was under the impression that code should, as a rule, be read slowly. (This is also one reason I don't like long variable names. I don't want to read names; I want to read code.)

16 and 32 bit integers need the unsigned, so the uint#_t (unsigned integer # bits type) nomenclature are used to save programmers a few keystrokes. But beginners don't need that.

At least those names tell you what you've got.
When I am given the choice of a "small" or "large" beverage, I have to ask "How big is a small?"

All in all though, thanks for working on this topic. Despite my critique, it's looking good.

In which sub-forum should the thread for the project go?
By this I do not mean: where should this discussion thread go?
What I mean is: where should the thread for the project itself go?

20

odometer:
I was under the impression that code should, as a rule, be read slowly. (This is also one reason I don't like long variable names. I don't want to read names; I want to read code.)

You are writing this for beginners, remember? They want the code to tell them what it's doing.

At least those names tell you what you've got.

They read like gobbledegook to me. (5R)-[(1S)-1,2-dihydroxyethyl]-3,4-dihydroxyfuran-2(5H)-one tells me what I've got. Vitamin C tells you what it is.

When I am given the choice of a "small" or "large" beverage, I have to ask "How big is a small?"

If you ever get a chance, check out this newfangled coffee shop, Starbucks...

In which sub-forum should the thread for the project go?
By this I do not mean: where should this discussion thread go?
What I mean is: where should the thread for the project itself go?

I would say that after you have finished it, put it in the Exhibitions sub-forum. Then ask people to review it, and don't make real-time changes based on the comments. After some time, redo the whole thing based on it's reception by the beginner community. Ask if that version should go into the Tutorials section.