code approach advice

hello so i am working with a can module with my car… My approach is if i receive the filtered message and the data byte is equal to 0x032 then i want to send the message 0x305 for 10 seconds, The only problem is when 0x032 is true it returns back to 0x00 after 2 seconds so i need to figure a way to hold that value to keep the ‘if’ statement true for 10 seconds but i do not know the best approach… Thanks for the help.

#include <mcp_can.h>
#include <SPI.h>

long unsigned int rxId;
unsigned char len = 0;
unsigned char rxBuf[8];
unsigned char headlights[7] = {0x00, 0x00, 0xC0, 0x00, 0x10, 0x00, 0x00};
//unsigned char unlock[3] = {0x00, 0x32, 0x00};
//unsigned char CHECK[2] = {0x00, 0x00}; //235
unsigned long previousmillis = 0;
unsigned long fadeout = 10000;
MCP_CAN CAN0(10);                          // Set CS to pin 10

void setup()
{
  Serial.begin(115200);
  if (CAN0.begin(MCP_STDEXT, CAN_33K3BPS, MCP_16MHZ) == CAN_OK) Serial.print("MCP2515 Init Okay!!\r\n");
  else Serial.print("MCP2515 Init Failed!!\r\n");
  pinMode(2, INPUT);                       // Setting pin 2 for /INT input


  //CAN0.init_Mask(0,0,0x07FF0000);                // Init first mask...
  CAN0.init_Filt(0, 0, 0x02600000); // UNLOCK               // Init first filter...
  //CAN0.init_Filt(1,0,0x03050000);    // LIGHTS            // Init second filter...

  CAN0.init_Mask(1, 0, 0x07FF0000);              // Init second mask...
  //CAN0.init_Filt(2,0,02600000);                // Init third filter...
  //CAN0.init_Filt(3,0,0x03050000);                // Init fouth filter...
  //CAN0.init_Filt(4,0,0x01060000);                // Init fifth filter...
  //CAN0.init_Filt(5,0,0x01070000);                // Init sixth filter...

  Serial.println("MCP2515 Library Mask & Filter Example...");
  CAN0.setMode(MCP_NORMAL);                // Change to normal mode to allow messages to be transmitted
}

void loop()
{
  if (!digitalRead(2))                   // If pin 2 is low, read receive buffer
  {

    CAN0.readMsgBuf(&rxId, &len, rxBuf); // Read data: len = data length, buf = data byte(s)
    if (rxBuf[1] == 0x032) {
      CAN0.sendMsgBuf(0x305, 0, 7, headlights);
    }
  }
}

When you receive 0x03 ‘set’ a flag and start a 10 second Timer.

When the flag is ‘set’, do your stuff.

When the Timer times out ‘reset’ the flag.

Hi larry thanks for your reply, I have tried this below with no joy, I must be going wrong somewhere with the code could you be so king to have a look ? Thanks

#include <mcp_can.h>
#include <SPI.h>
boolean flagTimer = true;
long unsigned int rxId;
unsigned char len = 0;
unsigned char rxBuf[8];
unsigned char headlights[7] = {0x00, 0x00, 0xC0, 0x00, 0x10, 0x00, 0x00};

unsigned long currentMillis;
unsigned long lightOnTime = 10000;
unsigned long lightMillis;
MCP_CAN CAN0(10);                          // Set CS to pin 10

void setup()
{
  Serial.begin(115200);
  if (CAN0.begin(MCP_STDEXT, CAN_33K3BPS, MCP_16MHZ) == CAN_OK) Serial.print("MCP2515 Init Okay!!\r\n");
  else Serial.print("MCP2515 Init Failed!!\r\n");
  pinMode(2, INPUT);                       // Setting pin 2 for /INT input


  //CAN0.init_Mask(0,0,0x07FF0000);                // Init first mask...
  CAN0.init_Filt(0, 0, 0x03050000); // UNLOCK               // Init first filter...
  //CAN0.init_Filt(1,0,0x03050000);    // LIGHTS            // Init second filter...

  CAN0.init_Mask(1, 0, 0x07FF0000);              // Init second mask...
  //CAN0.init_Filt(2,0,02600000);                // Init third filter...
  //CAN0.init_Filt(3,0,0x03050000);                // Init fouth filter...
  //CAN0.init_Filt(4,0,0x01060000);                // Init fifth filter...
  //CAN0.init_Filt(5,0,0x01070000);                // Init sixth filter...

  Serial.println("MCP2515 Library Mask & Filter Example...");
  CAN0.setMode(MCP_NORMAL);                // Change to normal mode to allow messages to be transmitted
}

void loop()
{
  currentMillis = millis();
  //Serial.println(currentmillis);
  if (!digitalRead(2))                   // If pin 2 is low, read receive buffer
  {

    CAN0.readMsgBuf(&rxId, &len, rxBuf); // Read data: len = data length, buf = data byte(s)



    if (flagTimer == true && rxBuf[1] == 0x32) {
      flagTimer = false;
      CAN0.sendMsgBuf(0x305, 0, 7, headlights);
      lightMillis = currentMillis;
    }
    if (flagTimer == false && currentMillis - lightMillis >= lightOnTime) {
      flagTimer = true;

    }
  }
}

I am working with a canbus module and need help with the timing, I want to read 0x32 and then set a flag which keeps the message for 10 seconds while the next message is sent. I have tried this but ultimately it shuts down after a couple of seconds… were am i going wrong?

#include <mcp_can.h>
#include <SPI.h>
boolean flagTimer = true;
long unsigned int rxId;
unsigned char len = 0;
unsigned char rxBuf[8];
unsigned char headlights[7] = {0x00, 0x00, 0xC0, 0x00, 0x10, 0x00, 0x00};

unsigned long currentMillis;
unsigned long lightOnTime = 10000;
unsigned long lightMillis;
MCP_CAN CAN0(10);                          // Set CS to pin 10

void setup()
{
  Serial.begin(115200);
  if (CAN0.begin(MCP_STDEXT, CAN_33K3BPS, MCP_16MHZ) == CAN_OK) Serial.print("MCP2515 Init Okay!!\r\n");
  else Serial.print("MCP2515 Init Failed!!\r\n");
  pinMode(2, INPUT);                       // Setting pin 2 for /INT input


  //CAN0.init_Mask(0,0,0x07FF0000);                // Init first mask...
  CAN0.init_Filt(0, 0, 0x03050000); // UNLOCK               // Init first filter...
  //CAN0.init_Filt(1,0,0x03050000);    // LIGHTS            // Init second filter...

  CAN0.init_Mask(1, 0, 0x07FF0000);              // Init second mask...
  //CAN0.init_Filt(2,0,02600000);                // Init third filter...
  //CAN0.init_Filt(3,0,0x03050000);                // Init fouth filter...
  //CAN0.init_Filt(4,0,0x01060000);                // Init fifth filter...
  //CAN0.init_Filt(5,0,0x01070000);                // Init sixth filter...

  Serial.println("MCP2515 Library Mask & Filter Example...");
  CAN0.setMode(MCP_NORMAL);                // Change to normal mode to allow messages to be transmitted
}

void loop()
{
  currentMillis = millis();
  //Serial.println(currentmillis);
  if (!digitalRead(2))                   // If pin 2 is low, read receive buffer
  {

    CAN0.readMsgBuf(&rxId, &len, rxBuf); // Read data: len = data length, buf = data byte(s)



    if (flagTimer == true && rxBuf[1] == 0x32) {
      flagTimer = false;
      CAN0.sendMsgBuf(0x305, 0, 7, headlights);
      lightMillis = currentMillis;
    }
    if (flagTimer == false && currentMillis - lightMillis >= lightOnTime) {
      flagTimer = true;

    }
  }
}

You want to be checking your 10 second delay outside your test for pin 2. That delay check should happen every time through loop()

#include <mcp_can.h>
#include <SPI.h>
boolean flagTimer = true;
long unsigned int rxId;
unsigned char len = 0;
unsigned char rxBuf[8];
unsigned char headlights[7] = {0x00, 0x00, 0xC0, 0x00, 0x10, 0x00, 0x00};

unsigned long currentMillis;
unsigned long lightOnTime = 10000;
unsigned long lightMillis;
MCP_CAN CAN0(10);                          // Set CS to pin 10

void setup()
{
  Serial.begin(115200);
  if (CAN0.begin(MCP_STDEXT, CAN_33K3BPS, MCP_16MHZ) == CAN_OK) Serial.print("MCP2515 Init Okay!!\r\n");
  else Serial.print("MCP2515 Init Failed!!\r\n");
  pinMode(2, INPUT);                       // Setting pin 2 for /INT input


  //CAN0.init_Mask(0,0,0x07FF0000);                // Init first mask...
  CAN0.init_Filt(0, 0, 0x03050000); // UNLOCK               // Init first filter...
  //CAN0.init_Filt(1,0,0x03050000);    // LIGHTS            // Init second filter...

  CAN0.init_Mask(1, 0, 0x07FF0000);              // Init second mask...
  //CAN0.init_Filt(2,0,02600000);                // Init third filter...
  //CAN0.init_Filt(3,0,0x03050000);                // Init fouth filter...
  //CAN0.init_Filt(4,0,0x01060000);                // Init fifth filter...
  //CAN0.init_Filt(5,0,0x01070000);                // Init sixth filter...

  Serial.println("MCP2515 Library Mask & Filter Example...");
  CAN0.setMode(MCP_NORMAL);                // Change to normal mode to allow messages to be transmitted
}

void loop()
{
  currentMillis = millis();
  //Serial.println(currentmillis);
  if (!digitalRead(2))                   // If pin 2 is low, read receive buffer
  {
    CAN0.readMsgBuf(&rxId, &len, rxBuf); // Read data: len = data length, buf = data byte(s)

    if (flagTimer == true && rxBuf[1] == 0x32) {
      flagTimer = false;
      CAN0.sendMsgBuf(0x305, 0, 7, headlights);
      lightMillis = currentMillis;
    }
  }
  if (flagTimer == false && currentMillis - lightMillis >= lightOnTime) {
    flagTimer = true;
  }
}
     //when flagTimer is true this means timing is disabled
     if (flagTimer == true && rxBuf[1] == 0x32) //if disabled, did we get 0x32
     {
      flagTimer = false; // enable timing (false = enabled)
      lightMillis = currentMillis; //restart timing
      rxBuf[1] = 0x00; //remove character 
     }

     //if the timing is enabled, has the 10 second timer expired
     if (flagTimer == false && currentMillis - lightMillis < lightOnTime) 
    {
       CAN0.sendMsgBuf(0x305, 0, 7, headlights);     // may need to add a short delay between transmits 
    }

   else
    {
      flagTimer = true; // disable Timing
    }

Note, it’s best to do something like this:

#define ENABLED false
#define DISABLED true

Then do this:

if (flagTimer == ENABLED && rxBuf[1] == 0x32)

@happyshow

TOPIC MERGED.

Please do NOT cross post / duplicate as it wastes peoples time and efforts to have more than one post for a single topic.

Continued cross posting could result in a time out from the forum.

Could you take a few moments to Learn How To Use The Forum.
Other general help and troubleshooting advice can be found here.
It will help you get the best out of the forum.

thanks for all the help with the code guys its really appreciated, And as for @ballscrewbob apologies it wont happen again :slight_smile:

larryd:
Note, it’s best to do something like this:

#define ENABLED false
#define DISABLED true

Then do this:

if (flagTimer == ENABLED && rxBuf[1] == 0x32)

I agree about defining the states. I think the arbitrary way the variable got named and the assignments led to an unnecessarily confusing state of affairs. The OP just took the advice and ran with it, without much thought.

It is a flag, it is not itself a timer. It doesn't have a time, it has a state. Also it does not "time flags". So call it a state. Flags really don't need to be called "flags", their role is obvious when used correctly. Also, timers might be enabled/disabled, but their primary states are running/not running.

So I propose something more like:

#define STOPPED false
#define RUNNING true

Then do this:

if (timerState == RUNNING and rxBuf[1] == 0x32)

It may seem like a trivial complaint, but when you get a big thicket of code to plow through, the more obvious the code can be made to read, the easier it is to focus on actual program functionality and data flow.

Another style you could use is to fold the description into the variable name, like:

timerIsRunning = true;
...
if (timerIsRunning and rxBuf[1] == 0x32)

Ouch :wink:

You are right got of course.