Why does serial.read stop working?

So, I've been playing with the Simple State Machine Library, for the purpose of automating a home brewery. State machines seem like the way to go given the finite, sequential states for brewing beer. I've started small, with a sketch just focusing on the state machine itself, but I'm having a problem - serial reads stop working! I'm using serial.reads to fake a button press on the computer - eventually I want to run a program on the PC that talks to the arduino for uploading recipes, logging, and managing state transitions.

Here's my sketch thus far:

#include <SM.h> //Simple State Machine Library
//State Machine names go here MachineName(headstate,bodystate)
SM Hold0(H0h,H0b);
SM Strike(S1);
SM Hold1(H1b);
SM Mash(M1);
//SM Hold2(H2) ;
//SM Boil(B1) ;

/**** global variables *****/
double Setpoint;
//String str;

void setup(){
  Serial.begin(9600);
}

void loop(){
  EXEC(Hold0);
}

State H0h(){
Serial.println("hello");
}

State H0b(){
  while (Serial.available()>0) {
    String str = Serial.readStringUntil('\n'); 
    if(str == "next"){
      Serial.println("going to Strike");
      EXEC(Strike);
      Hold0.Finish();
    }
  }
}

State S1(){
  Serial.println("S1"); 
  Setpoint = 155;
  int temp = 150; //manual faking of temperature
  while (temp < Setpoint){
    temp++;
    Serial.print(Strike.Statetime());
    Serial.print(" setpoint= ");
    Serial.print(Setpoint);
    Serial.print(" temp= ");
    Serial.println(temp);
  } 
  Serial.println("going to Hold1");
  Strike.Finish();
  EXEC(Hold1);
}

/*State H1h(){
  Serial.println("H1");
  //Hold1.Set(H1b);
}*/

State H1b(){
  Serial.println("h1b");
  while (Serial.available()>0) {
    String foo = Serial.readStringUntil('\n'); 
    if(foo == "next"){
      Serial.println("going to Mash");
      EXEC(Mash);
      Hold1.Finish();
    }
    else if(foo !="next"){
      Serial.println("HUH?");
    }
  }
}

State M1(){
  Serial.println("Mash");
}

The problem is that the sketch gets stuck at the entry to Hold1 - it won't accept any more input. I'm looking for "next" to come across the serial, but when I enter that in serial monitor, I get nothing. Here's the output that I do get:

hello (here I enter "next")
going to Strike
S1
0 setpoint= 155.00 temp= 151
1 setpoint= 155.00 temp= 152
16 setpoint= 155.00 temp= 153
49 setpoint= 155.00 temp= 154
81 setpoint= 155.00 temp= 155
going to Hold1 (here I enter "next" but nothing happens)

Any help would be appreciated. I'm new to the idea of state machines, but it seems like everything is flowing properly.... until it doesn't :frowning:

thanks,
JR

That seems to be a gratuitously complicated method of implementing a "state machine".

Perhaps your problem is related to using Strings.

The other part of the code that looks dodgy, is where you attempt to read the string from the serial. What do you expect the readStringUntil() function to do, exactly ? Is the other end of your serial communication sending the newline character that you expect ? You might have fallen into some version of the CR vs CR-LF problem.

michinyon:
That seems to be a gratuitously complicated method of implementing a "state machine".

Perhaps your problem is related to using Strings.

The other part of the code that looks dodgy, is where you attempt to read the string from the serial. What do you expect the readStringUntil() function to do, exactly ? Is the other end of your serial communication sending the newline character that you expect ? You might have fallen into some version of the CR vs CR-LF problem.

Thanks for the reply Michinyon, the readstringUntil('\n') works fine in the upper section, so it shouldn't be that. I'm just using that to simulate sending some info through serial by using serial monitor.

What I did find - after another glass of wine and some more debugging - is that the serial is still working, and the while loop isn't being skipped, it just goes by too fast. I added a delay(4000) to the code and was able to get to the next state, but why does it jump like that, when it doesn't in the Hold0 state?

The idea is that the Hold states should just sit there until I enter a command, then go to the next state. This is to give me time to add/remove grains, etc.

Okay! the answer came to me after typing my response below, but brings up a new question

What's happening is the While loop goes by too fast, so delay paused it, if I put it inside another while loop, I can make it stick around

#include <SM.h> //Simple State Machine Library
//State Machine names go here MachineName(headstate,bodystate)
SM Hold0(H0h,H0b);
SM Strike(S1);
SM Hold1(H1b);
SM Mash(M1);
//SM Hold2(H2) ;
//SM Boil(B1) ;

/**** global variables *****/
double Setpoint;
//String str;

void setup(){
  Serial.begin(9600);
}

void loop(){
  EXEC(Hold0);
}

State H0h(){
Serial.println("hello");
}

State H0b(){
  while (Serial.available()>0) {
    String str = Serial.readStringUntil('\n'); 
    if(str == "next"){
      Serial.println("going to Strike");
      EXEC(Strike);
      Hold0.Finish();
    }
  }
}

State S1(){
  Serial.println("S1"); 
  Setpoint = 155;
  int temp = 150; //manual faking of temperature
  while (temp < Setpoint){
    temp++;
    Serial.print(Strike.Statetime());
    Serial.print(" setpoint= ");
    Serial.print(Setpoint);
    Serial.print(" temp= ");
    Serial.println(temp);
  } 
  Serial.println("going to Hold1");
  EXEC(Hold1);
  Strike.Finish();
  
}

/*State H1h(){
  Serial.println("H1");
  //Hold1.Set(H1b);
}*/

State H1b(){
  while ((Hold1.Statetime() - 5000000)){
  while (Serial.available()>0) {
    String foo = Serial.readStringUntil('\n'); 
    if(foo == "foo"){
      Serial.println("going to Mash");
      EXEC(Mash);
      Hold1.Finish();
    }
    else if(foo !="foo"){
      Serial.println("HUH?");
    }
  }
  }
Serial.println("h1b");
}

State M1(){
  Serial.println("Mash");
}

But now it sticks around so long that if I type "foo" after the Hold1 state should have finished, I still get the output "going to Mash".
So, now I'm not quite understanding what "State.Finish()" really means. Seems like Finish() should make the state stop.

Anyway, michinyon if there is some less complicated way to do this, I'm all ears. I've looked at Switch(case), but that gets more complicated in a hurry when trying to handle timers, sensor inputs, and serial inputs all at once. I was hoping SM would be the more elegant solution, but maybe not ?

You can use the concept of a State machine without using the Statemachine library. Not using the library might give you more control or make the project easier to follow.

I wrote a demo program in this Thread that illustrates how different events can be managed using millis() and variables to keep track of the states of the system.

...R

void loop(){
  EXEC(Hold0);
}

First, all upper case names are reserved, by convention, for constants that are #defined into existence. That is certainly not the case here.

Second, the name tells us NOTHING about what the function does.

Third, it is pointless to have a function that does nothing but call another function. One of them is redundant. It isn't loop().

Fourth, the indenting in the rest of your code sucks. Use Tools + Auto Format BEFORE posting any more code.

PaulS:

void loop(){

EXEC(Hold0);
}



First, all upper case names are reserved, by convention, for constants that are #defined into existence. That is certainly not the case here.

Second, the name tells us NOTHING about what the function does.

Third, it is pointless to have a function that does nothing but call another function. One of them is redundant. It isn't loop().

Fourth, the indenting in the rest of your code sucks. Use Tools + Auto Format BEFORE posting any more code.

Why not throw in "and your mother wears combat boots!" while you are at it, PaulS?

At any rate, I'm merely following the examples set up in the SM library . My apologies that the library's use of EXEC and single functions within loop() offend you. I made the assumption that since a more experienced programmer made the library, I should just follow his/her lead. Merely pointing out defects in my usage without providing possible solutions is not particularly helpful though.

Regarding my indentations, I did an autoformat and found all of 4 diffs between the two files.
Here's the autoformatted output:

#include <SM.h> //Simple State Machine Library
//State Machine names go here MachineName(headstate,bodystate)
SM Hold0(H0h,H0b);
SM Strike(S1);
SM Hold1(H1b);
SM Mash(M1);
//SM Hold2(H2) ;
//SM Boil(B1) ;

/**** global variables *****/
double Setpoint;
//String str;

void setup(){
  Serial.begin(9600);
}

void loop(){
  EXEC(Hold0);
}

State H0h(){
  Serial.println("hello");
}

State H0b(){
  while (Serial.available()>0) {
    String str = Serial.readStringUntil('\n'); 
    if(str == "next"){
      Serial.println("going to Strike");
      EXEC(Strike);
      Hold0.Finish();
    }
  }
}

State S1(){
  Serial.println("S1"); 
  Setpoint = 155;
  int temp = 150; //manual faking of temperature
  while (temp < Setpoint){
    temp++;
    Serial.print(Strike.Statetime());
    Serial.print(" setpoint= ");
    Serial.print(Setpoint);
    Serial.print(" temp= ");
    Serial.println(temp);
  } 
  Serial.println("going to Hold1");
  EXEC(Hold1);
  Strike.Finish();

}

/*State H1h(){
 Serial.println("H1");
 //Hold1.Set(H1b);
 }*/

State H1b(){
  while ((Hold1.Statetime() - 5000000)){
    while (Serial.available()>0) {
      String foo = Serial.readStringUntil('\n'); 
      if(foo == "foo"){
        Serial.println("going to Mash");
        EXEC(Mash);
        Hold1.Finish();
      }
      else if(foo !="foo"){
        Serial.println("HUH?");
      }
    }
  }
  Serial.println("h1b");
}

State M1(){
  Serial.println("Mash");
}

You are correct that my functions have very little description to them -
Hold just means that the code shouldn't do anything until I send an input. This is to give time for adding ingredients or removing ingredients. Strike is the initial heating of the water.
Mash is the conversion of starches to sugars within the grains.
They are different states because the inputs/transitions for each state is different.
Hold just waits for a user's input, in this case the string "next" to come over the serial pipe.
Strike is just looking for temperature to hit its target.
Mash (isn't yet, but will soon) look for temperature to be near target and wait for some period of time.

Robin2,
The reason I chose the SM library is it seemed like an easy way to implement the state machine with some extra timing functions thrown in. Being able to have multiple "machines" running with separate Timeout() functions seems like it would be great if I want to scale up to a 3 pot system (currently, this is for a single pot, Brew-In-A-Bag). I will dig into your thread a bit more though, seems like you understand state machines quite well. I did try switch(case), but found that it doesn't like the switch being based on long strings coming thorough serial. Seems Switch can deal with bytes and integers. If statements seem to produce a lot of code, so I was trying to avoid them, particularly since there may be a variable number of states (Mash steps could be between 1 and 4, usually 1 or 2 steps).

Thanks,
JR

At any rate, I'm merely following the examples set up in the SM library .

At least PaulS got you to provide a link to the SM library you're using. It's you who have to provide that information. You should not waste our time by expecting all of us to search for it on Google just to find out that you may have used a different version.

but found that it doesn't like the switch being based on long strings coming thorough serial. Seems Switch can deal with bytes and integers.

SWITCH can deal with anything you throw at it. It is just another form of IF. Getting long strings through the serial connection is a little more complicated. If the strings are just for the purpose of choosing between CASE options then it makes sense to keep them short as - single characters if possible. Remember that "pleaseLetTheElephantsHaveWater" doesn't convey any more information to a computer than "E" or "W".

...R

pylon:

At any rate, I'm merely following the examples set up in the SM library .

At least PaulS got you to provide a link to the SM library you're using. It's you who have to provide that information. You should not waste our time by expecting all of us to search for it on Google just to find out that you may have used a different version.

Then he should have said "hey dummy, I don't know about that library and you didn't provide a link for me to learn more".
And I would've said "D'oh! You're right!".

Instead, he complained about my formatting, which isn't helpful. In fact, it probably wasted everyone's time while I went off and checked my \t in the file. If the bar for getting help on this forum is that we have perfectly formatted code that complies to convention perfectly, then nobody will be able to get help on this forum and nobody who's on the forum will actually need the help.

Basically, don't be Nick Burns, the company computer guy:

Basically, don't be Nick Burns, the company computer guy:

Well, you started out with some "state machine" mumbo jumbo instead of stating specifically what you want the arduino to do in your project. Probably best to start back with the basics of what you want to do. That being said, there are members of the forum that in my opinion have personality issues that become apparent in the way they think they are being "helpful". Anyhow there have been previous brewing projects discussed in the past and studying those previous post may provide some useful information.

https://www.google.com/search?as_q=brew&as_epq=&as_oq=&as_eq=&as_nlo=&as_nhi=&lr=&cr=&as_qdr=all&as_sitesearch=http%3A%2F%2Fforum.arduino.cc%2Findex&as_occt=any&safe=images&tbs=&as_filetype=&as_rights=

Robin2:

but found that it doesn't like the switch being based on long strings coming thorough serial. Seems Switch can deal with bytes and integers.

SWITCH can deal with anything you throw at it. It is just another form of IF. Getting long strings through the serial connection is a little more complicated. If the strings are just for the purpose of choosing between CASE options then it makes sense to keep them short as - single characters if possible. Remember that "pleaseLetTheElephantsHaveWater" doesn't convey any more information to a computer than "E" or "W".

...R

OK, I gotcha. What I'm not certain about is how I want to send signals regarding multiple Mash states. Do I want to have a single mash state that I want to call repeatedly, with different values for Setpoint and Time? Should I create multiple mash states that each get called sequentially? Or perhaps multiple mash states that can be called (or not called) in any order. That changes how I'd want to send the signal to the program, in the first and second options I can just send "next", or more simply "n". If I want to call mash states out of sequence, then I would need to be able to say Mash1, Mash2, or at least M2. So, I've been defaulting to the longer string signal coming from serial.

Can I have multiple cases running at once in switch(case)? I don't need to have the convenience features of SM for timers, I can write a if milliseconds - targetMilliseconds(30000) for each case. Having multiple machines running at once could be very useful though.

Thanks!

Then he should have said "hey dummy, I don't know about that library and you didn't provide a link for me to learn more".
And I would've said "D'oh! You're right!".

I agree.

Instead, he complained about my formatting, which isn't helpful. In fact, it probably wasted everyone's time while I went off and checked my \t in the file. If the bar for getting help on this forum is that we have perfectly formatted code that complies to convention perfectly, then nobody will be able to get help on this forum and nobody who's on the forum will actually need the help.

Sometimes I see horribly formatted code which misleads the interpretation, then I also complain about the formatting. Although your code is not an example of how one should format code, it's readable doesn't mislead in my opinion, so I agree also that PaulS' complains were a bit "picky". Nevertheless, if you read the sticky post at the top of the forum you would have linked the library in your first post and Paul could have seen that it's more or less the library example. If he then would have written the same post, it would have been a complete overreaction.

Back to your problem to calm down.

You didn't understand the state machine concept (at least not the own of that library). EXEC() does run one loop of the state machine. In your loop your calling state machine Hold0 and in that body function you finish the state machine. From that moment EXEC(Hold0) will return immediately and you end in an endless loop, doing nothing.

pylon:

Then he should have said "hey dummy, I don't know about that library and you didn't provide a link for me to learn more".
And I would've said "D'oh! You're right!".

I agree.

Instead, he complained about my formatting, which isn't helpful. In fact, it probably wasted everyone's time while I went off and checked my \t in the file. If the bar for getting help on this forum is that we have perfectly formatted code that complies to convention perfectly, then nobody will be able to get help on this forum and nobody who's on the forum will actually need the help.

Sometimes I see horribly formatted code which misleads the interpretation, then I also complain about the formatting. Although your code is not an example of how one should format code, it's readable doesn't mislead in my opinion, so I agree also that PaulS' complains were a bit "picky". Nevertheless, if you read the sticky post at the top of the forum you would have linked the library in your first post and Paul could have seen that it's more or less the library example. If he then would have written the same post, it would have been a complete overreaction.

Back to your problem to calm down.

You didn't understand the state machine concept (at least not the own of that library). EXEC() does run one loop of the state machine. In your loop your calling state machine Hold0 and in that body function you finish the state machine. From that moment EXEC(Hold0) will return immediately and you end in an endless loop, doing nothing.

Thanks, now that I'm not feeling quite so cranky ....
I understand from your post that my state machine Hold0 is in a loop in loop() so serial.available() is able to keep being .... available... Probably what I need to do then is move EXEC(Hold0) out of the loop() and into setup(), but give it its own loop to read characters. What is the best way to do that? giving it a loop to allow reading multiple characters from serial?

zoomkat:

Basically, don't be Nick Burns, the company computer guy:

Well, you started out with some "state machine" mumbo jumbo instead of stating specifically what you want the arduino to do in your project. Probably best to start back with the basics of what you want to do. That being said, there are members of the forum that in my opinion have personality issues that become apparent in the way they think they are being "helpful". Anyhow there have been previous brewing projects discussed in the past and studying those previous post may provide some useful information.

brew site:http://forum.arduino.cc/index - Google Search

Zoomkat, I posted another thread in the Project Guidance forum (which you posted on B)
http://forum.arduino.cc/index.php?topic=229539.msg1657014#msg1657014
I didn't want to double post though, since I thought this was a relatively small issue. Now that I see what the problem was (not that serial just wasn't reading, but that the while loop went by impossibly fast), I see that it's a larger issue with using State Machines and my implementation particularly.

I've looked at the code for BrewTroller and HABS, but both are using if statements. So.Many. Ifs. I'm trying to avoid that, and I'm also trying to create a program that allows for multiple mashes to create a mash profile that isn't a single mash time/mash temp.

Thanks,
JR

I understand from your post that my state machine Hold0 is in a loop in loop() so serial.available() is able to keep being .... available... Probably what I need to do then is move EXEC(Hold0) out of the loop() and into setup(), but give it its own loop to read characters. What is the best way to do that? giving it a loop to allow reading multiple characters from serial?

No, you misunderstood me. The EXEC(Hold0) is not that wrong in the loop() but having only that state machine being called in the loop you must never call it's Finish() method. EXEC() is not being called once and then the state machine runs "itself" but it's designed to be called repeatedly, it's the working horse of the state machine. I got the impression from your code that you thought that you call EXEC(Strike) and then that state machine is running in a kind of "background" task/process, so you can end the Hold0 state machine. This is not the case. An Arduino doesn't have an operating system that is taking care of processes and background tasks. It does run through our program, the only exception are the interrupts but that's more a hardware thing.
Each of your state machines have only one state and that's where I think is your conceptional error.

Maybe you should describe what your whole program is expected to do.

jrubins:
Having multiple machines running at once could be very useful though.

It's just more work for the small Arduino processor to do.

If you are eventually going to send the Mash codes from a PC program you can have the human-readable stuff on the PC but just send short codes to the Arduino. However if using codes with 3, 4 or 6 characters makes the Arduino code easier to understand I would happily use them. A key factor is that the codes all be the same length to make reception by the Arduino easier.

...R

pylon:

I understand from your post that my state machine Hold0 is in a loop in loop() so serial.available() is able to keep being .... available... Probably what I need to do then is move EXEC(Hold0) out of the loop() and into setup(), but give it its own loop to read characters. What is the best way to do that? giving it a loop to allow reading multiple characters from serial?

No, you misunderstood me. The EXEC(Hold0) is not that wrong in the loop() but having only that state machine being called in the loop you must never call it's Finish() method. EXEC() is not being called once and then the state machine runs "itself" but it's designed to be called repeatedly, it's the working horse of the state machine. I got the impression from your code that you thought that you call EXEC(Strike) and then that state machine is running in a kind of "background" task/process, so you can end the Hold0 state machine. This is not the case. An Arduino doesn't have an operating system that is taking care of processes and background tasks. It does run through our program, the only exception are the interrupts but that's more a hardware thing.
Each of your state machines have only one state and that's where I think is your conceptional error.

Maybe you should describe what your whole program is expected to do.

Pylon,
If I don't finish the SM Hold0, then I can never use the serial string "next" anywhere else. That's not 100% horrible, but does limit my ability to change how I go through my states (see above response to Zoomkat) .

I was hoping to avoid being a double poster, but I can see that you need a bit more context.
I'm building a home brewing system, this system will be unique because of 4 key features:

  • Propane heated, using a servo to control propane flow (as opposed to electric or on/off control of gas)
  • Brew In A Bag, not 3 vessel
  • Support multiple mash steps
  • Read data from BeerXML files

The first one is unique in the world of home brewing as far as I can tell. Others have taken the idea and run with it - my friend tob77 made a version in Python for RasPi, http://www.homebrewtalk.com/f235/raspberry-pi-brew-controller-451059/
But I'm a bit unsure if Arduino can support the sheer number of 'if' statements that he's using to get his states.

If you don't know about beer brewing (and BIAB particularly) what you need to know is that beer is made by soaking grain in hot water of around 150F for some period of time. This has the effect of converting the starches in the malted grain into sugars and extracting those sugars from the grain. The enzyme activity that does this conversion is controlled by the temperature of the water. Too cold, no enzyme activity, too hot, you denature the enzymes. There is a sweet spot where you can convert the starches using alpha and beta amylase, but each of those works better at a specific temperature. Beta amylase works best at 130-150, and Alpha works best at 150-160.

So, to best control the profile of the beer, I want to control the mash profile - how many steps, what temperature, and for how long. That's what brings me to the state machine, because I want to have multiple mash states that aren't always going to be used. tob77's solution is to have a "beta rest" whether or not he uses that step. I don't want to have the step if not necessary.

Some steps in brewing will always be there, though. There is always a strike (initial water heating) step and there will always be a boil step.

I've got my arduino program working pretty well, except getting the recipe information out of beerXML and into some format that will change Setpoint and control time for each of the steps. Also, there need to be some alarms and steps that have nothing going on for time to add the grain to the strike water.

Attached is my full sketch for the automated BIAB, but it's missing the getRecipes() section, and that's what I want the state machine for. Of course, if it works well there, I could do the whole thing in state machines, then I don't need delay(), huzzah!

I hope that's clear. Let me know if not and I can clarify.

Thanks,
JR

automatedBIAB_v1.ino (7.73 KB)

It appears that you could reduce this to fewer states (or none). All the recipe needs is a list of temperatures and how long each needs to be maintained. Perhaps additionally a flag to tell the system whether it needs to signal you to take action before moving on to the next temperature. Perhaps a name for the operation it's carrying out.

No need for XML therefore - you can just send a comma separated string, copy it into a buffer and parse it as you need to get the next step.

wildbill:
It appears that you could reduce this to fewer states (or none). All the recipe needs is a list of temperatures and how long each needs to be maintained. Perhaps additionally a flag to tell the system whether it needs to signal you to take action before moving on to the next temperature. Perhaps a name for the operation it's carrying out.

No need for XML therefore - you can just send a comma separated string, copy it into a buffer and parse it as you need to get the next step.

I like where you're going WildBill. I tried this, but don't really know how to pull it off. The XML can be parsed in Python and sent over the wire as a list of CSVs. I was thinking that would go into 3 arrays - names, temps, and times. I suppose I could use a 4th for "needs user input". And if "needs user input[X] = 'yes' then that state can wait until the user input is satisfied.
If you take a look at my code, I was thinking in this direction, but as I said, I'm not sure how to pull it off. I may take another crack at it tonight. The array of "needs user input" may be the piece I was missing. 8)

Thanks,
JR

Personally, I wouldn't bother with arrays for the instructions. The brewing machine only cares about one stage at a time so I'd parse the next stage's items out of the serial input when I needed them. If you send the whole recipe at the beginning, you'd probably need to buffer it in a char array, but I'd still parse on demand. Alternatively, you could have the arduino ask the python program for the next instructions as it needs them and parse as they arrive.