Serial Communication with Arduino to control Relays --- Code not entering 'If' statement

Hello,

I am attempting to upload code on an Arduino where I can receive "instruction sets" through serial communication telling the Arduino which relay to activate, when to activate it (in the form of time to wait after instruction received), and how long to keep it activated. The instructions will come in continuously, so it should not wait to finish executing an instruction before moving to the next one, rather, it should store the instructions and keep looping and execute the instructions based on the time on the board.

I originally generated the code using ChatGPT, and went through it line by line and did a fair amount of editing. I am posting the attempt at this below. Note it's got way more printing to the serial monitor than needed, because I was trying to see where the issue is.

Ultimately, what is happening is that the if statement where its supposed to be entering the code when the current time is between the activation time of the relay, and the total duration after the activation time --- that's not being entered. So it's an issue with the logic, but I just can't spot it. What is weird is that if the activation time is no delay after current time, then it enters it and runs for the duration. That is expected, but why not the same for "future activation times"?

Originally, the condition had one more condition to check to ensure that the activation time is not 0 in order to run, however, I will add that back after first figuring out what is going on here.

In terms of solutions I tried, I did the same code except declaring a unsigned long for both activation time and duration - same results, and also tried doing something simple - taking a hard coded instruction set and removing the instruction structure and seeing if I could get it to behave, I could not.

const int numRelays = 4; // Number of relays
const int relayPins[numRelays] = {2, 5, 6, 7}; // Define relay pins
const int defaultState = HIGH; // Default relay state (HIGH for negative logic)

struct RelayInstruction {
  int relayPin;
  float activationTime; 
  float duration; 
};

const int maxInstructions = 10; // Maximum number of pending instructions
RelayInstruction instructions[maxInstructions]; // Array to store relay instructions
int numInstructions = 0; // Number of pending instructions

void setup() {
  Serial.begin(9600); // Initialize serial communication
  for (int i = 0; i < numRelays; i++) {
    pinMode(relayPins[i], OUTPUT); // Set relay pins as OUTPUT
    digitalWrite(relayPins[i], defaultState); // Set the default relay state
  }
}

void loop() {
  if (Serial.available() > 0) {
    String command = Serial.readStringUntil('\n'); // Read data from MATLAB
    processCommand(command); // Process the received command
  }

  // Check and execute relay instructions
  unsigned long currentTime = millis();

  for (int i = 0; i < numInstructions; i++) {
    if (currentTime >= instructions[i].activationTime){
      Serial.println("Activation time started");
    }
    if (currentTime < instructions[i].duration){
      Serial.println("Duration ongoing");
    }
    if (currentTime >= instructions[i].activationTime &&
        currentTime <  instructions[i].duration) { 
      digitalWrite(instructions[i].relayPin, LOW); // Activate relay (LOW for negative logic)
      Serial.println("The relay should be turn off right now");
    } else {
      digitalWrite(instructions[i].relayPin, HIGH); // Deactivate relay (HIGH for negative logic)
      // Clear the instruction once it's completed
      instructions[i].activationTime = 0;
      instructions[i].duration = 0;
    }
  }

  // Dispose of executed instructions
  numInstructions = removeExecutedInstructions(instructions, numInstructions);
}

void processCommand(String command) {
  // Split the command received from MATLAB
  String parts[3];
  int partIndex = 0;
  int startIndex = 0;
  for (int i = 0; i < command.length(); i++) {
    if (command.charAt(i) == ',') {
      parts[partIndex] = command.substring(startIndex, i);
      startIndex = i + 1;
      partIndex++;
    }
  }
  parts[partIndex] = command.substring(startIndex);

  if (partIndex == 2 || partIndex == 3) {
    int relayPin = parts[0].toInt();
    Serial.println("Relay Pin: ");
    Serial.println(relayPin);
    unsigned long activationTime = parts[1].toFloat();
    Serial.println("Activation Time: ");
    Serial.println(activationTime);
    unsigned long duration = (partIndex == 3) ? parts[2].toFloat() : 0.0;
    Serial.println("Duration: ");
    Serial.println(duration);

    if (relayPin >= 0 && relayPin < numRelays) {
      // Valid instruction received, add it to the instructions array
      if (numInstructions < maxInstructions) {
        instructions[numInstructions].relayPin = relayPins[relayPin];
        unsigned long ct = millis();
        instructions[numInstructions].activationTime = (activationTime) + ct;
        Serial.println("Activation Time adjusted is: ");
        Serial.println(instructions[numInstructions].activationTime);
        instructions[numInstructions].duration = duration + instructions[numInstructions].activationTime;
        numInstructions++;
      } else {
        // Max instructions reached
        Serial.println("Max instructions reached");
      }
    } else {
      // Invalid relay pin received
      Serial.println("Invalid relay pin received");
    }
  } else {
    // Invalid command received
    Serial.println("Invalid command received");
  }
}

// Function to remove executed instructions from the array
int removeExecutedInstructions(RelayInstruction array[], int size) {
  int writeIndex = 0;
  for (int readIndex = 0; readIndex < size; readIndex++) {
    if (array[readIndex].activationTime != 0) {
      // Copy non-executed instructions to the writeIndex position
      array[writeIndex] = array[readIndex];
      writeIndex++;
    }
  }
  return writeIndex;
}

Hi @pbarduino1 ,
Welcome to the forum..

change both floats to type unsigned long, which is what millis returns..
that's going to change things drastically..
make that change first..

curious, what board??

have fun.. ~q

ChatGPT has obviously not read the fundamental Arduino tutorial Serial Input Basics.

Follow the methods of that tutorial to format, read and parse the serial data and you won't go wrong.

1 Like

Hi Qubits-us, thanks for the welcome & the response

So I had tried changing it to an unsigned long to see if that would work already, but now that you point it out, I realize I completely missed changing the data type in the structure, I had only changed it during the assignment. Also, now that I did a quick Google search on casting data types in C, I’m wondering if even that part I did wrong.

As I’m using .toFloat() to convert the string, should I be casting it using (unsigned long)activationTime and same for the duration variable?

Thanks again for pointing it out, I’m on my phone right now but I’ll try that on the computer tomorrow

Btw— the board is an Uno and I’m pretty sure R3

Thanks for the link, cattledog. Excellent tutorial. I had a feeling the baud rate might have been slow but I assumed the readStringUntil function inside the process command function would have taken care of that. Checking Google, apparently that function is non-blocking?? Surprising.

Still, given there wouldn’t be a command to process until the command variable reaches the newline character, and duration also printed out on my serial monitor just fine, I’m still a little confused as to where the gap is. One thing the tutorial mentioned that I may have forgotten is checking that the serial monitor was ending with a newline character when I tried manual instruction entry. At the same time, I got the correct parsed data so I am not sure how that would be possible if a newline was not added on.

Reading the tutorial, I definitely see some simpler ways to write this code, but at the same time, I can’t figure out the specific area where in the existing code that the problem occurs. Even if I were to rewrite this, for educational purposes would be great to figure out why this code does not work

Hello pbarduino1

Welcome to the worldbest Arduino forum ever.

I assume that you have written the programme partly by yourself, then it is quite easy to find the error.

There's a trick to figuring out why something isn't working:

Use a logic analyzer to see what happens.
Put Serial.print statements at various places in the code to see the values of variables, especially ones that control the motors, and determine whether they meet your expectations.

Have a nice day and enjoy coding in C++.

Serial.readStringUntil('\n');

Checking Google, apparently that function is non-blocking??

Wrong. The function will wait (block) for a timeout if '/n' is not seen. The default timeout is 1 second. It's not a long period of time, but it is considered blocking.

https://www.arduino.cc/reference/en/language/functions/communication/serial/readstringuntil/

1 Like

since you tried ChatGPT you might consider this approach. tested with my HW using different pins. shorter variable names make the code easier to read

the cmd is 3 values. multiple cmds separated and terminated with comma

10 0 1000, 12 0 2000, 13 1000 500, 11 5000 1000, 13 5000 500,

output:

addIntr: pin 10, 227527 start, 228527 stop
loop: set pin 10
addIntr: pin 12, 227537 start, 229537 stop
loop: set pin 12
addIntr: pin 13, 228598 start, 229098 stop
addIntr: pin 11, 232644 start, 233644 stop
addIntr: pin 13, 232690 start, 233190 stop
addIntr: pin 13, 232736 start, 233236 stop
loop: clear pin 10
loop: set pin 13
loop: clear pin 13
loop: clear pin 12
loop: set pin 11
loop: set pin 13
loop: set pin 13
loop: clear pin 13
loop: clear pin 13
loop: clear pin 11
const byte pins [] = { 2, 5, 6, 7};

const int  Npin = sizeof(pins);;

struct Instr {
    int           state;
    int           pin;
    unsigned long msecStart;
    unsigned long msecStop;
};

const int Ninstr = 10;
Instr instr [Ninstr] = {};          // initialize all fields to zero

enum { Available = 0, InUse, Set }; // Available (0) will be default value


enum { Off = HIGH, On = LOW };

char s [80];                // for use with sprintf()

// -----------------------------------------------------------------------------
void
addInstr (
    int           pin,
    unsigned long msecStart,
    unsigned long msecStop )
{
    sprintf (s, "addIntr: pin %d, %lu start, %lu stop",
            pin, msecStart, msecStop);
    Serial.println (s);

    for (int i = 0; i < Ninstr; i++) {
        if (Available == instr [i].state)  {
            instr [i].state     = InUse;
            instr [i].pin       = pin;
            instr [i].msecStart = msecStart;
            instr [i].msecStop  = msecStop;
            return;
        }
    }
    Serial.println ("addIntr: none available");
}

// -----------------------------------------------------------------------------
void loop ()
{
    unsigned long msec = millis ();

    if (Serial.available () > 0) {
        char buf [80];
        int n = Serial.readBytesUntil (',', buf, sizeof(buf)-1);
        buf [n] = '\0';

        int pin;
        unsigned long msecStart;
        unsigned long msecDuration;

        sscanf (buf, "%d %ld %ld", & pin, & msecStart, & msecDuration);
        addInstr (pin, msec + msecStart, msec + msecStart + msecDuration);
    }

    // Check and execute relay instructions
    for (int i = 0; i < Ninstr; i++) {
        switch (instr [i].state)  {
        case InUse:
            if (msec >= instr [i].msecStart) {
                instr [i].state = Set;
                digitalWrite (instr [i].pin, On); 

                sprintf (s, "loop: set pin %d", instr [i].pin);
                Serial.println (s);
            }
            break;

        case Set:
            if (msec >= instr [i].msecStop) {
                instr [i].state = Available;
                digitalWrite (instr [i].pin, Off); 

                sprintf (s, "loop: clear pin %d", instr [i].pin);
                Serial.println (s);
            }
            break;
        }
            
    }
}
// -----------------------------------------------------------------------------
void setup () {
    Serial.begin (9600);

    for (int i = 0; i < Npin; i++) {
        pinMode      (pins[i], OUTPUT);
        digitalWrite (pins[i], Off);
    }
}

Hi gcjr,

This was amazing, thank you. It works like a charm (I tried it on a Wokwi Simulator just now).

I only used C for one semester in college and C++ for one, and that's been (hope no one is counting) close to 10 years now, so I had to resort to ChatGPT to write the code and then go through it to try and recall the syntax of the language. For your code though I am quickly writing some questions to confirm if I've understood correctly. Just also to appreciate your efforts that this is not going to be a copy/paste job for me, but I am trying to learn what you did:

  • This was my first encounter with enums, and I understand that it is mainly for readability?

  • Prior the addInstr() function, You receive commands as pin starttime duration, and you use a blocking function to obtain the string, followed by the sscanf() function to extract it as into a long integer, which for our purpose suits unsigned longs. Because you're blocking only until the comma, it doesn't immediately go to the next set of instructions sitting in the serial bus without moving forward in the loop - correct?

  • The main loop is really constant checking of instructions, and looking at the time stamps in two different if statements so you can easily toggle the states separately. Now, I imagine it is not the case and there is simply a logic issue somewhere in the code I posted -- however, as was in the code, is there typically any issue to set up the if statement as comparing trying to have two conditions to evaluate to true? I can't imagine there would be, but just checking

Once again, thank you for the help here - much appreciated

Hi Paul,

Thanks very much for the welcome.

So I added some Serial print statements as below to just see what is happening when I enter one command. I am getting the values I expect for all, and to ensure that each condition separately is behaving as it should, before the main if statement, I put the two conditions separately (condition one being if activation time has started yet, condition two being that if the duration after the activation time is still ongoing).

In all the Print Line statements I am seeing what I would expect. As expected, if there is a non-zero activation value, it does not enter. However I am not sure why on subsequent loops as the time progresses, why eventually it's not entering. I have been looking down the code to see if something is changing the instructions on subsequent loops, and the only function that can do that is the one to remove executed instructions, but that only deletes zero'd out activation times.

I will check this out a bit further tomorrow. Do let me know if you can give me a hint as to where I should be looking

Thanks & have a great night!

 // Check and execute relay instructions
  unsigned long currentTime = millis();

  for (int i = 0; i < numInstructions; i++) {
    Serial.println("Current Time: " );
    Serial.println(currentTime);
    Serial.println("Activation Time: ");
    Serial.println(instructions[i].activationTime);
    if (currentTime >= instructions[i].activationTime){
      Serial.println("Activation time started");
    }
    if (currentTime < instructions[i].duration){
      Serial.println("Duration ongoing");
    }
    if (currentTime >= instructions[i].activationTime &&
        currentTime <  instructions[i].duration) { 
      digitalWrite(instructions[i].relayPin, LOW); // Activate relay (LOW for negative logic)
      Serial.println("The relay should be turn off right now");
    } else {
      digitalWrite(instructions[i].relayPin, HIGH); // Deactivate relay (HIGH for negative logic)
      // Clear the instruction once it's completed
      instructions[i].activationTime = 0;
      instructions[i].duration = 0;
    }
  }

for maintainability. they are an alternative to separately defining #defines or const. but they can also define a type that has a limited # of values which would require variables of that type to only have those enum variables. the compiler would issue errors if a variable is no assigned a value of that type

not because it's blocking, because it only reads up to the comma and then continues execution.

if it was while (Serial.available()), it would have continued reading and calling addInstr()

i think the code you posted was unnecessarily complicated. There might be a relatively simple fix, but the code is hard to read. i'd like to see the spec given to chatgpt

not sure what this is asking. sounds like if (x < 3 && y > 6), for example

const byte pins [] = {2, 5, 6, 7};
const byte conveyorpin = 3;
char conveyorstring[8];
char speedchar[2];
const int  Npin = sizeof(pins);
int speed;
float dutycycle;
int roundeddutycycle;

struct Instr {
    int           state;
    int           pin;
    unsigned long msecStart;
    unsigned long msecStop;
};

const int Ninstr = 30;
Instr instr [Ninstr] = {};          // initialize all fields to zero

enum { Available = 0, InUse, Set }; // Available (0) will be default value


enum { Off = HIGH, On = LOW };

char s [80];                // for use with sprintf()

// -----------------------------------------------------------------------------
void
addInstr (
    int           pin,
    unsigned long msecStart,
    unsigned long msecStop )
{
    sprintf (s, "addIntr: pin %d, %lu start, %lu stop",
            pin, msecStart, msecStop);
    Serial.println (s);

    for (int i = 0; i < Ninstr; i++) {
        if (Available == instr [i].state)  {
            instr [i].state     = InUse;
            instr [i].pin       = pin;
            instr [i].msecStart = msecStart;
            instr [i].msecStop  = msecStop;
            return;
        }
    }
    Serial.println ("addIntr: none available");
}

// -----------------------------------------------------------------------------
void loop ()
{
    unsigned long msec = millis ();

    if (Serial.available () > 0) {
        char buf [80];
        int n = Serial.readBytesUntil ('\n', buf, sizeof(buf)-1);
        buf [n] = '\0';

        int pin;
        unsigned long msecStart;
        unsigned long msecDuration;

        for (int j = 0;j<8;j++){
          conveyorstring[j]=buf[j];
        }
        conveyorstring[8] = '\0';
        if (strcmp(conveyorstring,"Conveyor") == 0){
          sscanf(buf, "Conveyor %d", & speed);
          analogWrite(conveyorpin,speed);
        }else{
        sscanf (buf, "%d %ld %ld", & pin, & msecStart, & msecDuration);
        addInstr (pin, msec + msecStart, msec + msecStart + msecDuration);
        }
        //memset(conveyorstring,0,sizeof(conveyorstring));
        
        
    }

    // Check and execute relay instructions
    for (int i = 0; i < Ninstr; i++) {
        switch (instr [i].state)  {
        case InUse:
            if (msec >= instr [i].msecStart) {
                instr [i].state = Set;
                digitalWrite (instr [i].pin, On); 

                sprintf (s, "loop: set pin %d", instr [i].pin);
                Serial.println (s);
            }
            break;

        case Set:
            if (msec >= instr [i].msecStop) {
                instr [i].state = Available;
                digitalWrite (instr [i].pin, Off); 

                sprintf (s, "loop: clear pin %d", instr [i].pin);
                Serial.println (s);
            }
            break;
        }
            
    }
}
// -----------------------------------------------------------------------------
void setup () {
    Serial.begin (9600);

    for (int i = 0; i < Npin; i++) {
        pinMode      (pins[i], OUTPUT);
        digitalWrite (pins[i], Off);
    }
    pinMode(conveyorpin,OUTPUT);


   
}

Hi gcjr,

The above issue in my comment - it is still persisting. I started a new topic, on it as it seems to be a new issue unrelated to the question at hand here. If you do have a moment, do check it out and let me know if you have any thoughts. And of course, once again, thank you so much for your help!

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