RS485 multidrop using EasyTransfer, need pointers on how to improve code

Other Hardware Home Automation
1

Hi!

I'm an inexperienced hobby programmer and would really like some feedback from you more experienced guys.

This is code that I will use in a grow-controller in addition to several other things I have planned for later ( Remote monitoring sensor data on a boat, battery under voltage guard, fire alarm, with SMS/phone alerts etc)

All the code is capable of now is sending back temperature and humidity, controlling relays and reading the outputs to the relays to confirm the state they are supposed to be in.

Please review my code and tell me what can be improved, maybe I did something really stupid that has to be done another way. :slight_smile:

Positive and especially negative feedback is very much appreciated!

Code from master:

/* A sketch for controlling relays, requesting and processing sensor information over RS485
   with the help of:
   EasyTransfer     ( REALLY great help )
   Brought to you by:
   Bill Porter
   www.billporter.info
*/
#include <EasyTransfer.h>
#include <SoftwareSerial.h>

#define SSerialRX        10  //Serial Receive pin
#define SSerialTX        11  //Serial Transmit pin

/*-----( Declare objects )-----*/
SoftwareSerial RS485Serial(SSerialRX, SSerialTX); // RX, TX  // Debug serial

EasyTransfer ETin, ETout; 

struct SEND_DATA_STRUCTURE{       // Variables being sent to slaves
  int address;
  int id;
  int port;
  int highlow;
};

struct RECEIVE_DATA_STRUCTURE{    // For receiving sensor values etc.
  int id;
  int port;
  int buttonstate;
};

RECEIVE_DATA_STRUCTURE rxdata;
SEND_DATA_STRUCTURE txdata;

//Reading ID's
int relay = 0;                 
int temp =  1;
int hum  =  2;
int dr   =  3;

int waitDelay = 100;


void setup(){
  Serial.begin(115200);        // Start serial for RS485
  RS485Serial.begin(115200);   // Debug software serial
  
  // Start the library, pass in the data details and the name of the serial port.
  ETin.begin(details(rxdata), &Serial);   // Initiate easytransfer for sending
  ETout.begin(details(txdata), &Serial);  // Receiving
}

void loop(){
  delay(waitDelay);
  
  // RS485(ADDRESS,ID,PORT/SENSOR, H/L)
  
  // A loop to send requests one after another to allow me to test speed and possible hickups
  RS485(1, temp, 0, HIGH);  // Request TemperatureSensor[0] from address 1, HIGH or LOW does nothing on temperature or humidity ID's.
  delay(waitDelay);
  RS485(1, hum, 0, HIGH);   // Request HumidityReading[0] from address 1.
  delay(waitDelay);
  RS485(1, relay, 1, HIGH); // Tell address 1 to turn relay 1 HIGH.
  delay(waitDelay);
  RS485(1, dr   , 1, LOW);  // Request address 1 to digitalRead relay 1 output. HIGH/LOW does nothing, will be used to confirm if it is requested state.
  delay(waitDelay);
  RS485(1, relay, 1, LOW);  // Tell address 1 to turn relay 1 LOW.
  delay(waitDelay);
  RS485(1, dr   , 1, LOW);
  delay(waitDelay);
  RS485(1, relay, 2, HIGH);
  delay(waitDelay);
  RS485(1, dr   , 2, LOW);
  delay(waitDelay);
  RS485(1, relay, 2, LOW);
  delay(waitDelay);
  RS485(1, dr   , 2, LOW);
  delay(waitDelay);
  RS485(1, relay, 3, HIGH);
  delay(waitDelay);
  RS485(1, dr   , 3, LOW);
  delay(waitDelay);
  RS485(1, relay, 3, LOW);
  delay(waitDelay);
  RS485(1, dr   , 3, LOW);
  delay(waitDelay);
  RS485(1, relay, 4, HIGH);
  delay(waitDelay);
  RS485(1, dr   , 4, LOW);
  delay(waitDelay);
  RS485(1, relay, 4, LOW);
  delay(waitDelay);
  RS485(1, dr   , 4, LOW);
  delay(waitDelay);
  RS485(1, relay, 5, HIGH);
  delay(waitDelay);
  RS485(1, dr   , 5, LOW);
  delay(waitDelay);
  RS485(1, relay, 5, LOW);
  delay(waitDelay);
  RS485(1, dr   , 5, LOW);
  delay(waitDelay);
  RS485(1, relay, 6, HIGH);
  delay(waitDelay);
  RS485(1, dr   , 6, LOW);
  delay(waitDelay);
  RS485(1, relay, 6, LOW);
  delay(waitDelay);
  RS485(1, dr   , 6, LOW);
  delay(waitDelay);
  RS485(1, relay, 7, HIGH);
  delay(waitDelay);
  RS485(1, dr   , 7, LOW);
  delay(waitDelay);
  RS485(1, relay, 7, LOW);
  delay(waitDelay);
  RS485(1, dr, 7, LOW);


}

//Function for sending and receiving data

void RS485(int address, int id, int port, int highlow){
  txdata.address = address;          // fill up transmit structure for request
  txdata.id = id;                    
  txdata.port = port;
  txdata.highlow = highlow;
  ETout.sendData();                  // Send data to address
  delay(10);
  for(int i = 0; i < 5; i++){        // Loop to be sure to catch data// maybe not neccesary when using hardware serial?
    if(ETin.receiveData()){          // Look for incoming data

        if(rxdata.id == dr){           // If received ID is dr : digitalRead
        RS485Serial.print(" Read port: ");
        RS485Serial.print(rxdata.port);    // Print what port was read
        RS485Serial.print(" H/L: ");
        RS485Serial.println(rxdata.buttonstate);   // Print what state port was
      }

      if(rxdata.id == relay){        // If id == relay, print port info
        RS485Serial.print("Port: ");
        RS485Serial.print(rxdata.port);
        RS485Serial.print(" Sent state: ");
        RS485Serial.print(rxdata.buttonstate);
        if(rxdata.buttonstate != txdata.highlow){      // Check to see if sent and received state matches
          RS485Serial.print("  Sent and received state dont match! ");
        }
      }

      if(rxdata.id == temp){
        if(rxdata.buttonstate == true){  //   If true requested sensor does not exist at node 
          RS485Serial.println("Sensor does not exist");
        }
        else{                                //  Print sensor data
          RS485Serial.print("Reading: ");
          RS485Serial.print("Temperature");
          RS485Serial.print(" Sensor: ");
          RS485Serial.print(txdata.port);  // Sensor ID
          RS485Serial.print(" Temp: ");
          RS485Serial.println(rxdata.port);  // Sensor reading
        }
      }

      if(rxdata.id == hum){
        if(rxdata.buttonstate == true){     // If true requested sensor does not exist at node
          RS485Serial.println("Sensor does not exist");
        }
        else{                              //  Print sensor data
          RS485Serial.print("Reading: ");
          RS485Serial.print("Humidity");
          RS485Serial.print(" Sensor: ");
          RS485Serial.print(txdata.port);    // ID of sensor
          RS485Serial.print(" Humidity: ");
          RS485Serial.println(rxdata.port);  // Sensor reading
        }
      }
    }
    delay(10);
  }
}

Code from slave 1:

The only problem I seem to be having is located in the master's code :

if(rxdata.buttonstate != txdata.highlow){ // Check to see if sent and received state matches
RS485Serial.print(" Sent and received state dont match! ");
}
}

txdata.highlow which is passed from the loop to the function, often says its 0, even though its actually HIGH as the led on the slave in addition to the digitalRead afterwards confirms it.

Here is my debug output:

Reading: Temperature Sensor: 0 Temp: 2000
Reading: Humidity Sensor: 0 Humidity: 2970
Port: 1 Sent state: 1 Read port: 1 H/L: 1
Port: 1 Sent state: 0 Read port: 1 H/L: 0
Port: 2 Sent state: 1 Read port: 2 H/L: 1
Port: 2 Sent state: 0 Read port: 2 H/L: 0
Port: 3 Sent state: 1 Read port: 3 H/L: 1
Port: 3 Sent state: 0 Read port: 3 H/L: 0
Port: 4 Sent state: 1 Read port: 4 H/L: 1
Port: 4 Sent state: 0 Read port: 4 H/L: 0
Port: 5 Sent state: 1 Read port: 5 H/L: 1
Port: 5 Sent state: 0 Read port: 5 H/L: 0
Port: 6 Sent state: 1 Sent and received state dont match! Read port: 6 H/L: 1
Port: 6 Sent state: 0 Read port: 6 H/L: 0
Port: 7 Sent state: 1 Read port: 7 H/L: 1
Port: 7 Sent state: 0 Read port: 7 H/L: 0

On port 6 you can see the problem occour. Where it occurs is random, and some loops it doesn't happen at all.

This is not a dealbreaker for me as I can just read the port afterwards instead of getting the info sent back right after setting the port HIGH/LOW, but it would be nice to know what may be causing the problem so I can learn something new today too. :slight_smile:

Thanks to everyone that uses their time to review my code!

2

just some remarks,

int relay = 0;
int temp = 1;
int hum = 2;
int dr = 3;

should be const int // as it never changes...
or const uint8_t

even better you should define them in a separate .h file that is included by both sender/receiver to enforce they are same on both ends.

int waitDelay = 100;
same

struct SEND_DATA_STRUCTURE{ // Variables being sent to slaves
int address;
int id;
int port;
int highlow;
};

could probably be shorter by using uint8_t
struct SEND_DATA_STRUCTURE{ // Variables being sent to slaves
uint8_t address;
uint8_t id;
uint8_t port;
uint8_t highlow;
};

would reduce communication by half

RECEIVE ... same

3

RS485(1, dr , 1, LOW);

where can I see the value of "slave[1].dr[1]"? I would expect something like

int value = RS485(1, dr , 1, LOW);

that would allow the sketch to process the value received from the slave (e.g. logging)

int RS485(uint8_t address, uint8_t id, uint8_t port, uint8_t highlow)
{
  txdata.address = address;
  txdata.id = id;
  txdata.port = port;
  txdata.highlow = highlow;
  ETout.sendData();


  for (int i = 0; i < 5; i++) //
  {
    delay(10);
    ETin.receiveData();  // can't you just check if all bytes are in?
  }

  switch (rxdata.id)
  {
    case dr:     return rxdata.buttonstate;
    case relay:  return rxdata.buttonstate;
    case temp:   return rxdata.port;
    case hum:    return rxdata.port;
  }
  return -999; // should never happen, sort of error state
}

consider making the RECEIVE_DATA_STRUCTURE
an union depending on the value of id or simpler

==>

struct RECEIVE_DATA_STRUCTURE {
uint8_t id;
uint8_t port;
float value; // float can hold all types of values. so even decimal part of a temp sensor.
};

4

missing a link to the EasyTransfer library....

5

could add

DEVICE:
const uint8_t uptime = 4; // returns the uptime of the slave
const uint8_t freeRAM = 5; // returns the freeRAM of the slave
const uint8_t EEPR = 6; // read / write EEPROM of slave?

META:
const uint8_t requestCount = ...; // returns # requests handled

just thinking out loud

6

should be const int // as it never changes...
or const uint8_t

even better you should define them in a separate .h file that is included by both sender/receiver to enforce they are same on both ends.

Will do! Will certainly make it alot easier to keep things organized across every device.
Didn't even think about the space saving by using the right type of variable.

RS485(1, dr , 1, LOW);

where can I see the value of "slave[1].dr[1]"? I would expect something like

int value = RS485(1, dr , 1, LOW);

that would allow the sketch to process the value received from the slave (e.g. logging)

To be implemented :smiley: I have been debugging the send/receive function for a little while, but since I found a solution to my problem I will now use your code sample to return data back to the controllers logic. ( Which I will begin working on soon )

Solution:

void RS485(uint8_t address, uint8_t id, uint8_t port, uint8_t highlow){
  txdata.address = address;          // fill up transmit structure for request
  txdata.id = id;                    
  txdata.port = port;
  txdata.highlow = highlow;
  ETout.sendData();                  // Send data to address
  waitForData = true;              
 // delay(1);
 //for(int i = 0; i < 8; i++){        // Loop to be sure to catch data// maybe not neccesary when using hardware serial?
  while(waitForData){
    if(ETin.receiveData()){          // Look for incoming data
        if(rxdata.id == dr){           // If received ID is dr : digitalRead
        RS485Serial.print(" Read port: ");
        RS485Serial.print(rxdata.port);    // Print what port was read
        RS485Serial.print(" H/L: ");
        RS485Serial.println(rxdata.buttonstate);   // Print what state port was
        waitForData = false;
      }

I solved it by setting boolean waitForData = true; after sending, then a while(waitForData) before checking serial. And then after reading data setting the waitForData = false;.

I now can have 0 delay in my sketch and it never says "Sent and received state dont match!" its right every time! It just stops for a little while when reading the DHT22.

DEVICE:
const uint8_t uptime = 4; // returns the uptime of the slave
const uint8_t freeRAM = 5; // returns the freeRAM of the slave
const uint8_t EEPR = 6; // read / write EEPROM of slave?

META:
const uint8_t requestCount = ...; // returns # requests handled

I appreciate you thinking out loud very much!

Found some code for checking free ram:

int freeRam () {
extern int __heap_start, *__brkval;
int v;
return (int) &v - (__brkval == 0 ? (int) &__heap_start : (int) __brkval);
}

Is this something that will use alot of resources to check whats available?
I don't really know too much about the inner workings of an AVR, but that's were i'm going to start focus more soon.

Thank you very much for your help, one more step forwards for me! :slight_smile:

7

Also added a timeout for receiving an answer from the node:

while(waitForData){
unsigned long dataMillis = millis();

if(dataMillis - previousMillis > dataTimeout){
previousMillis = dataMillis;
waitForData = false;
RS485(txdata.address, txdata.id, txdata.port, txdata.highlow);
}

Havent tested it yet, but guess it will work. Will probably add maximum number of retries, logging of failed attempts and possibly an sms/email alert if something critical wont turn on/off.

8

Is this something that will use alot of resources to check whats available?

No, its purpose is to detect memory leaks e.g. in sensor drivers ...
2 calls should give the same value back.

another think out loud one...

const uint8_t CONF = 7; // read configuration of slave

somehow returns a bitfield (or something) indicating types of sensors available.
or slaves should be able to return a "not supported" flag or so.