Domotics control system

Question

This is a project about a domotics control system based on arduino. Nothing great; it's only for testing myself and making stuff.

Before I started coding this, I searched on the net for some useful tips to use in my code, but I still remain a novice in programming.

The project is about 1600 lines long. I'm obligated to load it on an external site here.

DomoS.h

#ifndef DomoS_H
#define DomoS_H
#include <arduino.h>
class DomoS
{
private:
 static const byte STRINGMAXLEN = 64; //Maximum length for the command string
 static const byte SUBSTRINGMAXLEN = 16; //Maximum length for a single command
 static const byte MAXADDRESSPIN = 8; //Maximum number of adressing pin
 static const byte MAXNAMELEN = 10; //Maximum length for a peripheral name
 static const byte FILEVER = 0; //The version of the file type
 /*
 The DomoS setting file is made of two parts
 1) The header of the file which contains all of the configuration parameters
 of the DomoS module and the version of the file type
 2) The body of the file which contains all the settings for the different
 peripherals [for a explanation go to the declaration of the type]
 These are stored in sequential order
 With version 0 the different arduino EEPROM can contain up to:
 ATmega168 and ATmega8 [512byte]: 45 peripherals
 ATmega328 [1024byte]: 91 peripherals
 ATmega1280 and ATmega2560 [4096byte]: 371 peripherals
 */
 struct DomoSFileHeader //size 13byte
 {
 //The order here is also the order in the EEPROM
 //RESPECT THIS ORDER
 //The "EEPROM #" on the right of the variable name is the standard position in the EEPROM
 //Version of the file type, in case of change through developement
 byte fileVer; //EEPROM 2
 //Copy of the configuration parameters
 byte numPeripheral; //EEPROM 3
 byte numAddressPin; //EEPROM 4
 byte addressPin[MAXADDRESSPIN]; //EEPROM 5-12
 byte outputPin; //EEPROM 13
 byte writeToEeprom; //EEPROM 14
 };
 struct DomoSFileBody //size 11byte
 {
 char name[MAXNAMELEN]; //The name of the peripheral
 byte number; //The number of the peripheral and the addressing parameter
 };
 //Setup function
 void GetConfigurationDataFromEeprom(); //Gets the configurations data from the EEPROM
 void FirstStart(); //Starts all the magic before the first start
 boolean CheckSetupData(); //Checks if the system has been already setupped
 void ClearEeprom(); //Clears all the EEPROM
 void WriteSetupData(); //Writes to the first two cells of EEPROM the setup check values
 void AskData(DomoSFileHeader & data); //Asks to the user the configuration parameters
 void WriteConfigurationDataToEeprom (DomoSFileHeader data); //Writes the data variables into the EEPROM
 void Initialize(); //Initializes the DomoS module
 void UpdateNumPeripheral(char type); //Updates the peripheral number
 boolean ConvertDecimalToBinary(int number, boolean result[]); //Converts a decimal number to an array of boolean, return false if the number is greater than what the module can handle, else true
 void SetAddressing(boolean addressing[]); //Sets up the addressing lines
 byte SeparateCommandBySpace(); //Separates the _command string into two strings, the first is the first word before the space, the second is the original string with the first word deleted, returns the number of char written in _subCommand
 byte CompareSubCommand(); //Compares a command with the commands' dictionary
 boolean FetchCommand(); //Fetches a command from the serial port
 void DoCommand(byte numCommand); //Executes a command
 byte GetCommand(char* command); //Gets the number of a command
 void CommandToLowerCase(); //Converts the _command string to lower case
 void SubCommandShiftLeft(); //Shifts left of one position all the character in the _subCommand string
 byte SearchPeripheralByName(char peripheral[]); //Searches the peripheral by name
 void GetPeripheralName(byte numPeripheral, char name[]); //Writes in char name[] the name of numPeripheral-th peripheral
 boolean SearchDuplicatedPeripheral(DomoSFileBody & peripheral, boolean nameCustom, boolean numberCustom); //Checks if the peripheral is unique else tries to make it unique
 void GetPeripheralNumber(byte numPeripheral, byte & number); //Writes in "number" the number of numPeripheral-th peripheral
 byte ConvertBinaryStringToDecimal();
 void BlankNewPeripheral(DomoSFileBody & peripheral);
 boolean CreateParameterCheck(DomoSFileBody & peripheral);
 boolean WritePeripheral(DomoSFileBody peripheral, byte position);
 byte SearchPeripheralByNumber(byte number);
 void ComposeStringPeripheral(DomoSFileBody peripheral, byte phrase);
 byte GetError();
 boolean WritePeripheralToEeprom(DomoSFileBody peripheral, byte position); //Writes the peripheral to the EEPROM
 //boolean WritePeripheralToSd(DomoSFileBody peripheral, byte position); //Function not yet developed
 void Create(); //Create a peripheral
 void Turn(); //Activate a peripheral
 void Delete(); //Delete a peripheral, probably this wont be developed
 void Exit(); //Turn off DomoS module
 void Reset(); //Resets the DomoS module
 void List(); //Give a list of all the installed peripheral
 int parseInt();
 void ThrownError();
 //Declaration of configuration variables
 byte _numAddressPin; //Number of pins used for addressing peripherals
 byte _addressPin[MAXADDRESSPIN]; //Pins used for addressing
 byte _outputPin; //The pin used for the output, pin 6 will automatically be selected
 byte _writeToEeprom; //-1 if DomoS must store the peripheral settings in the EEPROM, else the CSPin where the SD card is connected for storing the settings in DomoS.dat file
 byte _numPeripheral; //Number of peripheral created by user
 byte _fileVer; //The version of the file type
 boolean _on; //Tell if DOMOS system is on
 byte _lastError; //Tell the last error thrown by DomoS
 /*
 Declaration of strings constant
 Inizialization in DomoS.cpp
 */
 static const byte NCOMMAND = 8; //number of commands allowed
 static const char* COMMAND[NCOMMAND]; //Array of commands, for explanation go to inizialization
 static const int NPHRASE = 12; //Number of phrases, for eventually translation
 static const char* PHRASE[NPHRASE];
 static const int NERROR = 24;
 static const char* ERROR[NERROR];
 static const int START = 2;
 static const byte SETUP[START]; //These two values are stored in the first two cell of EEPROM, if already present the system have been already setup, if not launch the first start procedure
 static const int RCLOAD = 300; //Number of millisecond necessary for charging of RC circuit
 //String for fetching and checking commands
 char _command[STRINGMAXLEN];
 char _subCommand[SUBSTRINGMAXLEN];
public:
 //Constructor
 DomoS();
 void Work();
 boolean IsOn(); //Control if the module is ready to handle new request
 //Errors constant
 static const byte OK = 0;
 static const byte COMMANDSTRINGTOOLONG = 1;
 static const byte SUBCOMMANDSTRINGTOOLONG = 2;
 static const byte COMMANDNOTRECOGNIZED = 3;
 static const byte NOCOMMANDPARAMETERS = 4;
 static const byte SUBCOMMANDNOTRECOGNIZED = 5;
 static const byte NAMENOTDEFINED = 6;
 static const byte NAMETOOLONG = 7;
 static const byte ASNOTDEFINED = 8;
 static const byte BINARYNUMBERTOOLONG = 9;
 static const byte EEPROMISFULL = 10;
 static const byte PERIPHERALNOTFOUND = 11;
 static const byte STRANGETURNPARAMETER = 12;
 static const byte BADTHINGSHAPPEN = 13;
 static const byte PERIPHERALZERONOTALLOWED = 14;
 static const byte PERIPHERALMAXIMUMNUMBERREACH = 15;
 static const byte PERIPHERALNAMENOTUNIQUE = 16;
 static const byte PERIPHERALNUMBERNOTUNIQUE = 17;
 static const byte CANTFINDFREENAME = 18;
 static const byte CANTFINDFREENUMBER = 19;
 static const byte VOLTAGETOOLOW = 20;
 static const byte VOLTAGETOOHIGH = 21;
 static const byte DECIMALNUMBERTOOBIG = 22;
 static const byte THEREAREZEROPERIPHERAL = 23;
};
#endif

DomoS.cpp

This is too long and doesn't fit in the post.

• Lstor: You are very unlikely to get a good review unless you pick out the relevant parts you want reviewed and add them to the post.

Answer 1

I am not familiar with Arduino, but as a general C++ developer, I've noticed the following issues (looking only at the code in the question, not the one behind links):

• No const-correctness.

  The biggest problem in the code, one which would cause me to return it back to the developer if I were reviewing this at work.

  None of your member functions are classified as const, meaning they will not be callable on a const object of type DomoS, or such an object accessed through a const-qualified path (such as a const DomoS &).

  Even more importantly, your functions which take a pointer or reference parameter do not use const on the destination type either, which again means const objects or, even worse, temporaries cannot be used as arguments for those parameters. This way, you also deprive yourself of a great opportunity for self-documenting code, creating space for confusion of the user (or future maintainer) of the class. Example:

  byte GetCommand(char* command); //Gets the number of a command
  

  Based on its signature, this function:

  1. returns a byte. Given the comment, I expect this to be a number of the command
  2. can write something into the string pointed to by command. As a user, I am confused. Will it do so? What if the buffer I pass in is not large enough for what the function will write? How do I determine what "large enough" is? Why can't I use a string literal as the argument (string literals are of type const char[], and thus passing one here would cast away constness)?
  3. can modify the DomoS object on which it's invoked. This is definitely not something I would expect from a function whose name starts with Get.

  Instead, I would expect the function's signature to be:

  byte GetCommand(const char* command) const; //Returns the number of a command
  

  The same applies to many other functions in the class, of course. This was just an example.

  Writing const-correct code also prevents some programmer errors, such as accidentally modifying an input-only parameter.

  Last but not least, it is a real pain to integrate a non-const-correct module into otherwise const-correct code. You have to use const_casts everywhere, which is 1) tedious to write, 2) a great source of bugs and 3) a maintenance nightmare. I understand this is less of an issue here since almost all of the class's functions are private, but it's a general principle worth mentioning anyway. From the external interface at least, IsOn() should definitely be const.

  A quick summary of "const-correctness":

  • A member function which does not modify the observable state of the object it's invoked on should be marked as const, so that it can be called on const-qualified objects.
  • A function taking a parameter by reference and not modifying the parameter should take the parameter by const &. This is particularly important because it allows temporary objects to be bound to the reference (temporaries can bind to const-references, but not to non-const references).
  • A function taking a parameter by pointer and not modifying the object pointed to should declare the parameter as pointer to const.

  Imagine this code:

  void prettyPrint(const std::string &s) {
   std::cout << s.size() << ' ' << s << '\n';
  }
  std::string x = "Hello";
  std::string y = " World!";
  prettyPrint(x + y);
  

  If there was no const in the signature of print(), this wouldn't compile, as x + y returns a temporary. If the purely observing member function size() of std::string wasn't marked as const, we couldn't call s.size() inside prettyPrint() because s is a const-reference.

• Taking structures by value.

  Some of your functions take parameters of type DomoSFileBody by value. This can be OK if the function does any internal modification on the arguments passed in: copying at call site can be more efficient than copying internally, as discussed in this great article.

  However, if the function does not modify its argument, you can save a significant amount of copying by passing these arguments as const DomoSFileBody & instead.

• Hand-crafted enumeration for the error code.

  The error constants would be perfectly represented as an enumeration. This has a distinct advantage of creating a full-fledged type, so that overload resolution can be used and code is more self-documenting. Function signature byte GetError(); doesn't really tell me much, but ErrorCode GetError(); clearly says what values I can expect.

  In C++11 (and many compilers support this even before that), you can explicitly specify the underlying type for an enumeration, so it could be a byte even so. The definition would then look like this:

  //Errors constant
  enum ErrorCode {
   OK,
   COMMANDSTRINGTOOLONG,
   SUBCOMMANDSTRINGTOOLONG,
   COMMANDNOTRECOGNIZED,
   NOCOMMANDPARAMETERS,
   SUBCOMMANDNOTRECOGNIZED,
   NAMENOTDEFINED,
   NAMETOOLONG,
   ASNOTDEFINED,
   BINARYNUMBERTOOLONG,
   EEPROMISFULL,
   PERIPHERALNOTFOUND,
   STRANGETURNPARAMETER,
   BADTHINGSHAPPEN,
   PERIPHERALZERONOTALLOWED,
   PERIPHERALMAXIMUMNUMBERREACH,
   PERIPHERALNAMENOTUNIQUE,
   PERIPHERALNUMBERNOTUNIQUE,
   CANTFINDFREENAME,
   CANTFINDFREENUMBER,
   VOLTAGETOOLOW,
   VOLTAGETOOHIGH,
   DECIMALNUMBERTOOBIG,
   THEREAREZEROPERIPHERAL,
   NERROR
  };
  

  Note the inclusion of NERROR as the last enumerator. This has the advantage of keeping this value always 1 + last enumerator, so that you cannot forget to increase the value if you later need to add further error codes.

  I would also introduce underscores to separate words in the enumerators (e.g. COMMAND_STRING_TOO_LONG), but that is mostly a matter of personal preference.

• Using signed types for sizes.

  NPHRASE and START (and also NERROR, but see above for that) are defined with type const int, even though they represent non-negative quantities (a number of something). The ideal type for this is std::size_t, which is again self-documenting as "this variable holds a size or count."

  If you cannot use this for reasons of type size (i.e. it's too big), at least use const unsigned int for the constants. That way, you clearly document they store non-negative quantities. It also prevents potential signed/unsigned issues when interacting with the standard library, which correctly uses unsigned types (std::size_t, actually) to represent sizes and counts.

• Inconsistent usage of char* p and char p[] in function arguments.

  These are semantically equivalent, but you should preferably pick one and stick to it.

• user92608: 0)Someone modified the tag of the question from c to c++, in fact the code use only c construct beside of the main class 1)Is the first time i hear the word "const correctnes", i took a look at wikipedia but i didn't understand well what you mean and how should i use this 2)Thanks for the tip, i'll use this in my next works 3)the arduino compiler don't support enumerator, i tried to implement it but without success
• Angew is no longer proud of SO: @user92608 So? The class is C++ (class, references etc.). And pretty much all the points are valid for C code as well.
• user92608: 4)Yes, you're right, that should only be a typo, these should also be byte const [already unsigned] 5)Luckily only one function use the char* p style, maybe another typo
• Angew is no longer proud of SO: @user92608 I've elaborated a bit more on const-correctness. If enumerations are not supported by the compiler, then your approach is the best that can be done. You might at least introduce a typedef for the error code type. While it will not be type-safe, it will at least be self-documenting.
• user92608: Then, if i understood well i should write boolean CheckSetupData(); -> const boolean CheckSetupData(); because this function don't modify the internal state of the class. But this function byte SeparateCommandBySpace(); can't be const because it modify some internal variables. Instead in this function void WriteConfigurationDataToEeprom (DomoSFileHeader data); i should write (const DomoSFileHeader & data) for avoid the copying of the structure Have I understood well??