millis() delay if statement not functioning as intended

Hello. I am writing a code that (theoretically) takes a serial input message from serial monitor “<Mode, ModeVal, RightInput1, LeftInput1, RightInput2, LeftInput2>” with RightInput2 and LeftInput2 being positive integers representing left and right time in milliseconds. RightInput1 and LeftInput1 are left and right speed integers. In my program, on an Uno, every 20 milliseconds two values speedl and speedr are written to a software serial port to control two motors with their own motor controller. The function I am having trouble with basically takes in the four inputs and is supposed to run at the initial conditions until the time (defined by RightInput2 and LeftInput2) has passed and then when those individual times pass, it turns their respective speeds speedr and speedl to 0 to wait for the next command from the serial monitor, while still running my other function of sending speedr and speedl to the software serial port.

The function that sends speedr and speedl works perfectly but the waiting to send 0 function is not working. I send the string from the serial monitor and the first loop, it changes speedl and speedr to the correct values, but after that, the three if statements that check if the time has passed all become true and it turns the speeds to 0 prematurely. My code is below, and I apologize if I didn’t explain this very well. I tried but it may need clarification.

These pieces of code convert the respective parts of the string to longs

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  RightInput2 = atol(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  LeftInput2 = atol(strtokIndx);     // convert this part to an integer

This is the code that is acting up. The functions that are not working as intended have a //------- next to them:

  int rd = RightInput1;
  int ld = LeftInput1;
  unsigned long rt = RightInput2;
  unsigned long timer;
  unsigned long lt = LeftInput2;
  timeoutTimer = millis();

  if ((ModeVal == 1) && (counter == 0)) {
    timer = millis();
    counter = 1;
    speedr = (rd) / rt;
    speedl = (ld) / lt;
    
#ifdef DEBUG
    Serial.println("DistanceMode Initialized");
#endif
  }
if ((millis() - timer) >= rt) {//----------------------------------------
    speedr = 0;
#ifdef DEBUG
    Serial.println("Right speed OFF");
#endif
  }
 if ((millis() - timer) >= lt) {//----------------------------------------
    speedl = 0;
#ifdef DEBUG
    Serial.println("Left speed OFF");
#endif
  }
 if (((millis() - timer) >= lt) && ((millis() - timer) >= lt)) {//----------------------------------------
    ModeVal = 0;
    speedr = 0;
    speedl = 0;
    counter = 0;
#ifdef DEBUG
    Serial.println("Process killed. all OFF");
#endif
  }

The debugging serial print is as follows with “<hello,1,20000,30000,3000,4000>” being the sample data sent:

DistanceMode Initialized
SpeedL: 7 SpeedR: 6
Right speed OFF
Left speed OFF
Process killed. all OFF
SpeedL: 0 SpeedR: 0
SpeedL: 0 SpeedR: 0
SpeedL: 0 SpeedR: 0
SpeedL: 0 SpeedR: 0
SpeedL: 0 SpeedR: 0
SpeedL: 0 SpeedR: 0

Each of those speed lines is printed 20ms apart but it should be printing the speeds of 7 and 6 about 150 and 200 times respectively…

Thanks for reading!

Everything work correctly. You need to fix logic. At the line #7 you set timer to current millis. Then a few lines below you are comparing it with new millis. But that few lines required a couple dozen microseconds to be executed. So timer always equal to new millis

alesam:
Everything work correctly. You need to fix logic. At the line #7 you set timer to current millis. Then a few lines below you are comparing it with new millis. But that few lines required a couple dozen microseconds to be executed. So timer always equal to new millis

Nice catch! Unfortunately, I just updated that (moved it to inside the first if statement at line #9) and it is still acting the same way.

Could you make a simple but complete sketch which can be compiled and tested. Your "fixed" parts still has errors and uncertain parts (due to missed code).

@alesam Thank you for the request to put together a simplified version for show and tell :slight_smile: Upon copying and pasting each of my function tabs into one sketch tab, I noticed I was re-initializing “timer” every time through the loop. I tried moving the initialization to the global initialization area instead, and now it runs exactly how I was wanting!!! Below is the consolidated version just for reference if anyone is curious. I moved the “unsigned long timer;” from the DistanceMode() function to line #13 in the sketch and changed it to “unsigned long timer = 0;” and that fixed my problem.

const unsigned long sendInterval = 20UL; //These are the intervals for sending every 20ms.
unsigned long sendTimer = 0;
const unsigned long timeoutInterval = 1000UL;
unsigned long timeoutTimer = 0;
unsigned long timer = 0;

int speedl = 0; //Initial output speed
int speedr = 0;
int counter  = 0; //Initial counter state


//Setup for recieving the string and parsing it. This mainly came from the forums(Thank you people!)//
const byte numChars = 32;
char receivedChars[numChars];
char tempChars[numChars];        // temporary array for use when parsing

// variables to hold the parsed data
char Mode[numChars] = {0};
int ModeVal = 0;
int RightInput1 = 0;
int LeftInput1 = 0;
unsigned long RightInput2 = 0;
unsigned long LeftInput2 = 0;

boolean newData = false;
//---------------------------------------------//


void setup() {
  Serial.begin(9600);

  sendTimer =  millis();
  timeoutTimer = millis();
}

void loop() {
  ReceiveData();
  if (ModeVal == 1) DistanceMode();
  Send(); //Always running and printing values to serial port every 20ms
}

void distanceMode() {
  int rd = RightInput1;
  int ld = LeftInput1;
  unsigned long rt = RightInput2;
  unsigned long lt = LeftInput2;
  timeoutTimer = millis();

  if ((ModeVal == 1) && (counter == 0)) {
    timer = millis();
    counter = 1;
    speedr = (rd) / rt;
    speedl = (ld) / lt;

    Serial.println("DistanceMode Initialized");
  }
  if ((millis() - timer) >= rt) {
    speedr = 0;
    Serial.println("Right speed OFF");
  }
  if ((millis() - timer) >= lt) {
    speedl = 0;
    Serial.println("Left speed OFF");
  }
  if (((millis() - timer) >= lt) && ((millis() - timer) >= lt)) {
    ModeVal = 0;
    speedr = 0;
    speedl = 0;
    counter = 0;
    Serial.println("Process killed. all OFF");
  }
}

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

  while (Serial.available() > 0 && newData == false) {
    rc = Serial.read();
    if (recvInProgress == true) {
      if (rc != endMarker) {
        receivedChars[ndx] = rc;
        ndx++;
        if (ndx >= numChars) {
          ndx = numChars - 1;
        }
      }
      else {
        receivedChars[ndx] = '\0'; // terminate the string
        recvInProgress = false;
        ndx = 0;
        newData = true;
      }
    }

    else if (rc == startMarker) {
      recvInProgress = true;
    }
  }
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    // this temporary copy is necessary to protect the original data
    //   because strtok() used in parseData() replaces the commas with \0
    parseData();
    newData = false;
  }
}

void parseData() {      // split the data into its parts

  char * strtokIndx; // this is used by strtok() as an index

  strtokIndx = strtok(tempChars, ",");     // get the first part - the string
  strcpy(Mode, strtokIndx); // copy it to messageFromPC

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  ModeVal = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  RightInput1 = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  LeftInput1 = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  RightInput2 = atol(strtokIndx);     // convert this part to a long

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  LeftInput2 = atol(strtokIndx);     // convert this part to a long
}

void Send() {
  if ((millis() - sendTimer) >= sendInterval) { //Always sends speed commands every interval.
    Serial.print("SpeedL: ");
    Serial.print(speedl);
    Serial.print(" SpeedR: ");
    Serial.println(speedr);
    sendTimer = millis();
  }

  if ((millis() - timeoutTimer) >= timeoutInterval) { //If timeoutTimer hasn't been reset lately, will shut off motors
    speedl = 0;
    speedr = 0;
    Serial.println("TIMEOUT. Shut off active.");
    Serial.print("SpeedL: ");
    Serial.print(speedl);
    Serial.print(" SpeedR: ");
    Serial.println(speedr);
  }
}