Guidance on style and form

Hi there,

About a month ago I started learning about Arduinos. I have to admit, once I got started, I became hooked.

Once my source became large enough, I discovered the multiple tabs in the IDE... oh happy days. Now I've discovered how to make my own libraries, which also should make my code much easier to read, maintain, and reuse.

The project I'm currently working on is a program for a Mega. It is basically a datalogger for a greenhouse for a friend. It's intended use is for wasabi, but could be used for anything I guess (a single rose bush that I can make flower out of season will be my first personal application).

The basic feature set is:

  • Monitor a dozen or so sensors
  • Automate various menial tasks (water levels, nutrients addition)
  • Datalog the readings and output states to an SD Card
  • Output the current state of the unit to an LCD screen
  • Report the data to date on the SD card out to the ethernet via HTTP (to a Windows service storing on a SQL server)

I have a heap of things that need to be done (primarily load a config file from the SD card, and some sort of PID type control on some of the outputs, e.g. temp control via exhaust fan and/or heater).

Whilst all of that was fairly easy (thank Freetronics for getting a local supplier!), my question today is one of style and technique. The amount of code I have is obviously ever expanding, but before I go too much further, I would like to have people who are familiar with the Arduino system double check that I'm not doing things that are:

  • Unnecessary
  • Wasteful on memory
  • Bad practice
  • Potentially dangerous to the unit
  • Have big potential for bugs

Could you please tell me the most convenient way for me allow you guys access to my code for review? (If this in inappropriate that's ok and I apologise in advance. The code has no licensing of any sort, you're welcome to steal or reuse any of it, just try not to laugh at it too hard is all I ask)

attach it as a zip file

Cheers :slight_smile:

controller_mega6.zip (8.89 KB)

logBuffer[logBufferCount][0] = 0;
    logBuffer[logBufferCount][1] = 0;
    logBuffer[logBufferCount][2] = 0;
    logBuffer[logBufferCount][3] = 0;
    logBuffer[logBufferCount][4] = 0;
    logBuffer[logBufferCount][5] = 0;
    logBuffer[logBufferCount][6] = 0;
    logBuffer[logBufferCount][7] = 0;
  }
  
  logBuffer[logBufferCount][0] = isnan(logBuffer[logBufferCount][0]) ? -255 : logBuffer[logBufferCount][0];
  logBuffer[logBufferCount][1] = isnan(logBuffer[logBufferCount][1]) ? -255 : logBuffer[logBufferCount][1];
  logBuffer[logBufferCount][2] = isnan(logBuffer[logBufferCount][2]) ? -255 : logBuffer[logBufferCount][2];
  logBuffer[logBufferCount][3] = isnan(logBuffer[logBufferCount][3]) ? -255 : logBuffer[logBufferCount][3];
  logBuffer[logBufferCount][4] = isnan(logBuffer[logBufferCount][4]) ? -255 : logBuffer[logBufferCount][4];
  logBuffer[logBufferCount][5] = isnan(logBuffer[logBufferCount][5]) ? -255 : logBuffer[logBufferCount][5];
  logBuffer[logBufferCount][6] = isnan(logBuffer[logBufferCount][6]) ? -255 : logBuffer[logBufferCount][6];
  logBuffer[logBufferCount][7] = isnan(logBuffer[logBufferCount][7]) ? -255 : logBuffer[logBufferCount][7];

Have you got an allergy to for loops?

Heh no, look a few lines down and you’ll see:

 for (int i = 0; i < RELAY_COUNT; i++)
    logBuffer[logBufferCount][i + 8] = relayPinState[i];

But it’s good question because this bit is something I don’t know what to do with:

    logBuffer[logBufferCount][0] = dht0.readHumidity();
    logBuffer[logBufferCount][1] = dht1.readHumidity();
    logBuffer[logBufferCount][2] = dht2.readHumidity();
    logBuffer[logBufferCount][3] = dht0.readTemperature(false);
    logBuffer[logBufferCount][4] = dht1.readTemperature(false);
    logBuffer[logBufferCount][5] = dht2.readTemperature(false);
    logBuffer[logBufferCount][6] = getCurrentTemp(PIN_TEMP_0);
    logBuffer[logBufferCount][7] = analogRead(PIN_LIGHT_0);

Ideally I’d like it configurable from a text file so that I can give it to the world, and they can easily change it to do whatever their setup needs. So ideally I want:

astract class FloatInputSensor
{
  abstract float GetValue();
}

DHTSensor : FloatInputSensor
{
  DHT dht;

  FloatInputSensor(int pin)
  {
    dht = DHT(int, DHT22);
  }
  override float GetValue()
  {
    return DHT.GetHumdity();
  }
}

for (int i = 0; i < SENSOR_COUNT; i++)
  logBuffer[logBufferCount][i] = sensor[i].GetValue();

Or something like that… but I think that’ll either be overkill or too much memory, or impossible, or something bad at least :-/ I think I’ll just have a sensor class, with a sensortype passed in, and a simple switch statement. Definitely open to ideas though.

unsigned long relayLowTime[RELAY_COUNT]    = { 12,   15,  23, 255, 255, 300, 255, 255 };
unsigned long relayHighTime[RELAY_COUNT]   = { 6,     3,  25, 255, 255, 600, 255, 255 };

Why use a 4-byte variable when you could use an unsigned int?


  serialWriteLog(F("SDLog: Open next file.\n"), LOG_LEVEL_INFORMATION);

      Serial.print("\nScheduled Relay: Scheduled event for relay ");

You use the F macro some times but not others. I suggest using it all the time, if you are going to do it. It isn’t critical, but it’s consistent.


void sdCardWriteDataLogBuffer(boolean closefile)
{ 
...
  if (closefile == true)

Booleans are true or false by their nature. Saying == true is a bit redundant. You may as well say:

if ((i > 6) == true)

Messy isn’t it? Better to say:

  if (closefile)

I agree with AWOL about the loops:

    logBuffer[logBufferCount][0] = 0;
    logBuffer[logBufferCount][1] = 0;
    logBuffer[logBufferCount][2] = 0;
    logBuffer[logBufferCount][3] = 0;
    logBuffer[logBufferCount][4] = 0;
    logBuffer[logBufferCount][5] = 0;
    logBuffer[logBufferCount][6] = 0;
    logBuffer[logBufferCount][7] = 0;

Why not:

    for (i = 0; i < 8; i++)
      logBuffer[logBufferCount][i] = 0;

Ditto further down.


Controller_Mega6.ino
environment.ino
ethernet.ino
lcd.ino
rtc.ino
scheduledRelay.ino
sdLogger.ino
serialWriteLog.ino

What’s with all the .ino files? Usually you have one .ino file and the rest .cpp files.

[quote author=Nick Gammon link=topic=112543.msg846028#msg846028 date=1341220091]Why use a 4-byte variable when you could use an unsigned int?[/quote]It's storing seconds since 1970. If I changed that to seconds since startup, I could use a more compact data type... but I want it to recover the schedule if the power is cycled for some reason so not sure what to do there.

[quote author=Nick Gammon link=topic=112543.msg846028#msg846028 date=1341220091]You use the F macro some times but not others. I suggest using it all the time, if you are going to do it. It isn't critical, but it's consistent.[/quote]Yup, just being lazy. Still using Serial.print in places at the moment too.

[quote author=Nick Gammon link=topic=112543.msg846028#msg846028 date=1341220091]Messy isn't it? Better to say:[/quote]Definitely. Not sure why I've done it this way, normally I never would. I assume that while(someint) is still someint != 0 ?

[quote author=Nick Gammon link=topic=112543.msg846028#msg846028 date=1341220091]I agree with AWOL about the loops:[/quote]Yeah, me too... I've gotten off my backside and corrected them.

[quote author=Nick Gammon link=topic=112543.msg846028#msg846028 date=1341220091]

Controller_Mega6.ino
environment.ino
ethernet.ino
lcd.ino
rtc.ino
scheduledRelay.ino
sdLogger.ino
serialWriteLog.ino

What's with all the .ino files? Usually you have one .ino file and the rest .cpp files.
[/quote]I'm not sure, when I add a tab to the parent .ino file, that's what I get. This is inside the Arduino IDE. Is there a more mature IDE? The current one is cute, but it makes me want to help work on it... just little things like no underscores (and thus hotkeys) for the menu items and things like the search and replace box buttons. Is that something I would be allowed to help modify?

I assume that while(someint) is still someint != 0 ?

Please don't do that. The comment about the unnecessary == true part applies ONLY to boolean variables.

For other variable types, EXPLICIT tests for a particular value are preferred.

Ok that's cool... Just wondering, I assume it's syntactically allowed though? I'm also assuming that this is C or some derivative of it?

I'm not sure, when I add a tab to the parent .ino file, that's what I get. This is inside the Arduino IDE.

When you add a tab, you get a dialog box that allows you to define a name for the new file. If you specify an extension, that extension will be used. If you don't, the .ino extension will be added.

So, specify the .cpp extension, instead of being lazy.

Just wondering, I assume it's syntactically allowed though?

Yes. If you don't define a complete condition, the incomplete condition is determined to be true or false on its own. Any non-zero value is true; any zero value is false.

Using something like

if(Serial.available())
{
  // Read the data
}

though tells me that you don't understand what the function returns, rather than telling me that you know that any non-zero value means that you can read data.

I much prefer to see that explicitly stated:

if(Serial.available() > 0)
{
  // Read the data
}

Why .cpp?

Why .cpp?

Because there is no .st2 compiler.

What else would you use?

Ok, that makes sense :slight_smile:

I would ask what an .st2 compiler is, but I guess I should go google that one :slight_smile:
What else would I use? The default obviously! Which is what I did and that’s what I got… should the default be changed to .cpp in the IDE? I hope this isn’t a well known religious war I’ve unwittingly come across?

I would ask what an .st2 compiler is

I just jabbed 3 random characters, so I doubt that google will tell you anything. If it did, I wouldn't trust the context to match.

should the default be changed to .cpp in the IDE?

No, it shouldn't. There are times when multiple .ino files are appropriate, and times when a single .ino file and some .cpp files are appropriate.

In your case, multiple .ino files might be appropriate. I haven't looked at what is in them, so I can't comment one way or the other.

PaulS:
I just jabbed 3 random characters, so I doubt that google will tell you anything. If it did, I wouldn't trust the context to match.

Ok.

PaulS:
No, it shouldn't. There are times when multiple .ino files are appropriate, and times when a single .ino file and some .cpp files are appropriate.

Oh, that opened a world of pain... I'll have to read more about when and why I think.

The comment about the unnecessary == true part applies ONLY to boolean variables.

Nonsense.
The result of the implicit test (zero/non-zero) is boolean.
It is fundamental to C.

malcolmnz:
I would ask what an .st2 compiler is ...

StarTrek2 language, used on the Enterprise I believe. I think you need 20 gigaquads of memory at least.

Nonsense.
The result of the implicit test (zero/non-zero) is boolean.
It is fundamental to C.

It is not fundamental to the way I think, and is sloppy. I expect any non-boolean variable to be explicitly compared to some value. Of course, that's just me.

  String nextnum = String(filenum);

I would wean yourself off the String class while there is still time. :slight_smile:

Your code isn't too bad BTW, can't really complain. But you did ask for comments. :slight_smile: