Tacho- I need an output signal a specified rpm

[Cyber]

Hi Jeff
Thanks for your advice, I will try the latest code as soon as I get some spare time, Very busy at work at the moment.

 

[Pommie]

This all seems rather complicated. If you have a variable that changes every half second then you can include it in your if statement and it'll just work.

Mike.
Here's an example using the built in LED.
BTW, flashing at 1Hz is very slow. Try changing 500 to 100 for a better flash rate.

void setup() {
  pinMode(LED_BUILTIN, OUTPUT);
}

int RPM=100;         //change this to turn flash on/off.
bool show=true;
long lastChange=0;

void loop(){
  if((millis()-lastChange)>=500){ //change state every 500mS (flash rate)
    lastChange=millis();          //remember when we changed
    show=!show;                   //change state
  }
  if((RPM>0) && (RPM<200) && show)        //if ALL are true
    digitalWrite(LED_BUILTIN,HIGH);       //turn on LED
  else                                    //otherwise
    digitalWrite(LED_BUILTIN,LOW);        //turn it off
}

 

[Jeff Wahaus]

This all seems rather complicated.

Really? I wrote the code to be very readable and understandable. The code example you give "works" but has glitches. The initial on or off state will have a random duration as the start of your timer isn't tied to the "flash led" condition. Also, the led may not light as soon as the condition occurs, with your code is it random.

 

[Pommie]

You do realize that your code has the same glitch. Plus, when millis rolls over you LED will have a fixed state for a very long time. Also, your readable and understandable code doesn't reference rpm at all! It just seems to be an over complicated LED flasher.

Mike.
BTW, maybe we could ask the mods for a code critique section so you can have more fun.

 

[Jeff Wahaus]

You do realize that your code has the same glitch.

Well on the first call it doesn't but on subsequent calls it would. That's easy enough to fix by seting timer_cmp = 0 in the case where your done flashing the LED.

Plus, when millis rolls over you LED will have a fixed state for a very long time.

No, that's not correct. timer_cmp rolls over too so it will still work correctly when millis() rolls over

Also, your readable and understandable code doesn't reference rpm at all!

You have great command of the obvious, sir!

It just seems to be an over complicated LED flasher.

Sorry, I don't find it complicated at all. It works as it should, I find it useful to create functions instead of clouding up the main loop with the LED timer details.

Your code has three issues:
1) The initial duration glitch is one.
2) When the trigger condition hits (rpm < 200) your code may (or may not 50/50 chance) start in the "off" state. So if the RPM's drop below 200 for a half second or less and then go back up above, your code may not even turn on the LED in this case.
3) Your code will have issues and stop working when millis() rolls over.

 

[Jeff Wahaus]

Here is the fixed Flash_LED function (with hopefully formatting this time):

 

[Jeff Wahaus]

How do you post code and keep the formatting in place? I cheated by posting a screen shot of my editor.

 

[Jeff Wahaus]

Here is an optimized version of that function. I think it may be a bit harder to understand for a C novice.
It is functionally equivalent to the longer version:

 

[Pommie]

You are assuming that your code gets called at least once per millisecond. If someone using your code has delays elsewhere (as most beginners do) then millis() can wrap and become very much smaller than timer_cmp. If on the other hand you calculate elapsed time and use that then the unsigned maths takes care of any wrapping.

    if((millis()-lastRead)>rate){
        lastRead=millis();
       .....

To keep code formatted use [code] before it and [/code] after it.

Mike.

 

[Jeff Wahaus]

You are assuming that your code gets called at least once per millisecond.

No, there is no requirement to call that frequently.

Consider the math. Say for example that millis() is getting close to wrapping and has a value of 4294967000. (4294967295 is largest number before it wraps)
My code calculates timer_cmp to be

timer_cmp = millis() + rate;

so, with a rate of 500 ms this becomes

timer_cmp = 4294967000 + 500;

which results in timer_cmp wraping
timer_cmp = 295;

so when millis() wraps some 205ms later it will become = 0, and then 295ms later timer_cmp matches (500ms later)

So the real issue here (which I don't really consider an issue) is that once every 49.7 days (assuming the system has been running this long) the LED would flash rapidly (more likely appear to remain lit) for a period of between 1 to 499ms and then resume normal flashing. In this example the duration would be 205ms.
This issue occurs because of the statement "if (millis() >= timer_cmp)" which becomes true as soon as timer_cmp wraps.

The wrap condition can be handled as a special case but the complexity involved isn't warranted considering the benign result. The probability of this race condition even occurring at all is actually much less than once every 49.7 days since it would only occur if the LED was in the flashing condition during this (500ms every 49.7 day) window.

 

[Pommie]

Consider if timer_cmp=4294967295 and millis is one less. If your routine is called 2mS later then millis() will be zero and timer_cmp=4294967295 - no flashing for 49 days.
I appreciate that in this case it is unlikely to be a problem but in other code it can be.

The wrap condition can be handled as a special case but the complexity involved isn't warranted

The wrap condition doesn't require any special case code just do it the way I said in my last post. I.E. calculate elapsed time.

Mike.

 

[Jeff Wahaus]

Consider if timer_cmp=4294967295 and millis is one less. If your routine is called 2mS later then millis() will be zero and timer_cmp=4294967295 - no flashing for 49 days.

Yes, this can be an issue. Not very likely but if it's possible then it will eventually happen.

The wrap condition doesn't require any special case code just do it the way I said in my last post. I.E. calculate elapsed time.

You've defined "lastChange" as a long and millis() returns an unsigned long. As soon as millis() exceeds MAX_LONG your variable will either go negative or have undefined behavior. So, assuming you fix this it will leave your code with only 2 issues: 1) random first toggle time, 2)LED may not Light as soon as RPM<200 condition is met.

I will concede that is it safer calculate the toggle time using: (millis() - lastRead) > rate
Cheers, Jeff

 

[Pommie]

Whoops, typo, normally I use uint32_t and changed to long for beginner clarity. Oh well.

Mike.
Edit, BTW, how are there issues with my code when it does exactly what was requested? You seem to have added additional conditions.

 

[Jeff Wahaus]

Whoops, typo, normally I use uint32_t and changed to long for beginner clarity. Oh well.

Mike.
Edit, BTW, how are there issues with my code when it does exactly what was requested? You seem to have added additional conditions.

Think about the application here. The LED is being used to indicate low RPM warning light. At a flash rate of 500ms this is a pretty long time to wait for your warning LED to light.
The specification was somewhat vague but I think common sense would indicate that you want the LED to light as soon as the RPM<200 condition is met and you want it to light initially for the same duration as all the other flashes.

Or did you intend for the LED to light at random between (0-500) milliseconds after the condition is met? Did you intend for the first flash to be of random duration between (0-500) ms?
I think both of these are a bigger problem then the condition of: If you keep the car running for 49 days continuously without turning it off you may experience an LED glitch.

 

[Pommie]

Think about the initial specification here and don't let feature creep set in. In my very first post in this thread I said that a 1Hz signal is not very good and suggested a faster rate.

What if the condition is met for 1mS, should the LED stay on for 1mS or 500mS? BTW, if you answer that you've just added another condition to the original requirement.

The LED is being used to indicate low RPM warning light.

You've added an assumption. Nowhere is this stated! Stop assuming. And stop adding features.

Mike.