Code Structure Advice

jamesbarrett · October 28, 2021, 11:32pm

Hi, I'm building a temperature monitor with the ability to switch relays depending on logic generated in node red, my first iteration had lots of repeated code setting each pin to do a function but I've since changed it so i can set any pin to either read temperature from ds18b20 or turn a relay on or off. i was wondering if my code could create memory issues once its in full time use with how its structured, all is working in the short time I've tested it.

Code included below, serial process was copied from another post a while ago for dealing with start and stop characters.

many thanks

#include <OneWire.h>
#include <DallasTemperature.h>

const byte numChars = 32;
char receivedChars[numChars];
boolean newData = false;

#define RELAY_OFF 0
#define RELAY_ON 1

void ProcessSerialData()
{
  static boolean recieveInProgress = false;
  static byte ndx = 0;
  char startMarker = '<';
  char endMarker = '>';
  char rc;

  while(Serial.available() > 0 && newData == false)
  {
    rc = Serial.read();

    if(recieveInProgress == true)
    {
      if(rc != endMarker)
      {
        receivedChars[ndx] = rc;
        ndx++;

        if(ndx >= numChars)
        {
          ndx = numChars - 1;
        }
      }
      else
      {
        receivedChars[ndx] = '\0';
        recieveInProgress = false;
        ndx = 0;
        newData = true;
      }
    }
    else if(rc == startMarker)
    {
      recieveInProgress = true;
    }
  }
}

void ShowNewData()
{
  if(newData == true)
  {
    ProcessSerialInput(receivedChars);
    //Serial.println(receivedChars);
    newData = false;
  }
}

void ProcessSerialInput(String data)
{
  if(data.substring(0, 5) == "relay")
  {
    if(data.substring(7, 9) == "on")
    {
      int p = data.substring(5, 7).toInt();
      RelayControl(p, RELAY_ON);
    }
    else if(data.substring(7, 9) == "of")
    {
      int p = data.substring(5, 7).toInt();
      RelayControl(p, RELAY_OFF);
    }
  }
  else if(data.substring(0, 5) == "tstat")
  {
    if(data.substring(7, 9) == "aa")
    {
      int p = data.substring(5, 7).toInt();
      ProcessTemperature(p);
    }
  }
}

void RelayControl(int Pin, unsigned char Status)
{ 
  pinMode(Pin, OUTPUT);
  digitalWrite(Pin, Status);
  Serial.println("RELAY,PIN," + String(Pin) + "," + String(Status));
}

String GetSensorAddress(DeviceAddress device)
{
  String address;
  address += "[";

  for(uint8_t i = 0; i < 8; i++)
  {
    address += String("0x");

    if(device[i] < 0x10)
    {
      address += String("0");
    }

    address += String(device[i], HEX);

    if(i < 7)
    {
      address += String(", ");
    }
  }

  address += String("]");

  return address;
}

void ProcessTemperature(int Pin)
{
  
  String serialDataTemp = "";
  String address = "";
  float tempC;

  OneWire OneWireBus(Pin);
  DallasTemperature Sensor(&OneWireBus);
  Sensor.begin();
  Sensor.requestTemperatures();
  
  for(int i = 0; i < Sensor.getDeviceCount(); i++)
  {
    tempC = Sensor.getTempCByIndex(i);
    Serial.println("TEMP,PIN," + String(Pin) + "," + String(tempC));
  }
}

void setup()
{
  Serial.begin(115200);
}

void loop()
{
  ProcessSerialData();
  ShowNewData();
}

wildbill · October 28, 2021, 11:46pm

What hardware are you running that on?

gcjr · October 29, 2021, 11:04am

code structure advice

i modified (simplified) your code and tested by simulating your devices

i reordered functions so that they are defined before used
i de-Capitalized symbol names following standard practice to only Capitalize constants
i think it odd that you configure the pin (pinMode()) in relayControl. aren't the pins known in advance?
i simplified processInput() to remove redundant code. however your use of substring limits you to single digit pins.
not sure why you use "<>" when you could just read a \n terminated line using Serial.readBytesUntil().
i simplified processSerialData() so that it restarts whenever if detects a "<" and handles overflow
processSerialData() invokes processInput() when ">" is detected
i left GetSensorAddress(), but it is not used
loop() simply calls processSerialData() (no need for "newData")

hope this is the type of advice you're looking for

consider

#undef MyHW
#ifdef MyHW
typedef int*  DeviceAddress;

struct OneWire  {
    OneWire (int)   { };
};

struct DallasTemperature  {
    DallasTemperature (OneWire *) { };
    void begin (void)  { };
    int  getDeviceCount (void)       { return 3; };
    int  getTempCByIndex (int)       { return 12; };
    int  requestTemperatures (void)  { return 34; };
};

#else
# include <OneWire.h>
# include <DallasTemperature.h>
#endif


#define RELAY_OFF 0
#define RELAY_ON 1

// -----------------------------------------------------------------------------
void relayControl (
    int             pin,
    unsigned char   state)
{
    pinMode (pin, OUTPUT);
    digitalWrite (pin, state);
    Serial.println ("RELAY,PIN," + String(pin) + "," + String(state));
}

// -----------------------------------------------------------------------------
void processTemperature (
    int Pin)
{
    String serialDataTemp = "";
    String address = "";
    float tempC;

    OneWire OneWireBus (Pin);
    DallasTemperature Sensor (&OneWireBus);
    Sensor.begin ();
    Sensor.requestTemperatures ();

    for (int i = 0; i < Sensor.getDeviceCount(); i++)
    {
        tempC = Sensor.getTempCByIndex (i);
        Serial.println ("TEMP,PIN," + String(Pin) + "," + String(tempC));
    }
}

// -----------------------------------------------------------------------------
void processInput (
    String data)
{
    String cmd = data.substring (0, 5);
    int    p   = data.substring (5, 7).toInt();
    String opt = data.substring (7, 9);

    if (cmd == "relay") {
        if (opt == "on")
            relayControl (p, RELAY_ON);
        else
            relayControl (p, RELAY_OFF);
    }

    else if (cmd == "tstat") {
        if (opt == "aa")
            processTemperature (p);
    }
}

// -----------------------------------------------------------------------------
char      receivedChars [80];

void processSerialData ()
{
    static byte ndx = 0;
    const char StartMarker = '<';
    const char EndMarker = '>';

    if (! Serial.available ())
        return;

    char c = Serial.read ();

    if (StartMarker == c)
        ndx = 0;

    else if (EndMarker == c)  {
        receivedChars[ndx] = '\0';
        Serial.println (receivedChars);
        processInput (receivedChars);
    }

    else  {
        receivedChars [ndx++] = c;

        if (sizeof(receivedChars) <= ndx)       // overflow
            ndx = 0;
    }
}

// -----------------------------------------------------------------------------
void setup ()
{
    Serial.begin (115200);
}

// -----------------------------------------------------------------------------
void loop ()
{
    processSerialData ();
}

jamesbarrett · October 29, 2021, 11:18am

I'm running it on a mega 2560. It's a few years old not sure what revision is any.

jamesbarrett · October 29, 2021, 11:28am

It is thank you.

My code is working fine when I call or they turns on or off. Also when I call returns the the temperature for each ds18b20 on that pin via serial which I read and process in node red, what do you mean it will only work on single digit pins.

My plan was to hard code any pins so once I start wiring I can use any digital pin for either temperature or relay control.

I had it working hard coded and the code was so repetitive it was bothering my and I thought I could improve it. Each temperature pin will have 3x ds18b20 these will be in zone and I will average these out incase one is a little out it should stabilise my readings. Then I will turn a relay on with a heater connected. This is a fish room control system.

My worry about the code was that creating DallasTemperature objects on the fly and then possibly using the same pin later as a relay pin code cause some memory issues. I will look at your code and try to understand your improvements after work.

Thank you

jamesbarrett · October 29, 2021, 11:32am

I definitely see the improvement with the if statements. And I see what you mean about the substring and single digits but I've been using pins 22 - 53 with no issues.

gcjr · October 29, 2021, 12:38pm

in my code, i don't capture "<", so the counts are off by one

wildbill · October 29, 2021, 12:42pm

If you have concerns about memory, grab a copy of a freemem or freememory function - Adafruit has one. It'll let you see if you have a leak. Given that you're using String objects in a number of areas of your code, I won't be surprised if you do.

jamesbarrett · October 29, 2021, 4:06pm

Thanks I will look at checking memory usage. And possibly try and eliminate string functions from my code if I can. The system isn't mission critical but I'm trying to improve, I'm a novice programmer, I can get things to work in a few languages but don't actually know them fluently enough to know what I'm doing wrong or what's a better way.

system · April 27, 2022, 4:06pm

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.