Vehicle daytime running lights control

For a fun project, I've gone into installing and controlling daytime running lights in my fun ride, a 2004 VW golf 5 speed.

I've noticed on vehicles that have these lights, that they extinguish or dim on the side of the vehicle that the signal light is being used. I assume the purpose of that is to allow the signal light to be prominent. Thus the need for control in my project.

Thus far my code only has 3 functions:

  1. Detect when the 'set up' pushbutton has been depressed for 5 seconds or more This is to eliminate the possible entry into 'set up' by an unintentional switch depression.

  2. When in 'set up', the user turns the signal light on and the code gathers the next 4 low pulses of the signal light signal. The code averages the 4 and adds 25% to establish a time threshold and uses that threshold to determine if the signal light is flashing off or has been turned off.

  3. Under normal running condition (not in 'set up'), It is intended that the daytime running light turns off when a signal light is turned on and stays off until it is determined that the signal light has been turned off.; the daytime running light to go back on at that time.

The first 2 functions work correctly. I'm somewhat frustrated with the 3rd. I might stumble upon my error(s) after some more hours of trying this, trying that, but have selfishly decided to implore the help of my fellow forum members to avoid the additional time and frustration.

The problem I'm having is that my daytime running light led indicator (pin 9) doesn't illuminate and the boolean 'lights_on' never goes true when the 'signal light signal has gone low time' (signal_off_mark) plus the time threshold (signal_off_threshold) is less than millis().

To emulate the signal light signal, I have pin 8 connected to an Attiny85 outputting an astable frequency pulse to an output pin (pulled low with 10k). The board I'm using is a Nano powered via USB. All leds are on a small board with integrated resistors.

The code is below. Please ignore the unused globals. That will be cleaned up once this stupid mistake is pointed out to me.

boolean set_up = false;
boolean timer_started = false;
boolean PB_switch_depressed = false;
int signal_average = 0;
const byte signal_pin = 8;
const byte set_up_PB = 4;
const byte set_up_led = 17;
const byte set_up_done_led = 10;
const byte running_lights = 9;
unsigned long set_up_PB_depressStart = 0;
int debounce_time = 50;
int long_depress_time = 5000;
int signalCounter = 0;   // counter for the number of button presses
//int signalState = 0;         // current state of the button
int last_signalState = 1;
int cumulative_signal_duration = 0;
int signal_duration = 0;
int average_signal_duration = 0;
int signal_duration_array[4];
byte s = 0;
unsigned long timeout = 3000000;
boolean signal_detect = false;
unsigned long signal_off_mark = 0;
int run_signal_duration = 0;
boolean signalState = true;
boolean previous_signalState = true;
boolean lights_on = false;
int signal_off_threshold = 0;



void setup() {
  Serial.begin(9600);
  pinMode (set_up_PB, INPUT_PULLUP);
  pinMode (set_up_led, OUTPUT);
  pinMode (running_lights, OUTPUT);
  // put your setup code here, to run once:
}

void loop() {
  read_set_up_PB();
  get_signals();
  normal_run();
  Serial.print("signal detect = ");
  Serial.println(signal_detect);
  Serial.print("signal State = ");
  Serial.println(signalState);
  Serial.print("signal off threshold = ");
  Serial.println(signal_off_threshold);
  Serial.print("millis = ");
  Serial.println(millis());
  Serial.print("signal off mark = ");
  Serial.println(signal_off_mark);
  Serial.print("                        Lights On = ");
  Serial.println(lights_on);
}


void normal_run() {
  signalState = digitalRead(signal_pin);
  if (signalState == HIGH) {
    digitalWrite(running_lights, LOW);
  }
  if (signalState == LOW && signalState != previous_signalState) {
    signal_off_mark = millis();
    if ((signal_off_mark + signal_off_threshold) < millis()) {
      lights_on = true; //debugging
      digitalWrite(running_lights, HIGH);
    }
  }
  previous_signalState = signalState;
}
/*This function enables the set_up function only if the set up push button is
 * depressed for a periond of 5 seconds or longer. It's purpose is to eliminate
 * any accidental depressions of the set up pushbutton, i.e. "are you sure".
 */
void read_set_up_PB() {
  PB_switch_depressed = !digitalRead (set_up_PB);
  if (PB_switch_depressed) {
    timer_started = true;
  }
  else {
    timer_started = false;
  }

  if (!timer_started) {
    set_up_PB_depressStart = millis();
  }
  if (timer_started && ((millis() - set_up_PB_depressStart) > long_depress_time)) {
    digitalWrite(set_up_led, HIGH);
    set_up = true;
  }
}
/*This function collects 4 signal light off durations and calculates their average.
 * It then adds another 25% to that average as a threshold to distinquish when the
 * signal light has been turned off compared to the signal flashing off when in use.
 */
void get_signals() {
  if (set_up) {
    cumulative_signal_duration = 0;
    for (unsigned int i = 0; i < 4; i++) {
      signal_duration_array[i] = (pulseIn(signal_pin, LOW, timeout) / 1000);
      cumulative_signal_duration = (cumulative_signal_duration + signal_duration_array[i]);
      average_signal_duration = (cumulative_signal_duration / (i + 1));
      s++;
    }
    if (s >= 3) {
      for (int p = 0; p < 4; p++) {
        Serial.print("---------------------------------------------------------------------------------------");
        Serial.print("Array element ");
        Serial.print(p);
        Serial.print(" = ");
        Serial.println(signal_duration_array[p]);
      }
    }
    Serial.print("Average Signal Duration = ");
    Serial.println(average_signal_duration);
  }
  signal_off_threshold = (average_signal_duration * 1.25);
  set_up = false;
  digitalWrite(set_up_led, LOW);
}

(At this point, adding Serial ouput resulted in too many characters. See attachment.)

Hope I haven' left anything out. Appreciate any assistance, thanks - Scotty.

Serial output.txt (3.11 KB)

  read_set_up_PB();
  get_signals();
  normal_run();

So, regardless of whether the setup switch has been pressed, determine whether the turn signals have been activated, and then, regardless of whether or not they are, do whatever a normal run means.

I don't follow that logic.

One or more of those functions should return a value. That value should be used to determine what else to do.

      signal_duration_array[i] = (pulseIn(signal_pin, LOW, timeout) / 1000);

(What) (are) (the) (parentheses) (for) (?)

      average_signal_duration = (cumulative_signal_duration / (i + 1));

Why are you computing the average after each reading? It makes more sense to take several readings and then produce an average.

  signalState = digitalRead(signal_pin);

How IS the switch wired? Why isn't the pin declared as an INPUT pin?

  if (signalState == LOW && signalState != previous_signalState) {
    signal_off_mark = millis();
    if ((signal_off_mark + signal_off_threshold) < millis()) {
      lights_on = true; //debugging
      digitalWrite(running_lights, HIGH);
    }
  }

The turn signal switch has just been activated (or deactivated). What is this supposed to do?

signal_off_millis is set to now. Then, now plus sometime later needs to be less than now. What are the odds of that ever happening?

So, regardless of whether the setup switch has been pressed, determine whether the turn signals have been activated, and then, regardless of whether or not they are, do whatever a normal run means.

Yes, for now. Later a boolean will be set to ensure a 'set up' has been done before 'normal running' can function.

determine whether the turn signals have been activated, and then, regardless of whether or not they are

No, that second function does not determine if the turn signals have been activated. It gathers turn signal duration information and only does so if the boolean set_up has been set by depressing the push button for 5 seconds or more.

One or more of those functions should return a value. That value should be used to determine what else to do.

I've used global booleans instead.

(What) (are) (the) (parentheses) (for) (?)

pulseIn(signal_pin, LOW, timeout) These parentheses are, according to this forum's Reference section, the proper format to use for this function.
The outer parentheses are used to enclose a calculation.

Why are you computing the average after each reading? It makes more sense to take several readings and then produce an average.

True, I could change that. Thanks for pointing that out.

How IS the switch wired? Why isn't the pin declared as an INPUT pin?

One side of the switch is wired to ground, the other to pin 4. Aren't I/O in a default state of INPUT unless otherwise declared?

The turn signal switch has just been activated (or deactivated). What is this supposed to do?

To detect a state change. If it has gone from HIGH to LOW, I want to start measuring the time it remains low. If it remains low for longer than the signal_off_threshold it indicates the turn signals have been turned off.

signal_off_millis is set to now. Then, now plus sometime later needs to be less than now. What are the odds of that ever happening?

I don't understand why you think that cannot happen. Isn't that very 'happening' observed in the serial output?

millis is set to now

. Yes, but in the next millisecond, now is old and stored in a variable called signal_off_mark. The value of that variable plus the value of signal_off_threshold is being compared to millis() (now) which has now gone way beyond what was 'now' before. Perhaps this is where my logic is faulty and where the intended results fail.

Thanks for responding

I've used global booleans instead.

That was dumb. You should keep the scope of all variables at a minimum.

There is overhead in calling a function. Calling the function, and having the function decide whether it should do anything is a waste, when you can decide, in loop(), whether the function even needs to be called.

One side of the switch is wired to ground, the other to pin 4.

But, you are not enabling the internal pullup resistor, or using an external one, so you have a floating pin condition. All bets are off as far as proper functioning of the switch.

Aren't I/O in a default state of INPUT unless otherwise declared?

Yeah, but so what? There is no harm in explicitly defining the switch as INPUT, and doing so makes it clear that you know what you are doing.

To detect a state change.

That's what the if statement is doing. What is the code in the body of the if statement supposed to do?

I want to start measuring the time it remains low. If it remains low for longer than the signal_off_threshold it indicates the turn signals have been turned off.

By sitting there doing nothing until that happens? That's a waste. When the switch becomes activated, record when that happens. The rest of what that statement is supposed to do needs to be dependent only on the start time having been recorded.

Yes, but in the next millisecond, now is old and stored in a variable called signal_off_mark.

True, but, by then, you have moved on and loop() will have been called again, and the turn signal switch will never again become activated, without having become deactivated, so the if statement will never again be true, until after the turn signals have been turned off.