Process Multiple "receive" at once

[Soloam]

Hello all, I have a node that contains contains 2 sensors, 2 lights to actuate! This node responds to a income message from a MQTT Gateway that communicates with OpenHab. So far so good, I can turn on and off the lights on the node with OpenHab.

The problem starts when I want to turn off both lights at the same time, for example a kill all lights switch. I can see on my broker that the message is sent, and they both arrive to my node, the problem is that sometimes one of the lights does not work. It's proving to be very hard to debug this issue, because the problem only occurs once in a while. So I ask:

Has any one had any kind of problems dealing with multiple income messages on the same response node? When I send the message one by one I rarely (and I mean almost never, or never) have a missed message, arriving to the node. I can't understand if the problem is one of the messages getting lost, or the code dropping one of them... Because when I'm in debug it works...

Thank You

[Yveaux]

@soloam share your sketch first. Extra eyes might spot an error in there ;-)

[Soloam]

Ok, this is my Light Actuator Node, I'm developing a all in one solution, so the code ended up bit complex and a complete mess, I still need to clean it up...

// Enable debug prints to serial monitor

//#define MY_DEBUG
//#define MY_DEBUG_X

// Enable and select radio type attached
#define MY_RADIO_NRF24

// Enabled repeater feature for this node
#define MY_REPEATER_FEATURE

//#define MY_NODE_ID 1

#include <SPI.h>
#include <MySensors.h>
#include <Bounce2.h>


#define RELAY_START_ID 1
#define SWITCH_START_ID 10

// EDIT THIS VALUES--------------------------------
#define REFRESH_INTERVAL 30000
#define RETRY_REPEATS 5

#define MESSAGE_REPEATS 5
#define MESSAGE_REPEATS_DELAY 10

int RELAYS[] = { 0, 6 }; //5,6
int SWITCH[] = { 7, 8 }; //7,8

// ------------------------------------------------


#define RELAY_ON  1
#define RELAY_OFF 0

const int numberOfActuators = sizeof(RELAYS) / sizeof(int);

Bounce        *debouncers[numberOfActuators];
MyMessage     *messagesRelay[numberOfActuators];
MyMessage     *messagesSwitch[numberOfActuators];
bool          oldvalues[numberOfActuators];
int           errorSending[numberOfActuators];
unsigned long lastRefreshTime[numberOfActuators];


// MY DEBUG -------------------------------------
void SerialPrintln(String text) {
#ifndef MY_DEBUG_X
  return;
#endif

  Serial.println(text);
}

void SerialPrintln(int text) {
#ifndef MY_DEBUG_X
  return;
#endif

  Serial.println(text);
}

void SerialPrint(String text) {
#ifndef MY_DEBUG_X
  return;
#endif

  Serial.print(text);
}

void SerialPrint(int text) {
#ifndef MY_DEBUG_X
  return;
#endif

  Serial.print(text);
}
//-------------------------------------


int relay_child_id(int index) {
  return (index + RELAY_START_ID);
}

int switch_child_id(int index) {
  return (index + SWITCH_START_ID);
}

void setup()
{
  SerialPrint("Loading ");
  SerialPrint(numberOfActuators);
  SerialPrintln(" actuators");

  for (int i = 0; i < numberOfActuators; i++) {
    SerialPrint("Starting Child ");
    SerialPrintln(i);

    //Start Messages
    messagesRelay[i]  = new MyMessage(relay_child_id(i), V_STATUS);
    messagesSwitch[i] = new MyMessage(switch_child_id(i), V_STATUS);

    // Setup the button
    if (SWITCH[i] > 0) {
      pinMode(SWITCH[i], INPUT);
      digitalWrite(SWITCH[i], HIGH); // Activate internal pull-up

      debouncers[i] = new Bounce();//Create Debouncer
      debouncers[i]->attach(SWITCH[i]);
      debouncers[i]->interval(5);

      debouncers[i]->update();
      oldvalues[i] = debouncers[i]->read();//Starts Default Switch State
    }

    // Make sure relays are off when starting up
    if (RELAYS[i] > 0) {
      digitalWrite(RELAYS[i], RELAY_OFF);
      pinMode(RELAYS[i], OUTPUT);
    }
  }
}

void SerialPrintLn(String thisIsAString) {
  SerialPrintln(thisIsAString);
}

void presentation()  {
  char buffer[50];

  // Send the sketch version information to the gateway and Controller
  sendSketchInfo("Light Actuator", "2.0");


  // Register all lights and switches to GW
  for (int i = 0; i < numberOfActuators; i++) {
    if (RELAYS[i] > 0) {
      sprintf(buffer, "Light %d", relay_child_id(i));
      SerialPrint("Presenting ");
      SerialPrintln(buffer);
      present(relay_child_id(i), S_BINARY, buffer);
    }

    if (SWITCH[i] > 0 && RELAYS[i] == 0) {
      sprintf(buffer, "Switch %d", switch_child_id(i));
      SerialPrint("Presenting ");
      SerialPrintln(buffer);
      present(switch_child_id(i), S_BINARY, buffer);
    }
  }
}

bool messageRepeat(MyMessage &message, bool ack = true) {
  int repeat = 1;
  int repeatdelay = 0;
  int index;

  index = relay_index_id(message.sensor);
  if (index < 0) {
    index = switch_index_id(message.sensor);
  }

  if (index < 0) {
    SerialPrintln("ERROR IN INDEX");
    return false;
  }

  SerialPrint("Sending message of child ");
  SerialPrintln(message.sensor);


  while (repeat <= MESSAGE_REPEATS) {
    if (send(message, ack)) {
      SerialPrintln("Send OK");
      errorSending[index] = 0;
      return true;
    } else {
      SerialPrint("Send ERROR ");
      SerialPrintln(repeat);
    }

    repeat++;
    repeatdelay = repeatdelay + MESSAGE_REPEATS_DELAY;
    wait(repeatdelay);
  }

  if (ack == true)
    errorSending[index]++;

  return false;
}

void status_update(int index) {
  int value;
  bool resend;
  if (errorSending[index] > RETRY_REPEATS) {
    errorSending[index] = 0;
    return;
  } else if (errorSending[index] > 0) {
    SerialPrint("Lets send again ");
    SerialPrintln(index);
    resend = true;
  } else {
    resend = false;
  }

  if ( ( lastRefreshTime[index] == 0 ) or (millis() - lastRefreshTime[index] >= REFRESH_INTERVAL) or ( resend == true ) )
  {
    if (lastRefreshTime[index] == 0) {
      lastRefreshTime[index] = millis();
    }

    SerialPrint("Sending Update of index ");
    SerialPrintln(index);

    //Update Relay
    if (RELAYS[index] > 0) {
      value = digitalRead(RELAYS[index]);
      messageRepeat(messagesRelay[index]->set(value == RELAY_ON ? true : false), resend);
    }

    //Update Button
    if (SWITCH[index] > 0 && RELAYS[index] == 0) {
      debouncers[index]->update();
      value = debouncers[index]->read();
      messageRepeat(messagesSwitch[index]->set(value), resend);
    }

    lastRefreshTime[index] = millis();
  }
}

void processLight(int index) {
  bool value;

  //Update State Of Light In Controller
  status_update(index);

  if (SWITCH[index] <= 0)
    return;

  debouncers[index]->update();
  value = debouncers[index]->read();

  //Process Switch
  if (value != oldvalues[index]) {
    //Process Relays
    if (RELAYS[index] > 0)
      lightToggle(index);

    //Process Switch
    if (SWITCH[index] > 0 && RELAYS[index] == 0)
      messageRepeat(messagesSwitch[index]->set(value), true);
  }

  oldvalues[index] = value;

  //Checks If Debounce State Changed During Process
  debouncers[index]->update();
  value = debouncers[index]->read();

  if (value != oldvalues[index]) {
    SerialPrintln("Value Changed During Process");
    processLight(index);
  }
}

void lightToggle(int index) { //Toggles Light State
  if (RELAYS[index] <= 0)
    return;

  int value = digitalRead(RELAYS[index]);
  lightUpdate(index, (value == LOW ? HIGH : LOW));
}

void lightUpdate(int index, int value) { //Updates Light State
  if (RELAYS[index] <= 0)
    return;

  //Changes Light State
  digitalWrite(RELAYS[index], value);

  SerialPrint("--> Relay Light ");
  SerialPrint(RELAYS[index]);
  SerialPrint(" is ");
  SerialPrintln(value);

  //Sends Message To Controller
  messageRepeat(messagesRelay[index]->set(value == RELAY_ON ? true : false));
}


void loop()
{
  for (int i = 0; i < numberOfActuators; i++) {
    processLight(i);
  }
}

int relay_index_id(int sensor) {
  if (sensor < RELAY_START_ID)
    return -1;
  else if (sensor >= SWITCH_START_ID)
    return -2;
  else if (sensor > numberOfActuators)
    return -3;
  else
    return sensor - RELAY_START_ID;
}

int switch_index_id(int sensor) {
  if (sensor < SWITCH_START_ID)
    return -1;
  else if (sensor >= ( SWITCH_START_ID + numberOfActuators) )
    return -2;
  else
    return sensor - SWITCH_START_ID;
}

void receive(const MyMessage &message) {
  int index;

  //Only Process V_STATUS Message
  if (message.type != V_STATUS)
    return;

  //Only Process Relay Status
  index = relay_index_id(message.sensor);
  if (index < 0)
    return;

  if (message.isAck()) {
    SerialPrintln("--------------------");
    SerialPrintln("->>>> Ack To Relay Message <<<<<-");
    SerialPrint("Sensor: ");
    SerialPrint(message.sensor);
    SerialPrint(" Index: ");
    SerialPrint(relay_index_id(message.sensor));
    SerialPrint(" Status: ");
    SerialPrintln(message.getBool());
  } else {
    if (RELAYS[index] <= 0)
      return;

    digitalWrite(RELAYS[index], message.getBool());
    SerialPrintln("--------------------");
    SerialPrintln("New Status Message");
    SerialPrint("Sensor: ");
    SerialPrint(message.sensor);
    SerialPrint(" Index: ");
    SerialPrint(relay_index_id(message.sensor));
    SerialPrint(" Status: ");
    SerialPrintln(message.getBool());

    //Updates Status On Other Controllers
    messageRepeat(messagesRelay[index]->set(message.getBool()));
  }
}

[mfalkvidd]

@soloam there is probably too much work going on in receive() (or rather in the functions it calls), so processing of the first message is not complete when the second message arrives and receive is called again.

Treat the receive function similar to an interrupt service routine; do minimal work (set a flag or something like that) and then do the work in loop based on which flag(s) are set.

[Soloam]

Thank you @mfalkvidd I will try to remove the message update to other controllers from the receive, it might be the problem... It was the last update that I made to the code. And the rest of the code is a simple digital write to turn on the light. Do you think that even that should go to the loop?

So a question, when a message arrives and the previews as not ended, will it continue with the first after the end of the second? I think that is the behavior of the interrupt.

Thank You

[mfalkvidd]

@soloam I think it will continue, but I think MySensors only allocates ram for one message, so when the first call continues the message has been overwritten with the contents of the second message. Not 100% sure about that though.

DigitalWrite could be quick enough to not cause problems, but I am not sure. You could try saving millis() between the first and second call and see how much time there is. Don't print the time to serial within receive though, since that might take too long :)

[Soloam]

@mfalkvidd Thank you, I will review the code to take that into consideration. I don't upload the code with the debug flag on (MY_DEBUG_X), I only turn it on to debug.

[Yveaux]

@soloam I had a chat with @mfalkvidd and we think the issue is the wait() call in messageRepeat. If a new message is pending, the wait() call will call receive() again with an overwritten message.

[Soloam]

@yveaux thank you for the input. I'll try to remove the message repeat from the recive function! I'll post back the results!

Thank you @Yveaux and @mfalkvidd

[mfalkvidd]

@soloam I just noticed that you're setting the ack parameter to true when sending (inside the repeat function). Setting ack to true will result in the message being echoed back, which will cause the receive function to be called.

Set it to false instead, and your code will function almost the way you intended. Setting ack to true will have absolutely no effect on the return value of send().

[Soloam]

@mfalkvidd thank you. I set the ack to true from a early stage of the code, that I ended up changing and removing the digitalwrite to the main loop! I never removed the true! I'll try it out. Thank you for the replay.

[Soloam]

@mfalkvidd and @Yveaux I made de replacement so that the code does not send the ack and simplify the receive... and the result was very positive, not 100%, but very close... If I spam on and off on the kill switch I get, very rarely a error of one of the lights to stay on or off when it all the others change. I think that probably when I spam. I'll review the code to try to improve it further, but for now this will work, no one will spam the button like crazy...

Thank You

[Yveaux]

@soloam good to hear! Thanks for reporting back :+1: