help with HandleRunningState

a continuation from a previous thread…
So I am trying to add/change the start triggers in my project (prior testing was just a button) now I am trying to incorporate 2 different methods of triggering my generator to start.
1- use a LDR to detect low light to start, and also maintain running until its bright enough, and the value rises above the threshold, then it will stop.
2- use a thermostat to determine a low level temp to start it up and maintain running for a defined length of time.

But when I tested, i realized I have an issue with the current setup, which currently only considers a start_trigger and runs a prescribed length of time.
When I attempt to continue the running (by low light level) it errors as it senses its already running.
The running timer I have would work fine for the temp option, but the running time for the LDR needs to be controlled by the LDR status instead.

The only thing I can think of is that I would need 2 separate HandleRunningState cases for each trigger condition?? (temp=timed, LDR=status)

But I’m sure someone has a better idea and simpler way to handle,
posting code separately

/*
  version6
  -fixed, not reacting to alternatorPin == HIGH to respond when its running
  - V7, adding voltage sense for alternator to determine RUNNING above 3 V across the voltage divider/optocoupler
*/
// system states
#define STOPPED   0
#define STARTING  1
#define PreCranking 2
#define CRANKING  3
#define PostCranking 4
#define RUNNING   5
#define ALARM     6
#define LOWLIGHT  7
#define LOWTEMP   8


#define Glow_Plug_On  10000 // 10 secs for testing
#define RUNNING_TIMEOUT 12000 // 12 secs for testing
#define NumStartAttempts 3 //sets the limit of attempts to try starting

const int ledPin = 13;          //red LED
const int starterPin = 12;      //green LED to represent turn on starter motor,
const int onPin = 11;           //yellow LED, represent ACC & IGN ON
//const int alternatorPin = 10;   //yellow button, used to simulate running status HIGH
int glowPlugPin = 4;            // Pin 4 to represent glow plug
int start_trigger = 2;          // trigger to start the process
int state = STOPPED;            // master variable for the state machine

// for starter functions
unsigned long StartedTime = 0; // initialize the time when the engine was started
unsigned long Running_Time = 0; // don't know what this is for and where its used, might not be needed
int StarterOnTime = 15000 ;      // the length of time we want to crank(turn on) the starter motor
int StarterOffTime = 1500;       // pause time between starts
int StarterTries = 0;           // initialize number of times the starter has been engaged
long int CrankStarterTime;
long int StarterOffNow;

// for alarm LED blinking:
int ledState = LOW;  // ledState used to set the LED
unsigned long previousMillis = 0;        // will store last time LED was updated
const long interval = 1000;           // interval at which to blink (milliseconds)

//for photocell
int LDR = 1;                  //analog pin to which LDR is connected, here we set it to 0 so it means A1
int LDRValue = 0;             //that's a variable to store LDR values
int light_sensitivity = 570;  //This is the approx value of light surrounding LDR

// for alternator voltage sense
int sensorValue;

void setup()
{
  pinMode(onPin, OUTPUT);             //pin 11 used for ign and acc
  pinMode(starterPin, OUTPUT);        //pin 12 used to turn on the starter motor
  pinMode(ledPin, OUTPUT);            //pin13
  //pinMode(alternatorPin, INPUT);      //pin 10 alternator for Running status when HIGH. using A0 input for battery voltage sensing instead.
  pinMode(glowPlugPin, OUTPUT) ;      //pin 4 used for glow plug
  pinMode(start_trigger, INPUT);      //pin 2 for start_trigger.In future to provide trigger from Photocell and/or Temp Sensor
  //pinMode(10, INPUT);                 //alternator for Running status when HIGH. using A0 input for battery voltage sensing instead.
  Stop();                   // Make sure everything is set correctly
  Serial.begin(9600);



}

void loop()
{

  switch (state) // when adding new states,add the "#define", then add the "HandlexxxState,,", then compile,and then add the "case"
  {
    case STOPPED:
      HandleStoppedState();
      break;
    case STARTING:
      HandleStartingState();
      break;
    case PreCranking:
      preCrankingState();
      break;
    case CRANKING:
      HandleCrankingState();
      break;
    case PostCranking:
      PostCrankingState();
      break;
    case RUNNING:
      HandleRunningState();
      break;
    case ALARM:
      HandleAlarmState();
      break;
    case LOWLIGHT:
      HandleLowLightState();
      break;
    case LOWTEMP:
      HandleLowTempState();
      break;

    default:
      Serial.print("Unknown State: ");
      Serial.println(state);
      Stop(); // Should never get here
      break;
  }

}

void Stop()
{
  digitalWrite(starterPin, LOW);
  digitalWrite(onPin, LOW);
  digitalWrite(glowPlugPin, LOW);
  state = STOPPED;

}
void HandleStoppedState()
{ LDRValue = analogRead(LDR);
  //Serial.println(LDRValue); // for troubleshooting to see the light value
  if ((LDRValue < light_sensitivity) || (digitalRead(start_trigger) == HIGH))
  {
    StartedTime = millis();
    digitalWrite(starterPin, LOW);
    digitalWrite(onPin, LOW);
    state = STARTING;

  }

}
void HandleStartingState()
{ sensorValue = analogRead(A0);
  // Convert the analog reading (which goes from 0 - 1023) to a voltage (0 - 5V):
  float voltage = (sensorValue * (5.0 / 1023.0) - 0); // analog voltage conversion
  if (voltage >= 2.5)
  { Serial.println(voltage);
    Serial.println("Abort cranking, engine is already running  ");
    StartedTime = millis();
    Stop(); // Don't crank the engine - it's already running
  }
  else
  {
    digitalWrite(onPin, HIGH);
    Serial.println("ignition & acc ON");
    delay (8000);
    digitalWrite(glowPlugPin, HIGH);
    Serial.println("Glow Plug ON");
    delay (Glow_Plug_On);
    {
      state = PreCranking;

    }
  }
}



void preCrankingState()
/*pre cranking state can keep track of number of attempts and note when this attempt started.
  What you need is indeed to set startCrankingTime to millis, but not in the cranking state.
  Set it in whatever sets the state to cranking. Then it stays fixed while cranking, time passes and you can check in cranking state whether you exceeded the timeout or started the engine.
  That's why I suggested adding a pre cranking state because you need to manage the number of starting attempts and set the startCrankingTime before you get into cranking state.
*/
{
  CrankStarterTime = millis ();

  if (StarterTries < NumStartAttempts)
  {
    StarterTries = StarterTries + 1;
    state = CRANKING;
  }

  else
  {
    Serial.print("Starting Attempts exceeded  ");
    Serial.println(StarterTries);
    digitalWrite(starterPin, LOW);
    digitalWrite(glowPlugPin, LOW);
    digitalWrite(onPin, LOW);
    state = ALARM;

  }



}

void HandleCrankingState()
/*Cranking state just cranks until the engine starts or millis tells you that you've exceeded the time allowed.
  If the engine started, you're running; otherwise you note the time and go to the post state.
  keep trying to start*/
{ // read the input on analog pin 0:
  sensorValue = analogRead(A0);
  // Convert the analog reading (which goes from 0 - 1023) to a voltage (0 - 5V):
  float voltage = (sensorValue * 5.0 / 1023.0); // analog volt conversion
  // print out the value you read:
  Serial.println(voltage);
  if (voltage <= 2.5)  // if battery voltage is below threshold, the alternator must not be running, so then start cranking
  {
    if (millis() - CrankStarterTime < StarterOnTime)
    { digitalWrite (starterPin, HIGH);
      Serial.println(" starting attempt #");
      Serial.println(StarterTries);

    }
    else                        // once starter on timer expires, then note the time and go to post cranking
    {
      StarterOffNow = millis ();
      state = PostCranking;
    }
  }

  else                      // the Running signal must be HIGH then, so now go to Running state
  {
    digitalWrite(starterPin, LOW);
    Serial.println("started now");
    StartedTime = millis();
    state = RUNNING;
  }

}


void PostCrankingState()
/*postCrankingfailure state tracks the time until you're ready to crank again and then puts you back in pre cranking.
   It's only purpose is to stop one cranking attempt from blending into the next.
   Just as you give the engine a rest between attempts when you do it manually.

*/
{
  if (millis() - StarterOffNow < StarterOffTime)
  { digitalWrite(starterPin, LOW);
  }
  else
  {
    state = PreCranking;
  }
}
void HandleRunningState()
{
  digitalWrite(glowPlugPin, LOW);
  if (millis() - StartedTime >= RUNNING_TIMEOUT) // when Running Timeout expires, then STOP.
  {
    Serial.println("Running Timeout");
    Stop();
  }
}
void HandleAlarmState()
{
  if (digitalRead(start_trigger) != HIGH)
  { Serial.println("In Alarm until reset");
    unsigned long currentMillis = millis();
    if (currentMillis - previousMillis >= interval)
    {
      // save the last time you blinked the LED
      previousMillis = currentMillis;
      // if the LED is off turn it on and vice-versa:
      if (ledState == LOW)
      {
        ledState = HIGH;
      }
      else {
        ledState = LOW;
      }
      // set the LED with the ledState of the variable:
      digitalWrite(ledPin, ledState);
    }

  }
  else
  {
    digitalWrite(ledPin, LOW);
    StarterTries = 0; //reset starter count
    state = STARTING;

  }
}

void HandleLowLightState()
{

}
void HandleLowTempState()
{

}

In the running state, what makes it transition to any other state? Your current code has only the timer. You are saying that if the light level is below some threshold, you ignore the timer and keep running?

void HandleRunningState()
{
  digitalWrite(glowPlugPin, LOW);
  getLightLevel();
  if(lightLevel < someThreshold) {
    //keep running
  } else {
    if (millis() - StartedTime >= RUNNING_TIMEOUT) // when Running Timeout expires, then STOP.
    {
      Serial.println("Running Timeout");
      Stop();
    }
  }
}

yes, correct. It currently is using the timer to determine when to stop.
now i realize I would need to have 2 conditions to determine when to stop,,
1-timer for the temp event or
2 -status for the photocell/LDR

How should the triggers interact? It sounds like light level should override a timed shutdown.

What happens if the generator was triggered by light level and it's now bright enough to stop, but during the run, the temperature has dropped enough that a cold trigger will be invoked?

In that scenario, is it ok to restart the generator immediately? Should the arduinio figure out that there's no point stopping? if so, should the timed run start from now, or the point where the temperature fell enough to trigger a start?

None of this is hard to code, just need to decide how it should work.

i should have clarified, the temp isn’t exterior, but rather an interior ambient or perhaps an engine temp (the temp sensor could be either attached to the engine or simply near it to monitor the temp of the engine compartment)

So, in the scenario of engine running already in the dark, then becomes light and temp drops, wouldn’t apply as it would already be warm. It would have to shut down first ( due to increased light level) and cool off to a threshold level to trigger a restart ( to warm up and prevent freeze up).

It would always start and keep running when it gets dark, until it gets light enough ( temp therefore would never trigger as it would be warm enough inside the compartment ).

But when its light out, then the cold monitoring would be applicable.

I would be inclined to add another global variable to keep a record of what trigger caused the generator to be started. It could be an enum if there will be many triggers, but if you're just concerned with light and temperature, a boolean would do.

Set that boolean, RunningForTemperature in the stopped state.

In running state, if you're RunningForTemperature, only the timeout can turn the generator off, else see how light it is and that'll tell you whether to stop.

wildbill:
I would be inclined to add another global variable to keep a record of what trigger caused the generator to be started. It could be an enum if there will be many triggers,

Don't do that.

This defines another state, layered upon the existing states. If you need more states then just make more states.

sorry,, I'm not clear on the distinctions between the two recommendations.
If my interpretation is on track, both appear to recommend 2 distinct states to deal with the different starting conditions (temp or light).
But I'm not clear on the finer points that you are both describing to appreciate the pro's/cons of each approach.

BTW,, thank you both for your input as you are both excellent sources of experience and information with very thorough ideas on how you would approach the solution.
Rob

If you start it because it’s cold then you need to remember that condition which existed before the start. That’s the definition of a state machine. It remembers what happened before and uses that information to decide what to do next.

Looking at your states again, you don’t really want to have different versions of pre-cranking and cranking for every possible start reason. So then it does make sense to store the start reason in a variable which isn’t the main state variable.

It makes for an interesting philosophical question about how to deal with extra state information which I haven't thought about before. All my state machines have been simple enough that the only thing I've been concerned with is which state am I in?

My solution with the global boolean will solve the problem in this case, but it feels dirty. Similarly, having to duplicate states and their handler code just to carry that "how did I start" information to end up in a different state doesn't seem right either.

The best I can come with so far is to suggest (as MorganS proposed) that what defines your state may have to comprise of more than one piece of data - "what state am I in" won't always suffice. Rather than slapping in extra globals, I think a better way would be to make the state variable a struct and pack everything that the state code needs to make decisions in there; in this case the original state integer (byte?) and a boolean to remember how the system was invoked. No new states required.

It still feels somewhat unsatisfactory. Apparently I need to do some reading about state machines.

RobG3987:
sorry,, I'm not clear on the distinctions between the two recommendations.
If my interpretation is on track, both appear to recommend 2 distinct states to deal with the different starting conditions (temp or light).
But I'm not clear on the finer points that you are both describing to appreciate the pro's/cons of each approach.

I'm suggesting no new states, but you will need to keep track of why you started the generator. That happens in the stopped state.

When the generator is in running state, what decides whether you should turn it off depends on why you started it.

Tweaking my original suggestion, you can see above that I now think that the "Why did I start" information should live in a struct with the original state variable, to make it clear that it's part of the state you have to remember.

MorganS:
If you start it because it’s cold then you need to remember that condition which existed before the start. That’s the definition of a state machine. It remembers what happened before and uses that information to decide what to do next.

Looking at your states again, you don’t really want to have different versions of pre-cranking and cranking for every possible start reason. So then it does make sense to store the start reason in a variable which isn’t the main state variable.

yes! i was hoping to find a way to avoid duplicating existing states and provide the flexibility to allow for determining how to keep running.
Thank you… Now I just have to figure out exactly how that is done, as I’m very new to this and lack the depth of knowledge and understanding on the structure required to make this idea come to fruition.

wildbill:
,,,,,, "Why did I start" information should live in a struct with the original state variable, to make it clear that it's part of the state you have to remember.

I think I'm getting the jist of what you two have been describing,,, but what is " a struct with the original state" ? Is "struct" an abbrev for something (structure,, instruction,,) or a different command ?

A struct is a C construct that lets you group data items together. q.v.

You don't need to use it here - it just seems cleaner to me.

You can just create a global variable to track why the generator started - the decisions you have to make using it will be the same whether it's grouped in a struct or not.

I would not use a struct. A global boolean would be fine. If you need to store more information then use more booleans or maybe an enum.

thanks both for your input. As I mentioned earlier I'm a novice to this so it tends to get over my head quickly and easily.
Are there any examples you could share with me that could show me what the functionality your proposing ?

What are ALL the reasons for starting?

For each of those, what are the reasons for stopping?

Write them down. See what is common, see what is different.

I'm not quite sure how this is to help me fill in the blanks.
Please remember I'm not on your level and would appreciate the explanations in more direct forms.
So, my reasons for starting are Low Light or Low Temperature.
Reasons for Stopping are High Light or High Temperature.
All the current states would work, except Stopped and Running.
What am I missing?
How do I get further details on solving this?

The reason to write all this down is to figure out precisely how you want the system to behave. When you know that, you'll be a lot closer to being able to code it up.

There are two triggers for running: low light and low temperature. Specifically, for each of them, what condition(s) should cause you to want to stop the generator?

Think about what you should do if both start conditions are true; what if both stop conditions are true?

You will need to know which condition started the generator so you know what to check to see whether you should stop (if they have different stop conditions, which I believe they do).