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.

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.