Need guidance on programming technique

I received my Arduino Uno 2-15-2014. I did a bunch of examples, modifying, screwing them up and fixing them.

I got confident enough to start on my Grand Aquarium Control project for my saltwater tank.

The object is to control Aquarium Level, Sump Level, RO/DI water Level, and temperature with fault-tolerant heaters.

I have the first part done, tested, and in production. The temperature/heaters part seems to be working, but I’m not confident enough to put that part into production yet and the code is a bit of a scrambled mess. Not something to be particularly proud of.

My problem is that my code is all “brute force”. The sketch is 700 + lines and I’m not done yet.

Having maintained computer systems, I insist on easily understood code that can be readily maintained/modified.

Can some of youse guys give me some guidance, helpful hints? Rewrite it for me? Well, maybe not the last thing.

Remember kindness is a virtue to be admired by one and all.

It’s too big to include. I attached it below. Thanks.

John

aquarium_CurrentDevelopment2.ino (28.6 KB)

tempSensorA = (sensors.getTempFByIndex(0));

The outer parenthesis aren't needed.

now() > (timeHeaterCOn + 180)

You use addition to check for elapsed times in various places. This doesn't work correctly when the timer rolls over. If you use subtraction it handles rollover correctly i.e.:

((now() - timeHeaterCOn) > 180)

Millis and micros values should be held as unsigned long values.

The code structure looks generally OK.

There seem to be lots of sets of compound conditions which are non-trivial (the comments help a lot) and I'd look for ways to simplify them. State machines can help a lot here especially when you're combining thresholds and time comparisons like this looking for a relatively small set of events.

There's a fair chunk of code checking whether the display data has updated. There are a few ways to simplify that. One way is to define a struct to hold the set of data being displayed, so that you can do a comparison of the whole set in a single comparison. Another is to decide that you will just update the display every second, or every ten seconds, or whatever.

With a sketch this big I would be tempted to separate the different functional areas into separate modules (i.e. a .cpp file and .h file) containing the functions, types and data specific to that functional area. Just to reduce the amount of data and code that needs to be thought about at a time. (You may not have any trouble with it today when it's fresh in your mind, but six months down the road it will seem much harder to get your head around the complexity.)

Not sure what you are doing here but your indentation suggests you want to execute the two lines if the condition is true.
At the very end of the sketch:
if (flag_AquariumSensorDeviation == HIGH)
lcd.setCursor(0,0);
lcd.print("Temp Sensor Deviate");
Is not the same as:
if (flag_AquariumSensorDeviation == HIGH)
{
lcd.setCursor(0,0);
lcd.print("Temp Sensor Deviate");
}

Use the F-macro…

lcd.print("Temp Sensor Deviate");

Becomes…

lcd.print( F( "Temp Sensor Deviate" ) );

This section is a perfect candidate for a separate function (heaterList[1] would be passed as a parameter)...

      switch (heaterList[1])                        // Turn on first heater in the list
      {
         case 'A':
            digitalWrite(HeaterC,HIGH);             // turn on the heater
            timeHeaterCOn    = now();               // save time started
            timeHeaterCOff   = 0;                   // clear time stopped
            stateHeaterC     = HIGH;                // save state of heater
            break;
        case 'B':
            digitalWrite(HeaterD,HIGH);             // turn on the heater
            timeHeaterDOn    = now();               // save time started
            timeHeaterDOff   = 0;                   // clear time stopped
            stateHeaterD     = HIGH;                // save state of heater
            break;
        case 'C':
            digitalWrite(HeaterE,HIGH);             // turn on the heater
            timeHeaterEOn    = now();               // save time started
            timeHeaterEOff   = 0;                   // clear time stopped
            stateHeaterE     = HIGH;                // save state of heater
            break;
      }

LarryD

Great save. Thanks. Bad structure on the "if" caused me to "perhaps" set the cursor and then print the line regardless of anything.

I fixed it.

John

You can compare blocks of memory using memcmp() from string.h.
The memXXX() functions are nice and clean.

Thanks for your input. It is helpful.

PeterH:

tempSensorA = (sensors.getTempFByIndex(0));

The outer parenthesis aren't needed.

Fixed.

PeterH:

now() > (timeHeaterCOn + 180)

You use addition to check for elapsed times in various places. This doesn't work correctly when the timer rolls over. If you use subtraction it handles rollover correctly i.e.:

((now() - timeHeaterCOn) > 180)

Fixed.

PeterH:
Millis and micros values should be held as unsigned long values.

Fixed. None of the timing is at all critical. The "180" should be a constant up at the top where it can easily be modified.
Your info is good to know for future projects where timing may be critical.

PeterH:
The code structure looks generally OK.

There seem to be lots of sets of compound conditions which are non-trivial (the comments help a lot) and I'd look for ways to simplify them. State machines can help a lot here especially when you're combining thresholds and time comparisons like this looking for a relatively small set of events.

There's a fair chunk of code checking whether the display data has updated. There are a few ways to simplify that. One way is to define a struct to hold the set of data being displayed, so that you can do a comparison of the whole set in a single comparison. Another is to decide that you will just update the display every second, or every ten seconds, or whatever.

With a sketch this big I would be tempted to separate the different functional areas into separate modules (i.e. a .cpp file and .h file) containing the functions, types and data specific to that functional area. Just to reduce the amount of data and code that needs to be thought about at a time. (You may not have any trouble with it today when it's fresh in your mind, but six months down the road it will seem much harder to get your head around the complexity.)

I will put some effort into that. Those are both areas I am struggling with.

Does the overall structure seem appropriate?

John

const int switchLowerSump        = 9;

"const byte" or "const uint8_t"

int stateAqOverflow;

With microcontrollers, get used to using the sensible-sized memory allocations - it seems to me unlikely that the overflow could be negative, or assume one of 65536 different states.

int flag_AquariumSensorDeviation = LOW;  // Error Flags

Flags are normally true or false, and again, rarely require negative or requiring 65536 distinct states.

long timeErrorOccurred      = 0;

It already is zero - no need for you to tell the compiler what it already knows.

char  heaterList[6] = "xABCx";

Again, the compiler knows how big the string is; no need to tell it.

time_t pctime = (1262347200);     // this sets the clock to some arbitrary date, I forget what date.

I don't know what function the parentheses serve here, but a "UL" or "L" suffix (I can't remember if "time_t" is signed or not) wouldn't go amiss.

if (stateAqOverflow ==  LOW)                   // if Overflow float is low

Get used to missing out the comments if the code is sufficiently descriptive.

     tempSensorA = (sensors.getTempFByIndex(0));    // Read all the temperature sensors
     tempSensorB = (sensors.getTempFByIndex(1));
     tempSensorC = (sensors.getTempFByIndex(2));
     tempSensorD = (sensors.getTempFByIndex(3));
     tempSensorE = (sensors.getTempFByIndex(4));

Screams "array and for loop".

if ((tempSensorD > tempSensorHighest)      &&    // It's higher
       (stateHeaterD = LOW)                   &&

Go directly to jail, do not pass GO, do not collect $200.

heaterList[4]  = heaterList[1];           // Rotate the heater list
          heaterList[1]  = heaterList[2];           //  using 4 as a work space
          heaterList[2]  = heaterList[3];
          heaterList[3]  = heaterList[4];

Never shuffle memory around - move the pointers, do some modulo arithmetic.

heaterList[4]  = heaterList[1];           // Rotate the heater list
          heaterList[1]  = heaterList[2];           //  using 4 as a work space
          heaterList[2]  = heaterList[3];
          heaterList[3]  = heaterList[4];

So now you copied heaterList[ 1 ] to both [ 4 ] and [ 3 ].

You can use a variable index to start at any one of those and work the set round robin by increment then AND with 3, 4 times in a loop.

Hi John,

You have had a lot of useful advice already.

The only thing I would take issue with is the suggestion to use additional .cpp or .h files. I agree that it acn be useful to divide the code into different files to keep similar stuff together and to keep diffierent stuff in a different place. Just use extra .ino files for this. Put them in the same directory as the main sketch. After it loads the main .ino file the IDE loads the other .ide files in alphabetical order. What I object strongly to about the traditional C/C++ approach is that it splits related material into different files which makes it much harder to follow the logic 6 months after you have written it.

I came across this acronym "DRY" (don't repeat yourself) in the context of Ruby on Rails which was invented by a very clever guy. It just means don't write the same code in two places - because, eventually, you will update one copy and forget to do the others.

You have a lot of repetitive code dealing with sensors and heaters that IMHO would be much shorter and clearer if it used arrays rather than different variable names such as SensorA, SensorB etc. It also looks like there may be some correspondence between (eg) SensorA and HeaterA which should allow even more code simplification if arrays are used. However to produce readable code using arrays would involve a big rewrite and a full understanding of the system which I don't have. For example, maybe there should be an array of Tanks with each tank having a sensor element, a heater element, max and min temperatures, water level, pump on/off etc.

I could also see that it might be useful to use a Struct to organize the data elements for each tank.

I have a strong feeling (with no evidence :)) that there is unnecessary complexity in the totality of the IF decision making process. Again it needs the ability to understand the whole project before being able to assess that properly. In any case it might look simpler if arrays and/or structs had been used.

I don't have an LCD so I haven't looked at that part of your code.

...R

As mentioned, your flag variables could be booleans. Which means that stuff like this:

   if (prevSensorE               != tempSensorE ||
       displayFirstTime          == LOW)

would become

   if (prevSensorE               != tempSensorE  ||        !displayFirstTime )

LOW may happen to be zero, but it doesn't really feel right outside of a context where digitalRead was being used.

Using a func_ prefix on your functions seems unnecessary.

Could you attach a new version with the changes you've made?

Edit: logic error

Most of what I was going to mention has already covered. (Paying attention to what datatype is really necessary for the intended range of values for each variable when declaring them, and using the F() macro to preserve memory with all your long texts.)

I’m not convinced you need to use the time library. You seem to just be using relative timing, not absolute timing. Do you plan on adding date and time to your readout and incorporating a battery backed RTC (I wouldn’t expect any of the Arduinos to not drift over time) some time in the future? If not, I’d probably drop the overhead of the time.h library and change all your calls to now() with millis(). And remember to change your timing delays to be based off of millisecond counting instead of second counting. (Basically multiply all your delays by 1000.)

I did find a case where your comments are out of synch (took me a reading double-take…) with what the code is doing. When you are checking how far low the temperatures are, your comments all state // Turn on first heater in the list. While that is true when the temperatures are low by 0.5, it isn’t for the other temperature deltas. Change your comment for 0.7 low (on line 444 of your originally posted code) to // Turn on second heater in the list, and the comment for 0.9 low (on line 469 of your originally posted code) to // Turn on third heater in the list. Probably just a case of not finishing making changes after a copy-paste. (I often make that error myself, so I’m keyed to watch for it.)

I notice an inconsistency with your currently implemented heater names and your heater list. You have heater names C,D,E but your heater list uses A,B,C. Heater list should use C,D,E (initialized as "xCDEx").

If you change your heater and temp sensor names from A,B,C,D,E to 1,2,3,4,5 (or for a little memory savings 0,1,2,3,4) then you can use the name as a direct index (and can make a checking if the temp sensor is a heater sensor a simple numeric comparison). If you change to numbers (lets use 1,2,3,4,5), then you will have Heater[3],Heater[4],Heater[5]. (Yes, Heater[0] through Heater[2] will be wasted memory not used for anything, but it does keep parity between heater names and temp sensor names and you should by now have saved plenty of memory by changing lots of variables from ints to bytes, implementing the F() macro, and dropping (possibly) the time.h library.) Then heaterList[0] starts as 3, heaterList[1] starts as 4, and heaterList[2] starts as 5. And it then follows to rotate the usage of your heaters you can use this to shift the values:

for(byte i=0; i<3; i++)
{
  heaterList[i]++;
  if(heaterList[i]>5)
  {
    heaterList[i]=3;
  }
}

I do agree with others that the massive case statements with identical actions to different sensors/heaters can be better (and more compactly) served using a function call with the sensor/heater name/number passed as a parameter. (IMHO case structures are best for instances where each case is radically different or the same function call with drastically different arguments.)

In an earlier post I mentioned using a STRUCT to manage the data for different tanks and I have been thinking a little more about the idea. The following snippets are intended to illustrate what I have in mind. Obviously I have no idea if the concept matches your physical requirements. But if it (or something like it) is suitable I think it would both shorten the code and make it easier to follow.

For the avoidance of doubt, this will NOT compile.

struct TANK {
  boolean heaterOn;
  boolean fillPumpOn;
  boolean drainPumpOn;
  boolean waterHigh;
  boolean waterLow;
  byte waterHighPin;
  byte waterLowPin;
  byte fillPumpPin;
  byte drainPumpPin;
  byte heaterPin;
  byte temperaturePin;
  byte tempHighLimit;
  byte tempLowLimit;
  byte targetTemp;
  byte currentTemp;

};

TANK tanks[3];
byte frWater = 0;
byte aquarium = 1;
byte sump = 2;

// you can refer to the heater pin for the frWater tank with tanks[frWater].heaterPin
//  you can also cycle through all the tanks with for (byte t = 0; t < 3; t++) {  }

// you need code in setup() to put values in the various variables for each tank

// Then you might have a waterLevel function like this

void waterLevelCheck() {
  for (byte t = 0; t < 3; t++) {
    if (tanks[t].waterHigh) {
       digitalWrite(tanks[t].drainPumpPin, HIGH);
    }
    else {
      digitalWrite(tanks[t].drainPumpPin, LOW);
    }
    if (tanks[t].waterLow) {
      digitalWrite(tanks[t].fillPumpPin, HIGH);
    }
    else {
      digitalWrite(tanks[t].fillPumpPin, LOW);
    }
  }
}

// and a sensor check function like this

void sensorCheck () {
  for (byte t = 0; t < 3; t++) {
    tanks[t].waterHigh = digitalRead(tanks[t].waterHighPin);
    ...
    tanks[t].currentTemp = analogRead(tanks[t].temperaturePin);
   
  } 
}

Because there is so much initial data required I would be tempted to define 3 arrays as Unions with the structs (i.e. overlapping memory locations) and enter the initial data in the arrays in the style

tank1Data = {0,0,1,…};

Then the initialization is done by the compiler rather than your code, and no extra data space is needed.

But that may make the concept too complicated for the moment.

…R

Excellent suggestions. Thanks a bunch.

I may be slow in responding as I look up and learn how all this new stuff works.

For instance, I was unaware of the F() macro and it's implications. I was thinking "I've got 32K of memory, why worry about conserving it". Oops, 1K. At the same time, by putting stuff in Flash memory, if there is a power failure, when it comes back up, it should all still be there.
Question: Is there any restriction on saving all the various situational states (timeHeaterCOn, prevLowerFwTub, stateSolenoidRoDi, etc.) in Flash memory? If so, a power flicker would not result in a complete reset. The process could just pick up where it left off.

I just received a DS1307 via the "slow boat from China". I haven't yet gone through the learning process for using it. Not necessary for this process, but fun. Actually, the whole thing is not necessary. I could just check the aquarium every day to make sure everything is working. But I'm 'way too lazy for that.

// Turn on first heater in the list

Oops. Fixed. That's what comes of careless copy and paste procedures.

Heater list should use C,D,E (initialized as "xCDEx").

Rats! I have looked at that 100 times and noticed nothing wrong! That's why I can't get those routines to work! I originally had Sensors A,B,C,D,E and Heaters A,B,C with Heaters A,B,C connected to Sensors C,D,E. This got so confusing that I took the time to change it so that C,D,E were connected to C,D,E. I missed the rotater. Thanks eversomuch. I would probably be gazing past it next winter.

massive case statements

I was playing around trying to see if I could use case statements to simplify things. It didn't in this case. But code once written tends to live on to haunt the unsuspecting traveler.

Using a func_ prefix on your functions seems unnecessary.

This was just to remind myself what these things were. When I see "func_" I instantly know what it is. Been doing that for 40 years. Old dog; new tricks. You know the deal.

define a struct to hold the set of data being displayed

Excellent suggestion. Now I have to investigate what a Struct is and how it works. Thanks for causing trouble.

You can compare blocks of memory using memcmp() from string.h.
The memXXX() functions are nice and clean.

You guys are determined to make me hurt my head by having to learn a whole bunch of new stuff, aren't you? Thanks. Looks like a much cleaner way to do it. Used with the F() macro, it will solve a bunch of my mess.

Millis and micros values should be held as unsigned long values.

Fixed. But it messed up the nice, neat columns of comments by making the code line longer. :~

"const uint8_t"

What's "uint8_t"?

Bytes instead of int

I never thought about it. Good idea. Thanks. Fixed.

change all your calls to now() with millis()

The time.h stuff looked more interesting than millis(). I saw a discussion about millis() rolling over to start over after some length of time and I didn't want to deal with it. I'm going to use a DS1307 clock sometime soon. Not needed. Just because it looks interesting.

If you want to avoid thisif....(stateHeaterD = LOW) 
then try to develop the habit of writing comparisons with the constant first
if (LOW == stateHeaterD) will keep the compiler happy, but if you forget how a comparison should look, then if (LOW = stateHeaterD) will upset the compiler, and you'll get an error.
OTOH, if you remember to do that, you'll remember how to write comparisons correctly.

"uint8_t" is a portable version of the Arduino "byte" data type.

AWOL:
"uint8_t" is a portable version of the Arduino "byte" data type.

So byte is Arduino specific. And uint8_t stands for Unsigned INTeger 8bits and (_t) is there just to make it even more difficult to type. :stuck_out_tongue:

Yes, the = vs == bites me often. COBOL compiler was smart enough to know what the situation was. :stuck_out_tongue_closed_eyes: Now THAT should set off a firestorm of comments. Probably attract folks from the SparkFun forums. XD

I had a friend that was an Assembler (IBM mainframe) programmer that told me he would rather write down the page than across the page.

Thanks for the info.
John

OldSalt1945:
For instance, I was unaware of the F() macro and it's implications. I was thinking "I've got 32K of memory, why worry about conserving it". Oops, 1K. At the same time, by putting stuff in Flash memory, if there is a power failure, when it comes back up, it should all still be there.
Question: Is there any restriction on saving all the various situational states (timeHeaterCOn, prevLowerFwTub, stateSolenoidRoDi, etc.) in Flash memory? If so, a power flicker would not result in a complete reset. The process could just pick up where it left off.

1K? UNO with ATMega328P chip has 2K RAM and Duemilanove with ATMega168P chip, IIRC, has 16K flash.

The flash memory only gets compiled code in Arduino, it's a kind of ROM at run-time.
However the UNO has 1K EEPROM you can read/write for power-off storage.

But... if you really want cheap mass storage and you can solder then you can make an SD module out of a micro-SD to full SD adapter (those sleeves that come with micro-SD cards), some resistors (there's a diode alternative) and wire and maybe even some kind of board for less than $5 in parts not counting the micro-SD card itself.
Just remember that SD has limited writes. Don't treat it the same as a hard drive. If you have a data file that you want to sort, write a sorted index file instead and access the data through the indexes. The card might last "forever" that way.

I just received a DS1307 via the "slow boat from China". I haven't yet gone through the learning process for using it. Not necessary for this process, but fun. Actually, the whole thing is not necessary. I could just check the aquarium every day to make sure everything is working. But I'm 'way too lazy for that.

If you include a set of indicator leds, or even one, then a quick glance at those can tell you how it's going.
RGB leds are pretty cheap now. I paid $1 for 5. The same light can be 8 easy to see as different colors (if off is a color) and blink rate can also provide whole new levels to those. That takes 3 pins though, for 1 pin you can blink and idiot light too.

Heater list should use C,D,E (initialized as "xCDEx").

Rats! I have looked at that 100 times and noticed nothing wrong! That's why I can't get those routines to work! I originally had Sensors A,B,C,D,E and Heaters A,B,C with Heaters A,B,C connected to Sensors C,D,E. This got so confusing that I took the time to change it so that C,D,E were connected to C,D,E. I missed the rotater. Thanks eversomuch. I would probably be gazing past it next winter.[/quote]

All of those beg using arrays so that the same code that uses one sensor/heater can work for all just by feeding it different indexes.

You can also give indexes names to reduce confusion and need fewer comments through enumerators.

Enums are like defines only better, the compiler can check their use.

define a struct to hold the set of data being displayed

Excellent suggestion. Now I have to investigate what a Struct is and how it works. Thanks for causing trouble.[/quote]

The compiler had some minor problem with structs in the past if they were not in their own .h file.

A struct is a named container for variables. You can group elements that go together as a meta-variable and even make arrays of those to index through. Then there's Classes which are bundled data and functions to handle the data. When you use Serial or most libraries you are using Classes through Class Objects. They make it harder for you to step on your own or included code/data (especially the included stuff you don't know so much about).

Be real happy about structs and classes, they already make your Arduino life easier.

You can compare blocks of memory using memcmp() from string.h.
The memXXX() functions are nice and clean.

You guys are determined to make me hurt my head by having to learn a whole bunch of new stuff, aren't you? Thanks. Looks like a much cleaner way to do it. Used with the F() macro, it will solve a bunch of my mess.

All the mem functions are address & bytes based. It's really very primitive and general which is also the power.
If I want to compared structs then I can write a whole bunch of lines of ifs or just 1 line with memcmp().
memmove() will let you shift a block of data (say bytes in an array) without fear of overwriting in the process.
memset() lets you set a block of memory to all the same value.

Millis and micros values should be held as unsigned long values.

Fixed. But it messed up the nice, neat columns of comments by making the code line longer. :~

"const uint8_t"

What's "uint8_t"?

unsigned 8 bit integer and the _t means type as in variable type.
The short name is byte.

Bytes instead of int

I never thought about it. Good idea. Thanks. Fixed.

change all your calls to now() with millis()

The time.h stuff looked more interesting than millis(). I saw a discussion about millis() rolling over to start over after some length of time and I didn't want to deal with it. I'm going to use a DS1307 clock sometime soon. Not needed. Just because it looks interesting.

millis() does roll over after 49.7-some days which is the longest interval you can get using unsigned long.
The rollover is NOT A PROBLEM as long as you use unsigned integers and the unsigned subtraction.
For a simple example of why, consider a clock face has 12 hours and we don't have problems with that.

It's possible to use 64 bit unsigned long long to time with. One VERY GOOD coder here, Morris Dovey, left a time library that uses those, the longest interval even using micros() instead of millis() is effectively forever.

OldSalt1945:
What's "uint8_t"?

Standard platform / compiler independent data-type...
http://www.nongnu.org/avr-libc/user-manual/group__avr__stdint.html

COBOL compiler was smart enough to know what the situation was

That's because COBOL has the MOVE verb (looking back on my COBOL career, I felt like I was SHOUTING all the time, but then 132 column line-printers and teletypes didn't do lower-case) for assignment, and reserved '=' for comparison.
Algol and Pascal have ':=' for assignment.
I can only assume K&R were seriously into brevity.