Minimum time for digital signal reading

Hello!
I have been working on an electronic exhaust relay controller for some time and while it works fairly well, I would like to make one improvement.
The signal that goes to the Uno R3 is a LOW, sent by the car's ECU. The Arduino then checks in which state the exhaust valve is, when necessary it then activates one relay to send the exhaust motor one or the other way. Unfortunately, the reading is instant and I have found that for some reason the ecu sometimes changes the signal for a very brief moment.

What I am trying to accomplish is a check function that delays the relay switching function for, say 500ms, and if the signal is still HIGH or LOW, activates the switching.
I have tried for hours to work with both the millis() option and the Filters.h library, but with no succes.

the following code is 100% my own, but if you like it its yours to use.

// Digital I/O Pin Constants
const int S1 = 2;    // Control input
const int R1 = A1;    // Relay R1
const int R2 = A2;    // Relay R2
#include <EEPROM.h>
int eeprom = 0;
int ledPin = 13;
int ValveOPEN;  //variable
int On; //variable

// Initialising pins
void setup() { 
ValveOPEN = EEPROM.read(eeprom);
pinMode(S1, INPUT_PULLUP);
pinMode(R1, OUTPUT);
pinMode(R2, OUTPUT);
digitalWrite(R1,HIGH);
digitalWrite(R2,HIGH);
pinMode(ledPin, OUTPUT);
  digitalWrite(R1,HIGH);
  digitalWrite(R2,HIGH);
  // from here both relays are inactive until change in input S1
  delay(1000);
}

void loop(){  
On = digitalRead(S1);
ValveOPEN = EEPROM.read(eeprom);
digitalRead(ValveOPEN);
digitalRead(On);

 //start monitoring S1

if((ValveOPEN) == 0 && (On) == HIGH){
    digitalWrite(R1,HIGH);
    digitalWrite(R2,LOW);
    digitalWrite(ledPin,LOW);
    delay(2000);
        digitalWrite(R2,HIGH);
  EEPROM.update(eeprom,255);
}

if((ValveOPEN) == 255 && (On) == LOW){
  digitalWrite(R1,LOW);
  digitalWrite(R2,HIGH);
  digitalWrite(ledPin,HIGH);
  delay(2000);
    digitalWrite(R1,HIGH);
  EEPROM.update(eeprom,0);
}
}

If your experiments with millis() &c. has the same kind of basic mistakes your code above has, no wonder you got nowhere.

For example:

digitalRead(On);

Read a digital pin, the number of which pin is stored in a variable, then do nothing with the result…

You need to read more basic examples. Your code has many things you will never see in a functioning program. You seem to be a bit ahead of your skis, that’s OK, no one was born knowing this stuff and it can be very demanding, demanding of a perfection we are not accustomed to in other communications.

But first, what is to the roll of the eeprom in your design?

a7

Hello
Each used delay() will kill the functionality of your sketch.

sounds like you want to debounce the input, but not in the same way you debounce a switch. you could in/decrement a counter based on it's value and only take action when the counter reaches a certain value or returns to zero. you probably don't need to read very frequently, maybe every half second

don't understand why you store the state in an eeprom. could you explain?

no need to repeatedly read eeprom if ValveOpen state updated in your conditionals that change the relays and leds.

what do the following do?

perhaps a more meaningful name than "On".

i prefer using an enum to define the relay Off/On state. i think yours are

enum { Off = HIGH, On = LOW };

you could do same for LEDs which I think are the same as above (no need for RlyOff and LedOff)

it's convention that only constants start with a Capital

alto777,
Maybe the code is not conventional, but I can assure you it works just as intended. I have some experience in coding in assembly language (advanced automotive custom tuning), but not so much with C.
The digitalRead(On) is used in the first and second if statement.

the Eeprom function makes sure that the exhaust valve is only returned to the closed position if the car was shut off with the valve open. If the car is shut off , the Arduino will remember the state of the exhaust valve and not try to force it shut or open when it already is.

Yes, some sort of debounce is what I was trying to accomplish.
[/quote]
perhaps a more meaningful name than "On".
[/quote]
It could have been named ECUsignal or something alike, I wrote On at the time. This is the first time I am sharing the code so maybe I can update it a bit so it is clearer.

I agree on the multiple eeprom reads, it is redundant since it is already read in the setup. The variable ValveOPEN is a flag that is only set after the actuation of the valve.

// Digital I/O Pin Constants
const int S1 = 2;    // Control input
const int R1 = A1;    // Relay R1
const int R2 = A2;    // Relay R2
#include <EEPROM.h>
int eeprom = 0;
int ledPin = 13;
int valveOpen;  //variable
int ecuSignal; //variable

// Initialising pins
void setup() { 
valveOpen = EEPROM.read(eeprom);
pinMode(S1, INPUT_PULLUP);
pinMode(R1, OUTPUT);
pinMode(R2, OUTPUT);
digitalWrite(R1,HIGH);
digitalWrite(R2,HIGH);
pinMode(ledPin, OUTPUT);
  digitalWrite(R1,HIGH);
  digitalWrite(R2,HIGH);
  // from here both relays are inactive until change in input S1
  delay(1000);
}

void loop(){  
ecuSignal = digitalRead(S1);
digitalRead(valveOpen);
digitalRead(ecuSignal);

 //start monitoring S1

if((valveOpen) == 0 && (ecuSignal) == HIGH){
    digitalWrite(R1,HIGH);
    digitalWrite(R2,LOW);
    digitalWrite(ledPin,LOW);
    delay(2000);
        digitalWrite(R2,HIGH);
  EEPROM.update(eeprom,255);
}

if((valveOpen) == 255 && (ecuSignal) == LOW){
  digitalWrite(R1,LOW);
  digitalWrite(R2,HIGH);
  digitalWrite(ledPin,HIGH);
  delay(2000);
    digitalWrite(R1,HIGH);
  EEPROM.update(eeprom,0);
}
}

You're still missing the point, what do you think this line does?

digitalRead(ValveOPEN);

It reads the valve flag status and uses it in the "if". I don't want the relays to activate when the valve has already turned.

No, it doesn't. If you're lucky, it does absolutely nothing. Compare it to the line above it.

Please don't edit your original code - it makes nonsense of the comments that follow. When you change it, just put the new version in your latest post.

Okay I get your point. I was rewriting the code in my reply to make it more readable for th forum but forgot to change 2 lines. No need to get all fussy about it.

My question was how to add a debounce-alike function to the incoming signal, the code is functional and has been implemented in my car for over 2 years. I am just irritated that once in a while the valve cycles on its own when a fluctuation in the signal occurs. Cant seem to find the cause of that but must be some kind of electrical interference (only does it on motor on). I can't even see it on the multimeter so when adding a 500ms delayed second check, I am confident my problem is solved. But how to code it (preferably more elegant than I can come up with)?

Excuse us. Your code is nonsense. Or was. No point in worrying about it now.

Please post the full code that has been working perfectly for years, in a new post and we start again from scratch.

a7

Cleaned up the code, just tested it and still works. I changed the eeprom read part as well.

// Digital I/O Pin Constants
const int S1 = 2;    // Control input
const int R1 = A1;    // Relay R1
const int R2 = A2;    // Relay R2
#include <EEPROM.h>
int eeprom = 0;
int ledPin = 13;
int valvestatus;  //variable
int ecusignal; //variable


// Initialising pins
void setup() { 
valvestatus = EEPROM.read(eeprom);
pinMode(S1, INPUT_PULLUP);
pinMode(R1, OUTPUT);
pinMode(R2, OUTPUT);
pinMode(ledPin, OUTPUT);
  digitalWrite(R1,HIGH);
  digitalWrite(R2,HIGH);
}

void loop(){  
ecusignal = digitalRead(S1);
digitalRead(valvestatus);
digitalRead(ecusignal);

 //start signal monitoring

if((valvestatus) == 0 && (ecusignal) == HIGH){
    digitalWrite(R1,HIGH);
    digitalWrite(R2,LOW);
    digitalWrite(ledPin,LOW);
    delay(2000);
        digitalWrite(R2,HIGH);
  EEPROM.update(eeprom,255);
  valvestatus = 255;
}

if((valvestatus) == 255 && (ecusignal) == LOW){
  digitalWrite(R1,LOW);
  digitalWrite(R2,HIGH);
  digitalWrite(ledPin,HIGH);
  delay(2000);
    digitalWrite(R1,HIGH);
  EEPROM.update(eeprom,0);
  valvestatus = 0;
}
}

Those lines are not doing anything. What do you think they are doing?

a7

I don't understand that you can't see how they are being read first and the result is used just 1 line below. If you say it does not do anything, how come I can trigger the relays for 2 seconds when I ground pin S1? Both "if" statements work just fine! Is my approach that uncommon?

So you haven’t published the code that worked for some time, yet.

As for digitalRead, see

https://www.arduino.cc/reference/en/language/functions/digital-io/digitalread/

a7

It is right in the opening post. User gcjr made the kind suggestion that the reading of the eprom over and over again in the loop is pointless so I removed it there and added the value that is being put in the eprom after the relays have switched in the flag variable.
Same result, but less reading of the eprom in the loop.

I tried using HIGH and LOW for this part before, but it doesn't work in combination with the Eeprom. the digitalRead is reading a variable, not a pin.

digitalRead() doesn't have such a feature. :roll_eyes:

To be fair, digitalRead can take a variable as an argument, which variable should be a pin number that digitalRead will, well, read.

But if the result is not stored or used in an expression somehow, it is simply amounts to doing nothing at all.

Except if digitalRead misbehaves when asked to check a pin value that is nonsense, like 255, dunno what it does or doesn’t do in that case.

@tijn1987 srsly, look at the documentation for digitalRead without delay. I mean do it now.

a7

Yeah, I decided he thought he would be read "to this variable" in just digitalRead(var)

OK, I see everyone just getting over the no-op lines.

Here's the code without it. I can see that it would work and do something.

const int S1 = 2;    // Control input
const int R1 = A1;    // Relay R1
const int R2 = A2;    // Relay R2

#include <EEPROM.h>
int eeprom = 0;
int ledPin = 13;
int valvestatus;
int ecusignal;

void setup() {
  valvestatus = EEPROM.read(eeprom);

  pinMode(S1, INPUT_PULLUP);
  pinMode(R1, OUTPUT);
  pinMode(R2, OUTPUT);
  pinMode(ledPin, OUTPUT);
  
  digitalWrite(R1, HIGH);
  digitalWrite(R2, HIGH);
}

void loop() {
  ecusignal = digitalRead(S1);

  if (valvestatus == 0 && ecusignal == HIGH) {
    digitalWrite(R1, HIGH);
    digitalWrite(R2, LOW);
    digitalWrite(ledPin, LOW);
    delay(2000);
    digitalWrite(R2, HIGH);
    EEPROM.update(eeprom, 255);
    valvestatus = 255;
  }

  if (valvestatus == 255 && ecusignal == LOW) {
    digitalWrite(R1, LOW);
    digitalWrite(R2, HIGH);
    digitalWrite(ledPin, HIGH);
    delay(2000);
    digitalWrite(R1, HIGH);
    EEPROM.update(eeprom, 0);
    valvestatus = 0;
  }
}

As for your fix, it might be a fix. On the other hand, there could be hardware issues that could also be addressed.

a7