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

odometer:
I was under the impression that code should, as a rule, be read slowly.

That is certainly not my view. If code is well written it should be as quick and easy to read as a page in one of Lee Child's books.

ChrisTenone:
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.

The 2-stage approach is excellent advice. However I'm not sure it would get much exposure in the Exhibitions section - maybe just put it into a separate Thread here - with a note to say that this is the discussion Thread.

...R

Regarding the variable typing, why don't you use both ways, one sort in the actual code either the _t variation or the 'normal' non consistent across different language versions and include the other behind there in the comments. Make sure that you are using the exact correct type, if it's a byte use a byte if it's an unsigned integer use an unsigned integer if it's a bool, use a bool. That way you explain both type names. I personally use the _t versions since like that i can actually see what the type is and how much i can store in there, and because just writing a 'u' in front is so much shorter than the word 'unsigned' which many times gets left out incorrectly therefore. About the scrolling, i come back to my earlier post and really i like my code and comments better separated. When i read code i actually don't want the comments cluttering it up, and if i have doubts i can look to the comments behind the lines. If there is a part of code that needs special introduction. I leave a line of space, Make the introduction, and write the space.
But in the end i think what you want to do is ask some inexperienced coders for their opinion rather than people who know 'better' make a few versions and present those to students and let them choose and see what they say about the why of their choice.

Robin2:
That is certainly not my view. If code is well written it should be as quick and easy to read as a page in one of Lee Child's books.
The 2-stage approach is excellent advice. However I'm not sure it would get much exposure in the Exhibitions section - maybe just put it into a separate Thread here - with a note to say that this is the discussion Thread.

...R

That would work, however keeping the thread active as a discussion would also keep it near the top. I was thinking that threads remain on page 1 longer in the Exhibition Gallery, but they also face the danger of being light sabered off.

A lot of code (not the OP's) reads like a run on sentence by William Faulkner - you know, the one that starts in one book, and finished in the next.

The balance between succinct and explanatory takes a bit of re-writing to achieve.

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.)

There is no rule that code should be read slowly. The best code reads like a favorite book. It is irony to have names so short that you need comments to explain what they are. If you want to read code, write clearly, as comments are not code. Code that is clean and easy to read benefits both novice programmers and experts. Also, the whole ticks thing is stupid!!!! You have ticks, a made up construct, and ONE_TICK, another made up construct, and then a flag, a completely unecessary variable, all for what? Why not update the display within the same condition check? Also, running everything inside the loop makes it difficult for novice programmers to know where they can add to your program. It is better to break everything into functions that not only explain what the function does without comments (simply by function name) but also allow the program to be easily modified or added to.

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

#include <LiquidCrystal.h>

// declare and initialize variables
unsigned long days;
byte hours;
byte minutes;
byte seconds;
byte deciseconds;
unsigned long updateTimestamp;
char buf[30]; // buffer for storing a string


const unsigned long OneDecisecond = 100000UL; // one decisecond in microseconds

// 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() {
  //syncTime();
  keepTime();
}

/*void synctime() {
  if (syncInput) {
    days = d;
    hours = h;
    minutes = m;
    seconds = s;
    deciseconds = 0;
    updateTimestamp = micros();
    updateTimestamp += OneDecisecond;
    updateDisplay();
  }
  }*/

void keepTime() {
  // check if it is time for the clock to tick
  if ((micros() - updateTimestamp) >= OneDecisecond) {
    updateTimestamp += OneDecisecond;
    // take care of all of the timekeeping math
    if (deciseconds >= 9) {
      deciseconds = 0;
      if (seconds >= 59) {
        seconds = 0;
        if (minutes >= 59) {
          minutes = 0;
          if (hours >= 23) {
            hours = 0;
            if (days >= 99999) days = 0;
            else days++;
          }
          else hours++;
        }
        else minutes++;
      }
      else seconds++;
    }
    else deciseconds++;
    updateDisplay();
  }
}

void updateDisplay() {
  // 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 ((seconds % 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, "%5ld%02uh%02um%02us%u", days, hours, minutes, seconds, deciseconds);
  lcd.print(buf);
}

Kinda watching this thread and it seems not one person has said you should walk or ride or kill the Equus asinus × Equus caballus, but rather to eschew obfuscation. You can toss the technical nomenclature out the fenestration !

But one for one, everyone has said to just make it understandable to a beginner.

the whole purpose of C is that you can use DoorSwitch and not have to address the port.

I would offer that when you post for review, ask the reviewer their programming level when they reply.

OK, here's what I have now.

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

// 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
int nowDays    = 0; // "days" part of elapsed time
int nowHours   = 0; // "hours" part of elapsed time
int nowMinutes = 0; // "minutes" part of elapsed time
int nowSeconds = 0; // "seconds" part of elapsed time
int nowTenths  = 0; // "tenths of seconds" part of elapsed time
unsigned long microsAtLastTenth = 0UL; // value of micros() at most recent 1/10 second

// a pretty important constant
const unsigned long MICROS_PER_TENTH = 100000UL; // number of microseconds per 1/10 second

// 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);
  
  // display the time (which at this point will be all zeros)
  updateTimeDisplay();
  
}

void loop() {
  
  // check if it is time for the clock to advance
  if ((micros() - microsAtLastTenth) >= MICROS_PER_TENTH) {
    
    // make the clock advance 1/10 of a second
    nowTenths++;
    
    // make sure that the next advance happens exactly when it is due
    // (they should happen exactly at intervals of 1/10 of a second)
    microsAtLastTenth += MICROS_PER_TENTH;
    
    // our clock needs to do more than count tenths of seconds
    // it also needs to count whole seconds, and minutes, and hours, and days
    // so let's go ahead and do that

    // too many tenths?
    if (nowTenths >= 10) {
      // trade 10 tenths for 1 second
      nowTenths -= 10;
      nowSeconds++;
    }
    
    // too many seconds?
    if (nowSeconds >= 60) {
      // trade 60 seconds for 1 minute
      nowSeconds -= 60;
      nowMinutes++;
    }
    
    // too many minutes?
    if (nowMinutes >= 60) {
      // trade 60 minutes for 1 hour
      nowMinutes -= 60;
      nowHours++;
    }
    
    // too many hours?
    if (nowHours >= 24) {
      // trade 24 hours for 1 day
      nowHours -= 24;
      nowDays++;
    }
    
    // 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

  // declare a buffer for storing a string (we'll need it later)
  // we expect to need only 16 characters, but to be safe, let's make room for 20
  // so we allow 20 characters worth of room, plus 1 extra for the null terminator
  // (Always allow 1 character extra worth of room for the null terminator!)
  char buf[21];

  // move to the top line of the display
  lcd.setCursor(0,0);
  
  // show the units
  lcd.print(" days  h  m  s  ");
  
  // convert the elapsed time to a string
  // for days, allow 5 digits worth of room
  // for hours, minutes, and seconds, allow 2 digits for each, and use leading zeros
  // for tenths, allow 1 digit worth of room
  sprintf(buf, "%5d %02d:%02d:%02d.%1d", nowDays, nowHours, nowMinutes, nowSeconds, nowTenths);

  // move to the bottom line of the display  
  lcd.setCursor(0,1);
  
  // show the string for the elapsed time
  lcd.print(buf);
}

And I think I'm about done with this stage. And like I said, the int for seconds, etc., would be helpful in case I end up needing to subtract time. (Want a countdown? Just change nowTenths++ to nowTenths-- and write conditionals to catch underflow!)

Personally, I prefer something like this

nowSeconds++;
if (nowSeconds >= 60) {
  nowSeconds -= 60;
  nowMinutes++;
}

to something like this

if (nowSeconds < 59) {
  nowSeconds++;
}
else {
  nowSeconds = 0;
  nowMinutes++;
}

because, with the former, it is easier to see why the conditional is the way it is: it works exactly like pen-and-paper arithmetic! If I modify my code to do a countdown, I expect I will use pen-and-paper-style "borrowing", as well.

The "pacing" - that is the white space vs comments vs code - is much improved! And thank you for eschewing the uint style types, and your choice of names reflect time spent thinking about the readability.

I'd give your project an A for the sketch (if you submitted it in my community college 'intro to technology' class.)

One nitpick: I usually increment the timing variable right after comparing it to the current time, just to make it's purpose clear. That is, I'd change:

...
  if ((micros() - microsAtLastTenth) >= MICROS_PER_TENTH) {
    nowTenths++;
    microsAtLastTenth += MICROS_PER_TENTH;
    ...

to:

...
  if ((micros() - microsAtLastTenth) >= MICROS_PER_TENTH) {
    microsAtLastTenth += MICROS_PER_TENTH;

    nowTenths++;
    ...

I have one question left though, and you may have already answered it, I can't remember:

Since the shortest interval you deal with is a tenth of a second, why do you use microseconds, rather than milliseconds? If you are considering extending this to hundredths, I have found that similar code fails to keep up around 1/100sec (while using a I2C oled), so you might need to change the logic to accomplish that anyway.

ChrisTenone:
I have one question left though, and you may have already answered it, I can't remember:

Since the shortest interval you deal with is a tenth of a second, why do you use microseconds, rather than milliseconds?

This way, you have better control over the speed of the clock.

I have set MICROS_PER_TENTH equal to 100000.
If the clock runs slow 1 minute per day, I compensate by setting MICROS_PER_TENTH to 99931.
If the clock runs fast 1 minute per day, I compensate by setting MICROS_PER_TENTH to 100069.
If I want a sidereal clock, I set MICROS_PER_TENTH to 99727.
If I want a Mars clock, I set MICROS_PER_TENTH to 102749.

If you are considering extending this to hundredths, I have found the code fails to keep up around 1/100sec (while using a I2C oled), so you might need to change the logic to accomplish that anyway.

That reminds me of an experiment I did to see the speed of the display (and driver library) I am using. I concluded that I could update one line in just under 1/200 of a second. For now, though, I think I'll stick to displaying tenths.

odometer:
This way, you have better control over the speed of the clock.

I have set MICROS_PER_TENTH equal to 100000.
If the clock runs slow 1 minute per day, I compensate by setting MICROS_PER_TENTH to 99931.
If the clock runs fast 1 minute per day, I compensate by setting MICROS_PER_TENTH to 100069.
If I want a sidereal clock, I set MICROS_PER_TENTH to 99727.
If I want a Mars clock, I set MICROS_PER_TENTH to 102749.

That reminds me of an experiment I did to see the speed of the display (and driver library) I am using. I concluded that I could update one line in just under 1/200 of a second. For now, though, I think I'll stick to displaying tenths.

You might want to note that the function micros() always returns a value divisible by 4. It works fine for the nominal value, but not for the slow, fast, sidereal or Mars clocks... If you want precise timekeeping, it's better to replace the crystal ultimately keeping time than to try and make up for it's inaccuracies in software.

Perehama:
If you want precise timekeeping,

AFAIK this proposed Tutorial is not about precise timekeeping. I am inclined towards @ChrisTenone's view that millis() will be perfectly good enough and I reckon it will be less confusing for a beginner.

...R

Robin2:
AFAIK this proposed Tutorial is not about precise timekeeping. I am inclined towards @ChrisTenone's view that millis() will be perfectly good enough and I reckon it will be less confusing for a beginner.

I know I asked for feedback, but at this point I feel as though the sketch isn't really mine any more.

Seriously, I wonder:
Is there some standard example sketch that has essentially the functionality of this sketch (except perhaps omitting tenths of seconds)? If not, why not? It would have taken no more effort to write such a sketch than to make so many comments on mine.

It seems that the same sketch, with only minor modifications (at least, to me they would appear minor), could serve as uptime meter, stopwatch, countdown timer, and time-of-day clock. Want an uptime meter? Just leave the sketch as-is. Want a stopwatch? Add a button, and have that button toggle whether or not the nowTenths++; happens. Add another button to set the time to zero. Want a countdown timer? Change nowTenths++; to nowTenths--; , and the arithmetic carries to borrows. Have a button increment the minutes. Want a clock? Keep the nowTenths++; as is, and add two buttons, one for advancing the minutes, the other for advancing the hours.

Maybe I shouldn't have said "beginner-friendly". I certainly don't intend to do any map() or bitRead()-level handholding. But I'm also not going to be trying any shenanigans like:

uint32_t timeAdd (uint32_t x, uint32_t y) {
  // no sanity checking of input
  // format is hhmmssff with ff being decimal fractions of a second
  // "out of range" results are e.g. A0000000 for 100 hours
  uint32_t binsum = x + y;
  uint32_t carry = ((binsum + 0x06A6A666) ^ x ^ y) & 0x11111110;
  return (binsum + ((carry - (carry>>4)) & 0x06A6A666));  
}

which is the kind of time manipulation that feels right to me. (I'm not going to try to sell you on it; I'm just going to say that to me it feels right, just like how to me RPN feels right for a calculator.)

What I meant by "beginner-friendly" was more like "not outright hostile to beginners".

Perehama:
You might want to note that the function micros() always returns a value divisible by 4. It works fine for the nominal value, but not for the slow, fast, sidereal or Mars clocks... If you want precise timekeeping, it's better to replace the crystal ultimately keeping time than to try and make up for it's inaccuracies in software.

That's why I wrote this:

    // make sure that the next advance happens exactly when it is due
    // (they should happen exactly at intervals of 1/10 of a second)
    microsAtLastTenth += MICROS_PER_TENTH;

and not this:

    // make sure that the next advance does not happen too soon
    // (it might happen a bit late, throwing off my timekeeping, but I don't really care)
    microsAtLastTenth = micros();

There is a reason I did not slavishly copy the Blink Without Delay logic.

I'm also not going to be trying any shenanigans like

Your shenanigans generate nearly twice as much code as the the string of ifs... :slight_smile:
(On an 8bit AVR.)

westfw:
Your shenanigans generate nearly twice as much code as the the string of ifs... :slight_smile:
(On an 8bit AVR.)

The point of the "shenanigans", so to speak, is to allow things like

if (bcdTime >= 0x22300000UL) {
  // turn off the lights at half-past ten
  digitalWrite(LIGHTS_PIN, LOW);
}

in which the time is one number, and you can read that number, and more or less deal with it the way you would think you would deal with it.
It also exactly matches the way that a DS3231 keeps time, except that the DS3231 is little-endian and doesn't do fractional seconds.

odometer:
The point of the "shenanigans", so to speak, is to allow things like

if (bcdTime >= 0x22300000UL) {

// turn off the lights at half-past ten
 digitalWrite(LIGHTS_PIN, LOW);
}

Shouldn't that be

if (bedTime >= 0x22300000UL) {

More seriously. why on earth would anyone want to encode half-past-ten like that? If you want a program that allows a user to enter times in a familiar format then deal with that as part of the user-input code and then convert the input into a single time value that represents the number of seconds, or tenths of seconds since some datum in the past.

The more this Thread continues the less coherent the concept of this tutorial seems - extra things are dragged in all the time, and I am not referring to comments by the responders. To my mind an effective tutorial has a single clear objective and everything peripheral to that is omitted.

...R

Robin2:
More seriously. why on earth would anyone want to encode half-past-ten like that? If you want a program that allows a user to enter times in a familiar format then deal with that as part of the user-input code and then convert the input into a single time value that represents the number of seconds, or tenths of seconds since some datum in the past.

Maybe I'm not having a user enter any times. Maybe I'm just hard-coding some times in, and I want to be able to see -- that's the key word here, see -- what my code is doing.

I am human. I am like nearly all humans in that it is not immediately obvious to me that 81000 seconds past midnight is exactly the same moment as half-past ten in the evening. And really, I don't care worth a worm-eaten fig how many or how few seconds, milliseconds, or microseconds are between the time I care about and some epoch. Maybe if I had world-class skill at mental arithmetic I would be happy dealing with times as raw seconds. But I don't, and I'm not.

That will have to do for an explanation.

odometer:
-- that's the key word here, see -- what my code is doing.

In that case just enter the times as characters in a char array -

char bedTime[] = "22:30:00";

...R