Refactoring from *.ino to multiple cpp/h files

Long story short, I have been working on a project for a few years off and on based on the ESP8266 then ESP32. In arduino ide I had 10 ino files and was getting frustrated with the code base. I couldnt find errors efficiently, and it's a mess.

So, taking all the knowledge I gained since starting, i wanted to drop the IDE, and use something im more familiar with. And get closer to a traditional c/c++ coding style. so, right now Im using Visual Studio Code with PlatformIO. the SoC on my board is a ESP-WROOM-32U.

I'm going to ask quite a few questions in the thread, but ill start with these:
(Im coming from a self taught C# background. Im a SharePoint Developer by trade, to give context)

  • I had over 100 global variables in my main ino, is that generally a bad practice?
  • If not, what is the best way to allow access to them from all my cpp files?
  • Is it a better practice to try and create Classes within the .h files, or just have functions?

This topic is sort of a pet peeve with me as I see it done wrong so many times in Arduino Land. For my take, see Reply #3 Here.

Some globals are inevitable, but the fewer the better. Each one is a potential problem when debugging - this variable is changing somewhere - which of my eleven source files is doing it? What complex interaction between routines is responsible?

Using a decent IDE will help of course, but check whether those variables actually all need to be global. Can they be static instead? Either file level or function.

Maybe some of them form a logical group and would be better off in a class, perhaps with getters and setters. Encapsulation reduces the scope of your search when you're trying to debug.

theskaz:
I had over 100 global variables in my main ino, is that generally a bad practice?

Globals should be kept to a minimum, to the extent possible. But there is no rule ax to how many is too many. 100 would seem to me like a lot. I don't know that I've ever had than many in a well-constructed application, even ones consisting of tens of thousands of lines of code.

theskaz:
If not, what is the best way to allow access to them from all my cpp files?

To the extent practical, variables should be contained within the objects that control them. If other objects need to access the variables, provide getters to read them, and setters, where necessary, to validate and set new values. As much as possible, only the owning object should be able to change variable values, to avoid having to search the whole application to figure out how a bad value got set.

theskaz:
Is it a better practice to try and create Classes within the .h files, or just have functions?

Is it better to get where you're going by bus or bicycle? That is apples and oranges. The whole idea of object-oriented programming is to package data, and the code that uses that data, in one place, so it can be tightly controlled. The object should abstract the data, so the "consumers" of that data need not know the details of how that data is created and manipulated. You should be able to use an object without knowing the details of HOW that objects does what it does. If the object is a motor driver, you want to be able to tell it "move to position x", without knowing, or caring, HOW it gets there, whether it is a DC motor, AC motor, or stepper motor.

There are COUNTLESS online tutorials on the fundamentals of OOP. You should spend time reading, and studying, some of them because there is FAR more to learn than you will learn in a few forum posts. Start with the basics, ignoring advanced topics like poly-morhphism, multiple inheritance, etc. Master the basics of abstraction and encapsulation, then add the other skills one at a time over a longer period of time.

This free e-book:

http://www.charlespetzold.com/dotnet/

by Charles Petzold, is an excellent place to start, as are his other c# books. Though focused on c#, the basic concepts are exactly the same, and the language is, in most respects, similar enough to c++ to still be highly useful.

Regards,
Ray L.

Thank you guys, i read that post and had to look up static variables. basically they retain their value across function calls, which is very helpful.

I would like to try an exercise where I have 3 onewire devices, 2 ADS1115, and 1 BME280 that i want to utilize in a single cpp/h file set.

Let's call it sensors.cpp/h. In sensors.cpp, i would have the following includes:

#include "sensors.h"
#include <PID_v1.h>
#include <OneWire.h>
#include <DallasTemperature.h>
#include <Adafruit_Sensor.h>
#include <Adafruit_BME280.h>
#include <Adafruit_ADS1015.h>

now, each of those files include a class that i just implement. so creating a TempSensor class seems redundant, even though i would have custom functions that do what i need. I assume there is some sort of inheritance in c++ (like c#) where i could do something like "class TemperatureSensor : DallasTemperature" and implement additional functions. I don't think I would need to override any existing functions. my overall worry is bloating the program overly using abstraction.

my current code base is about 2000 lines of code.

Does moving to an OOP approach instead of a procedural approach cause a lot of overhead on these little guys?

Wondering if the OP is using the ESP32 OS called freeRTOS?

Idahowalker:
Wondering if the OP is using the ESP32 OS called freeRTOS?

I am primarily using arduino-esp32. I do have some elements that use the esp-idf to create tasks.

I ask because by using freeRTOS, there are mechanisms to handle your stated concerns.

Here is a sample of my use of the using the ESP OS in the Arduino IDE:

/*music responsible led strip sample code
  created by Yu Sun on 07/28/2015
*/
#include "esp_system.h" //This inclusion configures the peripherals in the ESP system.
#include "freertos/FreeRTOS.h"
#include "freertos/task.h"
#include "freertos/timers.h"
#include "freertos/event_groups.h"
#include <Adafruit_NeoPixel.h>
#include "AudioAnalyzer.h"
////
/* define event group and event bits */
EventGroupHandle_t eg;
#define evtDo_AudioReadFreq       ( 1 << 0 ) // 1
#define evtDo_LEDs                 ( 1 << 1 ) // 10
////
TickType_t xTicksToWait0 = 0;
////
QueueHandle_t xQ_LED_Info;
////
const int NeoPixelPin = 26;
const int LED_COUNT = 24; //total number of leds in the strip
const int NOISE = 150; // noise that you want to chrop off
const int SEG = 6; // how many parts you want to seperate the led strip into
const int SerialDataBits = 115200;
const int Priority4 = 4;
const int TaskStack40K = 40000;
const int TaskCore1  = 1;
const int TaskCore0 = 0;
const int AudioSampleSize = 6;
const int Brightness = 150;
const int A_D_ConversionBits = 4096; // arduino use 1024, ESP32 use 4096
////
Analyzer Audio = Analyzer( 5, 15, 36 );//Strobe pin ->15  RST pin ->2 Analog Pin ->36
// When we setup the NeoPixel library, we tell it how many pixels, and which pin to use to send signals.
Adafruit_NeoPixel leds = Adafruit_NeoPixel( LED_COUNT, NeoPixelPin, NEO_GRB + NEO_KHZ800 );
////
int FreqVal[7];//create an array to store the value of different freq
////
void setup()
{
  eg = xEventGroupCreate();
  Serial.begin( SerialDataBits );
  Audio.Init(); // start the audio analyzer
  leds.begin(); // Call this to start up the LED strip.
  clearLEDs();  // This function, defined below, de-energizes all LEDs...
  leds.show();  // ...but the LEDs don't actually update until you call this.
  ////
  xQ_LED_Info = xQueueCreate ( 1, sizeof(FreqVal) );
  //////////////////////////////////////////////////////////////////////////////////////////////
  xTaskCreatePinnedToCore( fDo_AudioReadFreq, "fDo_ AudioReadFreq", TaskStack40K, NULL, Priority4, NULL, TaskCore1 ); //assigned to core
  xTaskCreatePinnedToCore( fDo_LEDs, "fDo_ LEDs", TaskStack40K, NULL, Priority4, NULL, TaskCore0 ); //assigned to core
  xEventGroupSetBits( eg, evtDo_AudioReadFreq );
} // setup()
////
void loop() {} // void loop
////
void fDo_LEDs( void *pvParameters )
{
  int iFreqVal[7];
  int j;
  // TickType_t xLastWakeTime = xTaskGetTickCount();
  // int xLastWakeTime = micros();
  leds.setBrightness( Brightness ); //  1 = min brightness (off), 255 = max brightness.
  for (;;)
  {
    xEventGroupWaitBits (eg, evtDo_LEDs, pdTRUE, pdTRUE, portMAX_DELAY);
    xQueueReceive( xQ_LED_Info, &iFreqVal , xTicksToWait0 );
    j = 0;
    //assign different values for different parts of the led strip
    for (j = 0; j < LED_COUNT; j++)
    {
      if ( (0 <= j) && (j < (LED_COUNT / SEG)) )
      {
        set(j, iFreqVal[0]); // set the color of led
      }
      else if ( ((LED_COUNT / SEG) <= j) && (j < (LED_COUNT / SEG * 2)) )
      {
        set(j, iFreqVal[1]); //orginal code
      }
      else if ( ((LED_COUNT / SEG * 2) <= j) && (j < (LED_COUNT / SEG * 3)) )
      {
        set(j, iFreqVal[2]);
      }
      else if ( ((LED_COUNT / SEG * 3) <= j) && (j < (LED_COUNT / SEG * 4)) )
      {
        set(j, iFreqVal[3]);
      }
      else if ( ((LED_COUNT / SEG * 4) <= j) && (j < (LED_COUNT / SEG * 5)) )
      {
        set(j, iFreqVal[4]);
      }
      else
      {
        set(j, iFreqVal[5]);
      }
      // Serial.println( micros() - xLastWakeTime ); // round robin takes 1024 micros, .001024 mS
      // xLastWakeTime = micros();
    }
    leds.show();
    xEventGroupSetBits( eg, evtDo_AudioReadFreq );
  }
  vTaskDelete( NULL );
} // void fDo_ LEDs( void *pvParameters )
////
void fDo_AudioReadFreq( void *pvParameters )
{
  //  int64_t EndTime = esp_timer_get_time();
  //  int64_t StartTime = esp_timer_get_time(); //gets time in uSeconds like Arduino Micros
  for (;;)
  {
    xEventGroupWaitBits (eg, evtDo_AudioReadFreq, pdTRUE, pdTRUE, portMAX_DELAY);
    // EndTime = esp_timer_get_time() - StartTime;
    // Serial.println( (int)EndTime );
    Audio.ReadFreq(FreqVal);
    for (int i = 0; i < 7; i++)
    {
      FreqVal[i] = constrain( FreqVal[i], NOISE, A_D_ConversionBits );
      FreqVal[i] = map( FreqVal[i], NOISE, A_D_ConversionBits, 0, 255 );
      //      Serial.print(FreqVal[i]);//used for debugging and Freq choosing
      //      Serial.print(", ");
      //      Serial.println();
    }
    xQueueSend( xQ_LED_Info, ( void * ) &FreqVal, xTicksToWait0 );
    // StartTime = esp_timer_get_time();
    xEventGroupSetBits( eg, evtDo_LEDs );
  }
  vTaskDelete( NULL );
} // fDo_ AudioReadFreq( void *pvParameters )
////
//the following function set the led color based on its position and freq value
void set(byte position, int value)
{
  // segment 0, red
  if ( (0 <= position) && (position < LED_COUNT / SEG) ) // segment 0 (bottom to top), red
  {
    if ( value == 0 )
    {
      leds.setPixelColor( position, 0, 0, 0 );
    }
    else
    {
      leds.setPixelColor( position, leds.Color( value , 0, 0) );
    }
  }
  else if ( (LED_COUNT / SEG <= position) && (position < LED_COUNT / SEG * 2) ) // segment 1 yellow
  {
    if ( value == 0 )
    {
      leds.setPixelColor(position, leds.Color(0, 0, 0));
    }
    else
    {
      leds.setPixelColor(position, leds.Color( value, value, 0)); // works better to make yellow
    }
  }
  else if ( (LED_COUNT / SEG * 2 <= position) && (position < LED_COUNT / SEG * 3) ) // segment 2 pink
  {
    if ( value == 0 )
    {
      leds.setPixelColor(position, leds.Color(0, 0, 0));
    }
    else
    {
      leds.setPixelColor(position, leds.Color( value, 0, value * .91) ); // pink
    }
  }
  else if ( (LED_COUNT / SEG * 3 <= position) && (position < LED_COUNT / SEG * 4) ) // seg 3, green
  {
    if ( value == 0 )
    {
      leds.setPixelColor(position, leds.Color( 0, 0, 0));
    }
    else //
    {
      leds.setPixelColor( position, leds.Color( 0, value, 0) ); //
    }
  }
  else if ( (LED_COUNT / SEG * 4 <= position) && (position < LED_COUNT / SEG * 5) ) // segment 4, leds.color( R, G, B ), blue
  {
    // Serial.println( position );
    if ( value == 0 )
    {
      leds.setPixelColor(position, leds.Color( 0, 0, 0));
    }
    else //
    {
      leds.setPixelColor(position, leds.Color( 0, 0, value) ); // blue
    }
  }
  else // segment 5
  {
    if ( value == 0 )
    {
      leds.setPixelColor(position, leds.Color( 0, 0, 0)); // only helps a little bit in turning the leds off
    }
    else
    {
      leds.setPixelColor( position, leds.Color( value, value * .3, 0) ); // orange
    }
  }
} // void set(byte position, int value)
////
void clearLEDs()
{
  for (int i = 0; i < LED_COUNT; i++)
  {
    leds.setPixelColor(i, 0);
  }
} // void clearLEDs()

Also, with the Arduino IDE I use the ESP32 SPI API to take advantages of the ESP32 SPI features; such as being able to do SPI Communications in the background.

An example of using the ESP32 LED API for servo torques in the Arduino IDE

void setup()
{
  ////
  // create event group, make before timer trigger
  eg = xEventGroupCreate();
  Serial.begin( SerialDataBits );
  //  SerialBrain.begin( SerialDataBits );
  SerialTFMP.begin(  SerialDataBits );
  //// pinMode( BlinkLED, OUTPUT );//blink LED at pin 2
  ////
  ledcSetup ( Channel_X, TIMER_FREQUENCY, TIMER_WIDTH ); //
  ledcAttachPin ( XaxisServoPin, Channel_X );   //
  ledcWrite (Channel_X, usToTicks ( iX_Posit90 ) );       //
  vTaskDelay( ServoDelay60mS );
  //  ////
  ledcSetup( Channel_LIDAR, TIMER_FREQUENCY, TIMER_WIDTH); //
  ledcAttachPin( Servo_LIDAR_Pin, Channel_LIDAR);   //
  ledcWrite( Channel_LIDAR, usToTicks ( iLIDAR_Posit90 ) );
  vTaskDelay( ServoDelay60mS );
  //  ////
  ledcSetup ( Channel_Y, TIMER_FREQUENCY, TIMER_WIDTH ); //
  ledcAttachPin ( YaxisServoPin, Channel_Y );   //
  ledcWrite (Channel_Y, usToTicks ( iY_Posit90 ) );  //
  vTaskDelay( ServoDelay60mS );
  //  // make semaphores used in timer function before timer function begins
  sema_X = xSemaphoreCreateMutex();
  sema_Y = xSemaphoreCreateMutex();
  sema_LIDAR_FOR_ALARM = xSemaphoreCreateMutex();
  sema_SendSerialToBrain = xSemaphoreCreateMutex();
  ///////////// create queues
  // queue length, queue item size
  xQ_LIDAR_INFO = xQueueCreate( 1, sizeof(struct stu_LIDAR_INFO) ); // sends a queue copy
  xQ_X_INFO  = xQueueCreate( 1, sizeof( float ) ); // sends a queue copy
  xQ_Y_INFO  = xQueueCreate( 1, sizeof( float ) ); // sends a queue copy
  xQ_LIDAR_FOR_ALARM = xQueueCreate( 1, sizeof(struct stu_LIDAR_INFO) ); // sends a queue copy
  ////////////////////////  freeRTOS TASKS
  //////////////////////////////// TASK CORE 1 ///////////////////////////////////////////////////////////////////////////////////////////////////
  xTaskCreatePinnedToCore( fLIDAR_ServoAspectChange, "fLIDAR_ServoAspectChange", TaskStack20K, NULL, Priority4, &xHANDLE_LIDAR_ServoAspectChange, TaskCore1 ); // assigned to core
  sema_LIDAR_INFO = xSemaphoreCreateMutex();
  xSemaphoreGive( sema_LIDAR_INFO );
  xTaskCreatePinnedToCore( fDoLIDAR, "fDoLIDAR", TaskStack20K, NULL, Priority4, &xHANDLE_DoLIDAR, TaskCore1 ); //assigned to core
  sema_LIDAR = xSemaphoreCreateMutex();
  xSemaphoreGive ( sema_LIDAR );
  xSemaphoreGive ( sema_LIDAR_FOR_ALARM );
  xTaskCreatePinnedToCore( fSendLIDAR_InfoSerialToBrain, "fSendSerialToBrain", TaskStack20K, NULL, Priority3, &xHANDLE_SendLIDAR_InfoSerialToBrain, TaskCore1 ); // assigned to core
  // add in task handles and check if null
  xTaskCreatePinnedToCore( fLIDAR_Is_OK, "fLIDAR_Is_OK", TaskStack20K, NULL, Priority3, NULL, TaskCore1 ); // assigned to core
  //////////////////////////////// TASK CORE 0 ///////////////////////////////////////////////////////////////////////////////////////////////////
  xTaskCreatePinnedToCore( fSendSerialToBrain, "fSendSerialToBrain", TaskStack20K, NULL, Priority3, &xHANDLE_SendSerialToBrain, TaskCore0 ); // assigned to core
  xTaskCreatePinnedToCore( fTweakServoX, "fTweakServoXY", TaskStack20K, NULL, Priority3, &xHANDLE_TweakServoX, TaskCore1 ); // assigned to core
  xTaskCreatePinnedToCore( fTweakServoY, "fTweakServoXY", TaskStack20K, NULL, Priority3, &xHANDLE_TweakServoY, TaskCore1 ); // assigned to core
  xTaskCreatePinnedToCore ( fGetIMU, "v_getIMU", TaskStack40K, NULL, Priority4, &xHANDLE_GetIMU, TaskCore0 );
  vTaskDelay ( 300 );
  xEventGroupSetBits( eg, evtGetIMU );

} // setup()
////******************************************
void loop() {} // loop

Also note that vTaskDelay is non-blocking. If, during the delay time another task can be run then that task will run.

Been working with this all day, and learning on my feet. and I have come up with this:

main.cpp

#include "sensors.h"

#define ON_LED 12
#define USB_TX 1
#define USB_RX 3
#define TEMPA_BUS 4          
#define TEMPC_BUS 5         
#define ELEMENT2_SSR_RELAY 13 
#define ELEMENT1_SSR_RELAY 14
#define PUMP1_RELAY 15       
#define DEBUG_TX 16
#define DEBUG_RX 17
#define ELEMENT1_CONTACTOR_RELAY 18 
#define ERROR_RELAY 19    
#define SDA 21
#define SCL 22
#define TEMPB_BUS 23            
#define ELEMENT2_CONTACTOR_RELAY 25
#define DISPLAY_TX 26
#define DISPLAY_RX 27
#define PUMP2_RELAY 33

uint64_t chipId;

void setup()
{
  Serial.begin(BAUD_RATE);                                      //Programming
  Serial1.begin(BAUD_RATE, SERIAL_8N1, DISPLAY_RX, DISPLAY_TX); //Display Port
  Serial2.begin(BAUD_RATE, SERIAL_8N1, DEBUG_RX, DEBUG_TX);     //Debug Port

  chipId = ESP.getEfuseMac(); //The chip ID is essentially its MAC address(length: 6 bytes).

  Sensors sensors();

}

void loop()
{
  // never used
}

sensors.h

#ifndef __SENSORS_H__
#define __SENSORS_H__

#include <OneWire.h>
#include <DallasTemperature.h>
#include <Adafruit_Sensor.h>
#include <Adafruit_BME280.h>
#include <Adafruit_ADS1015.h>

class Sensors
{
private:
    Adafruit_BME280 bme;
    OneWire oneWireA;
    OneWire oneWireB;
    OneWire oneWireC;
    DallasTemperature sensorA;
    DallasTemperature sensorB;
    DallasTemperature sensorC;
    Adafruit_ADS1015 adsLeg1;
    Adafruit_ADS1015 adsLeg2;
    TaskHandle_t OneWireTaskHandle;
    void SensorInit();
    void SensorTask(void *parameter);

public:
    float currentPressure = 0;
    float currentHumidity = 0;
    float internalTemp = 0;
    float currentAltitude = 0;
    float ammeterLeg1 = 0;
    float ammeterLeg2 = 0;
    int actualSensorBTemp = 0;
    int actualSensorCTemp = 0;
    int actualSensorATemp = 0;

    Sensors();
};

#endif /* __SENSORS_H__ */

sensors.cpp

#include "sensors.h"
#include <PID_v1.h>

#define SDA 21
#define SCL 22
#define TEMPA_BUS 4  //Onewire1
#define TEMPB_BUS 23 //Onewire2
#define TEMPC_BUS 5  //onewire3
#define TEMP_PRECISION 9
#define SEALEVELPRESSURE_HPA (1013.25)

Adafruit_BME280 bme;
OneWire oneWireA(TEMPA_BUS);
OneWire oneWireB(TEMPB_BUS);
OneWire oneWireC(TEMPC_BUS);
DallasTemperature sensorA(&oneWireA);
DallasTemperature sensorB(&oneWireB);
DallasTemperature sensorC(&oneWireC);
Adafruit_ADS1015 adsLeg1(0x48);
Adafruit_ADS1015 adsLeg2(0x49);

char ampDisplayLeg1[5];
char ampDisplayLeg2[5];
float leg1;
float leg2;
int maxValueLeg1 = 0;
int minValueLeg1 = 0;
int maxValueLeg2 = 0;
int minValueLeg2 = 0;
float multiplier = 0.036;

Sensors::Sensors(){
    SensorInit();
    
    xTaskCreate(
      SensorTask,         /* pvTaskCode */ **ERROR: argument of type "void (Sensors::*)(void *parameter)" is incompatible with parameter of type "TaskFunction_t"**
      "OneWireWorkload",   /* pcName */
      2000,                /* usStackDepth */
      NULL,                /* pvParameters */
      7,                   /* uxPriority */
      &OneWireTaskHandle); /* pxCreatedTask */

}
void Sensors::SensorTask(void *parameter)
{
    TickType_t oneWireDelay = pdMS_TO_TICKS(750);
    TickType_t adsTaskDelay = pdMS_TO_TICKS(50);

    for (;;)
    {
        Serial2.println("Requesting Temperatures");
        sensorC.requestTemperatures();
        sensorB.requestTemperatures();
        sensorA.requestTemperatures();

        vTaskDelay(oneWireDelay);

        UBaseType_t uxHighWaterMark = uxTaskGetStackHighWaterMark(NULL);
        Serial2.print(xPortGetCoreID());
        Serial2.print("OneWire Task Memory: ");
        Serial2.print(uxHighWaterMark);
        Serial2.print(" ");
        Serial2.println(esp_get_free_heap_size());
        Serial2.println("Updating Temperatures");
        if (sensorC.getTempFByIndex(0) > 0 && sensorC.getTempFByIndex(0) < 212)
            actualSensorCTemp = sensorC.getTempFByIndex(0);
        if (sensorA.getTempFByIndex(0) > 0 && sensorA.getTempFByIndex(0) < 212)
            actualSensorATemp = sensorA.getTempFByIndex(0);
        if (sensorB.getTempFByIndex(0) > 0 && sensorB.getTempFByIndex(0) < 212)
            actualSensorBTemp = sensorB.getTempFByIndex(0);
        Serial2.print("SensorA: ");
        Serial2.println(actualSensorATemp);
        Serial2.print("SensorB: ");
        Serial2.println(actualSensorBTemp);
        Serial2.print("SensorC: ");
        Serial2.println(actualSensorCTemp);
    }
    vTaskDelete(NULL);
}
void Sensors::SensorInit()
{
    Serial2.println("Setting up onewire devices");
    pinMode(TEMPC_BUS, INPUT);
    pinMode(TEMPB_BUS, INPUT);
    pinMode(TEMPA_BUS, INPUT);
    sensorC.begin();
    sensorC.setResolution(TEMP_PRECISION);
    sensorC.setWaitForConversion(false);
    sensorA.begin();
    sensorA.setResolution(TEMP_PRECISION);
    sensorA.setWaitForConversion(false);
    sensorB.begin();
    sensorB.setResolution(TEMP_PRECISION);
    sensorB.setWaitForConversion(false);
}

Does that look like it is on the right track?

my idea is the constructor will create a task that will be in an infinite loop to always get the latest values from the sensors, and in the main.cpp i can access the latest values at any time.

That looks terrible to me. You have one giant class that does everything, even down to the for(;:wink: loop to replace loop().

Start with the numbered instances of sensors. Any time you have numbers in your variable names you should be using arrays. How would you add one more temperature sensor? You have to edit so many places in both the .h and .cpp files.

And those sensor instances should not all be declared all the time. What if you want to re-use this class in a project that doesn't have any Dallas sensors? There are at least 6 objects simply wasting precious memory space.

The top of your code has a big block of #define. That is worse than using globals, which you said you want to avoid. You end up #including this .h into every .cpp file because each file needs to use one or two of those. Which ones? Well, you don't really know but you include it anyway to be sure. #define is bad because it is un-typed. If you have a float constant that just happens to hold the value 1, let the compiler know it is a float so that it will use floating-point maths with it.

Then your code does nothing useful with the sensors. It doesn't log them to a log file. It doesn't display them on a screen. It doesn't use those sensors to make decisions, drive motors or anything. Many Arduino projects end up with a mess because you put all of the display code and business logic into various functions scattered around. Sometimes a function called displayTemp() will.be driving a motor as well as the display.

If you want to have a big giant Sensors class, use the Adafruit one. Don't write your own.

You said that your original goal was to avoid globals. In a certain way that is not possible because the display needs access to all the sensor values and the business logic also needs all the sensors and the SD card needs all the sensors and it makes sense to gather all the sensor interface code into one .cpp file. But think about what information the modules DON'T need to share. The pin number for each sensor is only needed inside the sensor interface file and in setup(). So write your sensors like every other Arduino library with a beginSensors() function. setup() cam call that and that hides away inside sensorInterface.cpp with all the implementation details like pin numbers and the actual sensor code.

Think about typical maintenance tasks. You know from your job that 95% of your time is spent doing little jobs like "Put the inside temperature on the 2nd page display screen." If you already have that temperature then you only have to modify the display code of page 2. If you have to add a new sensor, you add it where you have an available physical pin, then you get it working with a simple Serial.print(), compile it and run it and test it. Then add it to the log file. Compile and test that. Then add it to the display; test that. Each of those testable steps should only touch one or two source files. You should not have to edit every file in your project before it is testable - that way you are chasing bugs through all your files.

I had a well thought out response and highlighted everything. When i went to post, It made me log in again and I lost it all >:(

So, long story short:

Thank you for taking the time to respond in great detail. I want to acknowledge the time it took you to curate that post.

It may help to kind of understand my project at a 30k foot view.

I have a board that I developed that has the following:
5 songle relays
3 onewire temp sensors (2 will be used as PIDs)
2 ads1115 sensors used as ammeters
2 SSRs
1 bme280
1 Serial display connected to a Nextion Screen

all controlled with an ESP-WROOM-32U. Attached is a picture of the board

The goal of this topic is for me to be able to use a real world example to create a class appropriately and implement it. And in doing that, creating a pattern that I can use for the rest of the classes I intend (currently) to create, leaving me with a framework that I can come back to after some time and not get lost like I just did.

I do want to touch on a couple of points in your post:

That looks terrible to me. You have one giant class that does everything, even down to the for(;:wink: loop to replace loop().

Does having an understanding that I intend on having about 7 of these infinite loops running with different priorities change that?

Start with the numbered instances of sensors. Any time you have numbers in your variable names you should be using arrays. How would you add one more temperature sensor? You have to edit so many places in both the .h and .cpp files.

something like this?

const int SDA = 21;
const int SCL = 22;
const int TEMPA_BUS = 4;  //Onewire1
const int TEMPB_BUS = 23; //Onewire2
const int TEMPC_BUS = 5;  //onewire3
const int TEMP_PRECISION = 9;
const float SEALEVELPRESSURE_HPA = 1013.25;

Adafruit_BME280 bme;
OneWire onewireDevices[3] = {OneWire(TEMPA_BUS), OneWire(TEMPB_BUS), OneWire(TEMPC_BUS)};
DallasTemperature tempSensors[3] = {DallasTemperature(&onewireDevices[0]), DallasTemperature(&onewireDevices[1]), DallasTemperature(&onewireDevices[2])};
Adafruit_ADS1015 adsLegs[2] = {Adafruit_ADS1015(0x48), Adafruit_ADS1015(0x49)};

And those sensor instances should not all be declared all the time. What if you want to re-use this class in a project that doesn't have any Dallas sensors? There are at least 6 objects simply wasting precious memory space.

This one I dont understand. All of these sensors are always getting the latest info so that other possible classes can use them. if the board is on, it should be polling all these guys.

Then your code does nothing useful with the sensors.

Not yet. trying to get the structure down before I go any further.

You said that your original goal was to avoid globals.

To be a little clearer, my goal is to use them correctly. I was declaring everything globally.

Think about typical maintenance tasks.

THIS!!! This is why im here. I had somewhat workable code, came back months later (after I got these new boards) and could not figure out what the hell I was doing. I wanted to switch from using AWS to Azure IoT Hub, and in going through my code, I was constantly thinking:
"What the hell was I doing"
"Why is this here instead of there"
"Why is it breaking all of the sudden"
"Who wrote this"

So I wanted to step back and create something more manageable and this seems like a good first step

IMG_20190628_151913_small.jpg

The problem is, you're not embracing / implementing OOP techniques. You're simply wrapping non-OOP techniques into a class.

You have a house, an object. The house has various rooms, objects. Each object has a purpose. Purposed items are put in objects that have been set aside for that purpose. Some items can be shared across objects, other items are not shared across objects.

Consider, I have an object called plusOne. plusOne takes in a number, does its thing, and outputs a number. What plusOne does on the inside is its own business.

Consider that there is an MPU9250 IMU containing 3 gyro's, three accelerometers, and three magnetometers. I can create an object, a class, that just represents the MPU9250 that I will use to programmatically interact with the MPU9250. That's all that object does.

Does having an understanding that I intend on having about 7 of these infinite loops running with different priorities change that?

Could you elaborate more on the above comment?

gfvalvo:
The problem is, you're not embracing / implementing OOP techniques. You're simply wrapping non-OOP techniques into a class.

To be clear, I understand OOP in general. I don't understand OOP's implementation in C++ and how to implement it correctly on such a small system.

What I was thinking with the Sensors class, is to have something like

Sensors.temps[0]

or Sensors.pressure

to gain access to the values in other code, but have the "class" continuously polling for the latest values, but the rest of the code doesnt need to know how its getting those values.

Generally, you don't want to expose the members of your class like that, it means that client class knows more than it needs to about how you have implemented your class, which is bad news when you decide to change it.

For temps, have a GetTemperature function in the class that returns a float and takes an index. Even that's a bit too tightly bound, maybe GetKitchenTemperature instead.

The point is that you want to be free to change the internals of your class, without changing its interface, its contract with its clients.

Does having an understanding that I intend on having about 7 of these infinite loops running with different priorities change that?

You didn't #include your operating system although I did see the code for it in the for(;:wink: loop. Most Arduino projects do not rely on an operating system to support multiple infinite loops. The overhead of an operating system is not good for a system that controls physical things like motors.

something like this?

Yes that uses arrays. But this system has some purpose, doesn't it? One temperature is inside temperature, or one is the freezer or whatever. Why not give them names for what they do or where they are placed?

You can use names with arrays too. So sometimes you can use the sensors as an array. For example, the low-level fetchTemperatures() routine just needs to know how many there are, not their names. So your code would look like...

void heaterOutputs() {
  if(tempSensor[frontOven] < frontOvenHeaterThreshold) heatOn(frontOvenHeater);
  ...
}

...

void fetchTemperatures() {
  for(int thermometerNumber = 0; thermometerNumber < NumberOFThermometers; thermometerNumber++) {
  fetchTemperature(thermometerNumber);
}

Isn't that a thousand times easier to read than sensors[17]?

Once again, look at Adafruit_Sensors.h. See how they are able to switch out the type of sensor in just one place and the rest of the code doesn't care what type it is? Imagine you change the system some months from now and you put in a BME280 in a position that previously had a DS18B20. Using that library, none of the other code needs to change.

Yes that uses arrays. But this system has some purpose, doesn't it? One temperature is inside temperature, or one is the freezer or whatever. Why not give them names for what they do or where they are placed?

You can use names with arrays too. So sometimes you can use the sensors as an array. For example, the low-level fetchTemperatures() routine just needs to know how many there are, not their names. So your code would look like...

It used to have a specific purpose. Today, there are 2 distinct use cases for this board. One is to control brewing beer, and the other is to control distilling spirits.

so Temp Sensor A can be either in the HLT for brewing, or in the column for distilling. that is why I changed everything to be more generic.

I am looking at the Adafruit_Sensor.h file and see there are virtual functions, and a virtual class. does that mean that I cannot implement the class directly, but instead inherit from it?

I can see that working for my ADCs and the BME, but not seeing it yet for theOneWire busses. Still learning.

The best example of a virtual function is in the Print class. This is what you are calling when you do a Serial.print() or Wire.print() or LCD.print().

When Print was written, the designers knew it was going to be used for Serial. So it could have been part of the Serial class. But they also knew that Arduino enthusiasts are going to want to print to screens and all sorts of other devices. Way back in the 2000's there was no LCD screen for Arduino. So how did they make .print(x,BIN) work for the screen you bought last week?

Look in the library that came with that screen (or any other device using Print). They didn't have to include a full copy of the Print class. That would be a disaster if Arduino changed the Print class and some of your devices use the new one and some use the old one. There would be an unbelievable number of incompatible .print() methods circulating around your Arduino and the forums would be full of questions and problems with .print(). [Have you ever seen anyone complain that they can print something to Serial but not to their LCD? Never.]

The LCD library - and every other library that needs to use .print() - inherits the Print class. So all the methods like .print() and .println() are always available. But that early-2000's library still doesn't know how to print to the new hardware. It has one single virtual function which is intended to send one single character to the output device. The LCD class contains the "real" function which actually sends the character to the LCD. The rest of the stuff, like BIN and HEX stays in the Print class where it belongs.

Note that there's actually a couple of layers I'm skipping here. The Stream class is an important mediator in front of Print.

Adafruit_Sensors takes this to the next level with many virtual functions. You have a lot of different thermometers that may return Celcius or Farenheit. You don't want to bother the high-level code with the conversions and the details of OneWire and everything else. So you hand the details over to the Adafruit_Sensors library (using the sensor-specific library) and your high-level code gets the readings it wants in the units it wants.