How to get out of multi-level serial level data entry check?

How do I fix this up so that if command p/P does not get 5 digits, or it gets a bad digit, it dumps me out of the
first , overall, if()?
Right now, if I get a P command, I can get a report of a bad digit, or a bad position when a non 0-9 is entered, or when not all 5 digits are received, but then I can't seem to move along to another command without hitting 0 and Enter a couple of times, or I'm kind of left in limbo with recovery a question.

Thanks.

unsigned long maxPosition = 13350; // max extension, ~42 inch
unsigned long minPosition = 0; // min retraction
// motor "position" to move to
unsigned long goToPosition;
unsigned long motorPosition[]= {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,};
// track how many steps up or down have been moved
unsigned long motorNow[]     = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,};
byte goodDigit;


// Now - Position => 0, move motor down
// Now - Position <= 0, motor up
// Now - Position = 0, you are there.


byte x;
byte motorNumber = 0;
byte command = 0;
byte moveCommand;
byte goodCommand = 0;
byte goodMotor = 0;
byte upDown = 0;
byte motorToMove; // the motor commanded to move
byte motorCommand[16]; // the motor command received 1 (retract), 2 (unwind), or 3 (stop)
byte nextStep;
byte charStep[6];
byte stepNumber[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0}; // keep track of which step each motor is on

void setup() {
  // Turn on Serial
  Serial.begin(115200);
  Serial.println ("Ready to Go.");

}
void loop() {
  // comment out Serial when using 1284P, comment out Serial1 when using PC
  // test with Serial to start, comment out and uncomment Serial1 when transition
  // copy this section for Serial1
  if (Serial.available() > 1 ) { // read 2 bytes
    /* if u/U or d/D or s/S, # 0 to 15 and upDown becomes 1,2,3
        if p/P, #0 to 15, then  chars, turn into a # 0 to 99,999 (13,350 current limit with platform height)
    */
    command = Serial.read(); // u/U = move up, d/D = move down, s/S = stop moving,
    // p/P = go to position, t/T = go to top, w/W = where now, z/Z = set motorPosition value to 0
    motorToMove = Serial.read(); // read motor numberm 0 to 9, a to f/A to F
    if ((command >= 'a' || command <= 'z') || (command >= 'A' || command <= 'Z')  ) {
      Serial.print ("Command ");
      Serial.print ((char)command);
      goodCommand = 1;
    }
    else {
      goodCommand = 0;
      Serial.print (" bad command ");
      Serial.println ((char)command);
    }


    if (motorToMove >= '0' && motorToMove <= 'f') {
      motorToMove = motorToMove - 48; // ASCII to decimal for 0 to 9
      if (motorToMove > 9) {
        motorToMove = motorToMove - 7; // further ASCII  to decimal for A to F
      }
      if (motorToMove > 15) {
        motorToMove = motorToMove - 32; // further ASCI to decimal for a to f
      }
      Serial.print (" Motor # ");
      Serial.print (motorToMove); // should be 0 to 15 now
    }
    if (motorToMove >= 0 && motorToMove <= 15) {
      goodMotor = 1;
    }
    else {
      Serial.print (" bad motor #, ");
      Serial.println (motorToMove);
      goodMotor = 0;
    }


    if (goodCommand == 1 && goodMotor == 1) {

      if (command == 'p' || command == 'P') { // move to Position commanded
        Serial.print (" Move to ");
        moveCommand = 0xB; // need 5 characters still for 99,999
        // get 5 characters
        while (Serial.available() < 4) {
          // wait for more characters
        }
        // got 5, continue
        goodDigit = 1;
        for (x = 0; x < 5; x = x + 1) {
          charStep[x] = Serial.read();
          charStep[x] = charStep[x] - 48; // read order is EDCBA
          if (charStep[x] < 0 || charStep[x] > 9 ) {
            Serial.print (" bad digit ");
            goodDigit = 0;
            Serial.print (charStep[x], DEC);
          }
          if (goodDigit == 1) {
            goToPosition = (charStep[0] * 10000UL) + (charStep[1] * 1000UL) + (charStep[2] * 100UL) + (charStep[3] * 10UL) + charStep[4];


            if (goToPosition >= maxPosition) {
              goToPosition = maxPosition; // set max extension
            }
            if (goToPosition <= minPosition) {
              goToPosition = minPosition;
            }
            Serial.print (" position: ");
            Serial.println (goToPosition);
            motorPosition[motorToMove] = goToPosition; // set the commanded motor's position


            // motor x
            if (motorNow[motorToMove] > motorPosition[motorToMove]) { // move up
              motorCommand[motorToMove] = 1; // retract
            }
            else if (motorNow[motorToMove] < motorPosition[motorToMove]) { // move down
              motorCommand[motorToMove] = 2; // extend
            }
            else if (motorNow[motorToMove] == motorPosition[motorToMove]) {
              // same motorPosition, do nothing
              motorCommand[motorToMove] = 3; // stop
            }
          }
          else {
            Serial.println (" bad position ");
          }
        }
      }
    }
  }

}
if ((command >= 'a' || command <= 'z') || (command >= 'A' || command <= 'Z')  ) {

Shouldn't this be:

if ((command >= 'a' && command <= 'z') || (command >= 'A' && command <= 'Z')  ) {

?
And then you could:

command |= 1 <<5; // all lower case

When I saw your Title the thing that immediately jumped into my mind was "you should not be in a multi-level serial entry" and having looked briefly at your program I am reinforced in that view.

Receive the entire command before trying to parse any of it. Then you won't get stuck halfway up a prickly tree - or trying to saw through the branch you are sitting on.

...R
Serial Input Basics - simple reliable non-blocking ways to receive data.

@Robin2,
Has to be multilevel.
I have other commands that are just letter/number.
Those work fine.
If this particular one comes in, it is letter/number, then 5 more numbers.

@JCA34F,
Hmm, Yes, it looks like got carried away with ORs there. Will get that corrected and try it.
I think that came from testing for 4 letters with OR, when I added 5 & 6 and upper case, I switched from individual letters and went to a range, but not correctly.

Thanks.

CrossRoads:
@Robin2,
Has to be multilevel.

I don't agree at all.

Please provide examples of your commands.

I presume in the long run these commands will come from another "computer" (an Arduino or a PC) and if so you should now be designing a computer-to-computer command system rather than one that is suited to a human.

But my "disagreement" applies regardless of whether a human or a computer is sending the commands.

...R

can you take a step back and list all the possible commands ?

I would tend to agree with Robin, with possibly a parsing of the input buffer along the way every time a new character is detected if you don't have an end marker defined for your commands

the challenge if you don't have an endMarker being that if U1 et U10 are possible commands, you have no way to know if the user is done after receiving U1 or if user is slow typing and you have more to receive and might end up in an infinite wait unless you implement a timeout

The commands are simple, letter + number.
From the serial monitor, letters are of of: u U d D s S w W t T p P (command)
All are then followed with one of: 0 to 9 and a A b B c C d D e E f F (motor number)

p P also see 5 additional numbers: 0 to 9 (position to move to, 00000 to 99999).

I was reading about Serial.readByteUntil('\n', buffer, maxLength)
That seems like it could be handy, so I'd could change the line ending to New Line ( '\n' ??)
and if I got
'd' 'f' '\n' I could take one course, moving motor 15 down.
'p' 'f' '1' '2' '3' '4' '5' '\n' I could take a 2nd course, moving motor 15 to 12345
Or somehow recognize if I got
'p' 'f' '1' '2' '3' '\n' I could stop there, moving motor 15 to 123

but if I got
'p' 'f' '1' 'w' '3' '\n' I could stop there and recognize a misentered number (fat finger, or bad/corrupted data from upstream processing) and just toss the command out.

I've only ever worked with a start byte and then so many known digits to follow, this kind of interaction is new.

I plan to make the central 1284P send out characters so that I always have the option to connect up to any input and confirm what is coming in if I see weird stuff going on while testing. That's the next step once I get this 2560 code just a touch more robust on the incoming data side.

What I don't get is how to tell the code to take the 2 character, or the 3 or more character path.

if (Serial.available () >2){ // 2 bytes minimum  plus \n
  charArray[x] = Serial.read();
  if (charArray[x] != '\n' ){
  x = x +1;
  doneReading = 1;
  }
else {doneReading = 0;}
}

if (doneReading ==1 && x ==2){
doneReading = 0;
command = charArray[0];
motor = charArray[1];
  x = 0;
}

if (done Reading ==1 && x >2){ // 3 to 9 with \n at the end
 doneReading = 0;
 command = charArray[0];
  x=x-1; // up to 6 left
  motor = charArray[1];
  x=x-1; // up to 5 left
  digit0 = charArray[2];
  x=x-1; // up to 4 left, but could be none
  if (x>0){
  digit1 = charArray[3];
  digits = 2;
  x = x-1;
  }
  if (x>0){
  digit2 = charArray[4];
  digits = 3;
  x = x-1;
  }
  if (x>0){
  digit3 = charArray[5];
  digits = 4;
  x=x-1;
  }
  if (x>0){
  digit2 = charArray[6];
  digits = 5;
  }
  x = 0;
}

Am I just going about this the long way?

CrossRoads:
The commands are simple, letter + number.
From the serial monitor, letters are of of: u U d D s S w W t T p P (command)
All are then followed with one of: 0 to 9 and a A b B c C d D e E f F (motor number)

To my mind that is not the way to think about them, even if it is true.

Think of a complete command as a separate entity and always think of them like that. For example

U7a

And if the reason for the Uu is so that the lazy human can enter either then just simplify and stick to one case - personally I prefer upper case because some of lower case characters can be ambiguous without spectacles.

Then receive them as a complete command before trying to parse them.

...R

If you can change the requirements to enforce a end marker ‘\n’ or space or whatever char that is not part of the accepted vocabulary then you make your life much easier.

Of course a line feed is super convenient when typing from the serial monitor and if you just ignore the ‘\r’ when you get one in, then the sending program can use println()

To deal with upper or lower case what you can do is To use toupper() on the received char when adding it to the received buffer. Then you only need to compare with upper case

Robin2:

CrossRoads:
@Robin2,
Has to be multilevel.

I don't agree at all.

I agree with Robin's disagreement. Once your input reaches this level of complexity and your code starts getting "multilevel", it's time to write a proper parser. See LALR parser - Wikipedia . This thing only needs a parser with three or four states.

Got it figured out

void loop() {
  // comment out Serial when using 1284P, comment out Serial1 when using PC
  // test with Serial to start, comment out and uncomment Serial1 when transition
  // copy this section for Serial1


  /* if u/U or d/D or s/S or t/T or w/W # 0 to 15 and upDown becomes 1,2,3
    if p/P, #0 to 15, then  chars, turn into a # 0 to 99,999 (13,350 current limit with platform height)
  */


  if (Serial.available () > 0) { // string terminated by \n
    newByte = Serial.read();
    // Serial.println(newByte);
    if (newByte == '\n' ) {
      Serial.println ("");
      Serial.print ("Done Reading, # of chars = ");
      Serial.println (x);
      doneReading = 1;
      numChar = x;
      if (x > 0) {    // make sure there's something to print
        Serial.print ("Array = ");
        for (i = 0; i < x; i=i+1) { // last character is new line
          Serial.print(charArray[i]);
        }
        Serial.println("");
      }
      y = x;
      x = 0;
    }
    else {
      charArray[x] = newByte;
//      Serial.println(charArray[x]);
      x = x + 1;
    }
  }

After, I have an array with 2 byte, or an array with 3 or more bytes.
I parse those up for the aX commands, or the aXn to pXnnnnn commands.

Thinking some more about this I reckon it would be a lot more efficient use of the Mega's time if you send a single message that contains instructions for all 16 motors even if some of the motors don't need to move.

There would be no need for a motorID because the position of the command in the message would determine which motor it is intended for.

...R

CrossRoads:
The commands are simple, letter + number.
From the serial monitor, letters are of of: u U d D s S w W t T p P (command)
All are then followed with one of: 0 to 9 and a A b B c C d D e E f F (motor number)

p P also see 5 additional numbers: 0 to 9 (position to move to, 00000 to 99999).

I gave it a try

/*
  for https://forum.arduino.cc/index.php?topic=707708.msg4756495#msg4756495

  The commands are simple, letter + number.
  From the serial monitor, letters are of of: u U d D s S w W t T p P (command)
  All are then followed with one of: 0 to 9 and a A b B c C d D e E f F (motor number)

  p P also see 5 additional numbers: 0 to 9 (position to move to, 00000 to 99999).


*/

const char* commands  = "udswtpUDSWTP";
const char* params    = "0123456789abcdefABCDEF";

char currentCommand = '\0';
const byte maxParams = 5;
char currentParam[maxParams + 1]; // +1 to always have a well formed cString
byte currentParamIndex = 0;

enum t_readStatus : byte {NOTHING_TO_READ, GOT_END_MARKER, GOT_COMMAND, GOT_PARAM, NOT_WHATS_EXPECTED};
enum t_states : byte {WAITING_COMMAND, WAITING_PARAMS} currentState = WAITING_COMMAND;

// return true if an endMarker is received
bool isEndMarker(char c)
{
  return (isspace(c));
}

bool isCommand(char c)
{
  return (strchr(commands, c) != NULL);
}

bool isParam(char c)
{
  return (strchr(params, c) != NULL);
}

void skipEntry()
{
  Serial.read();
}

t_readStatus peekNextEntry(char& c)
{
  int nextEntry = Serial.peek();
  if (nextEntry == -1)     return NOTHING_TO_READ;

  c = (char) nextEntry;
  if (isEndMarker(c))   return GOT_END_MARKER;
  if (isCommand(c))     return GOT_COMMAND;
  if (isParam(c))       return GOT_PARAM;
  return NOT_WHATS_EXPECTED;
}

bool getOrder()
{
  bool commandFound = false;

  switch (currentState) {

    case WAITING_COMMAND:
      {
        t_readStatus peekStatus = peekNextEntry(currentCommand);
        if (peekStatus == GOT_COMMAND) {
          currentCommand = toupper(currentCommand);
          currentParam[0] = '\0';
          currentParamIndex = 0;
          currentState = WAITING_PARAMS;
        }
        if (peekStatus != NOTHING_TO_READ) skipEntry();
      }
      break;

    case WAITING_PARAMS:
      {
        char aParam;
        t_readStatus peekStatus = peekNextEntry(aParam);

        switch (peekStatus) {

          case NOTHING_TO_READ: break;

          case GOT_PARAM:
            if (currentParamIndex > maxParams - 1) { // buffer full
              Serial.print(F("**** ERROR: Command = [")); Serial.print(currentCommand);
              Serial.print(F("] Params too long = [")); Serial.print(currentParam); Serial.print(aParam); Serial.println(F("]"));
              currentState = WAITING_COMMAND;
            } else {
              currentParam[currentParamIndex++] = toupper(aParam);
              currentParam[currentParamIndex] = '\0';
            }
            skipEntry();
            break;

          case GOT_COMMAND:
          case GOT_END_MARKER:
            if (currentCommand != 'P') {
              if (currentParamIndex != 1) {
                Serial.print(F("**** ERROR: Command = [")); Serial.print(currentCommand);
                Serial.print(F("] with improper parameter [")); Serial.print(currentParam); Serial.println(F("]"));
              } else {
                commandFound = true;
              }
            } else { // command is 'P', double check we  have at least 1 digit and only digits
              commandFound = (currentParamIndex > 0); // at least one param
              for (byte i = 0; commandFound && (i < currentParamIndex); i++) commandFound = isDigit(currentParam[i]);
              if (!commandFound) {
                Serial.print(F("**** ERROR: Command = [")); Serial.print(currentCommand);
                Serial.print(F("] with unusable parameter [")); Serial.print(currentParam); Serial.println(F("]"));
              }
            }
            currentState = WAITING_COMMAND;
            break;

          case NOT_WHATS_EXPECTED: // error in input
            Serial.print(F("**** ERROR: Command = [")); Serial.print(currentCommand);
            Serial.print(F("] Param = [")); Serial.print(currentParam); Serial.print(aParam); Serial.println(F("]"));
            skipEntry();
            currentState = WAITING_COMMAND;
            break;
        }
      }
  } // switch current parser state
  return commandFound;
}

// executing command with its parameter. (cammand and params are all upper case)
void executeCommand()
{
  Serial.print(F("Executing Command = [")); Serial.print(currentCommand);
  Serial.print(F("] with Param = [")); Serial.print(currentParam); Serial.println(F("]"));
}

void setup() {
  Serial.begin(115200);
  Serial.println("Enter command");
}

void loop() {
  if (getOrder()) executeCommand();
}

the parser is a small stream based with 1 byte look ahead state machine that works because there is no overlap in commands symbols and parameter symbols ==>The end marker of a legit order can be either a 'space' (CR, LF, space, tab, ...) or the start of a new command.

The state machine first tries to read a Command symbol (and ignores everything that is not a Command) and then starts accumulating parameters.

Once something comes in that is not a legit parameter (ie hex digit in this case) — it could be a new command or an end marker — or if we have exceeded the max size of the param buffer, then that signals it's time to double checks the coherence of the command:

  • if command is 'P' then you need just digits and not too many
  • if command is not 'P' then you need just 1 legit hexadecimal character

==> That means you can send "u3\n" or have multiple orders in one go such as "u3u4u5p2222\n" and the parser will "do" the right thing

give it a try!

J-M-L:
I gave it a try

From a very brief glance it seems a great deal more complicated than simply receiving the whole message and then checking what each part of it contains.

...R

Robin2:
it seems a great deal more complicated than simply receiving the whole message and then checking what each part of it contains.

This is very true!

What I wanted to try was to allow for just 1 byte look ahead and be in a position to execute the commands as soon as possible without reading a full line till a well defined end marker arrives

I agree it has limited value :slight_smile: (but you would still need to do the tokenization and parsing of the full sentence received once you have your buffer, which would boil down to something relatively similar to what I had to write. So your code would end up somewhat as complicated as well if you want to allow multiple commands in a row without end markers in between)

Thinking some more about this I reckon it would be a lot more efficient use of the Mega's time if you send a single message that contains instructions for all 16 motors even if some of the motors don't need to move.

There would be no need for a motorID because the position of the command in the message would determine which motor it is intended for.

I might get to something like that eventually. For now I am working on getting one Mega working with some simple commands so that I can proceed to check six sets of hardware and make sure all are wired and working correctly.
As part of testing so far, I discovered that some motors turn opposite of others, leading me to re-pin 10 connectors last night.

Once I can control motors individually correctly, then I can proceed to commanding them in groups. Might decide on a fixed format for messages for that, 16 groups of 5 characters, let the Mega command the motors to their various positions.

I think for now I have enough built in enough so that I command up or down as needed to determine positions (to capture a car shape for example), tweak each ball up or down a little as needed, then report back where those positions are. And keep track of the positions so that I don't accidentally retract too far and wrap the ball up into the motor, or retract too far and wind up in the wrong direction.
Top retracts until Stop is commanded, then Zeroize sets the 0/home point.
Up & Down move to tweak a position, with Stop, and then Where to tell where it is at.
Finally, Position will move to that spot from where ever it is, that way I don't have to re-home for every pattern.
Instead, I can go from shape to shape.
Or from shape to some moving wavy thing, then to the next shape. Haven't put much thought into a wavy thing yet.

It's a work in progress ...

still working on the BMW Kinetic Sculpture?

Yes. More on that at Creating The BMW Kinetic Sculpture - Project Guidance - Arduino Forum
Software questions work out better over here tho.
9 tabs of code in the IDE. Well, 7 really, first is notes on changes made, last is just a ; to close out loop.
Rest are more logical -

  • presetup declarations and setup()
  • start of loop with serial command receiving and blink without delay time tracking
  • parsing up 2 character commands and checking validity
  • parsing up 7 character commands and checking validity
  • executing 2 character commands with position tracking if BWD indicates it's time
  • executing 7 character commands with position tracking if BWD indicates it time (actually spread over 2 tabs)
  • end of loop, just a ;

I've got two sections that do the motor stepping that are almost identical. One moves for the 2 character commands, the other moves to a position. Both execute the same step sequence, just with different control checking before doing it.
Seems like I ought to be able to have both call this sequence, vs duplicating it.
I just haven't got there yet.

// motor 15
switch (motorCommand[15]) {
case 2:
  //if (upDown == 1) { // Extend (unwind) looking at shaft
  switch (stepNumber[15]) {         // 0110, 1100, 1001, 0011
    case 0:
      digitalWriteFast (30, 0); // 0110
      digitalWriteFast (32, 1);
      digitalWriteFast (34, 1);
      digitalWriteFast (36, 0);
      stepNumber[15] = 1;
      break;
    case 1:
      digitalWriteFast (30, 1); // 1100
      digitalWriteFast (32, 1);
      digitalWriteFast (34, 0);
      digitalWriteFast (36, 0);
      stepNumber[15] = 2;
      break;
    case 2:
      digitalWriteFast (30, 1); // 1001
      digitalWriteFast (32, 0);
      digitalWriteFast (34, 0);
      digitalWriteFast (36, 1);
      stepNumber[15] = 3;
      break;
    case 3:
      digitalWriteFast (30, 0); // 0011
      digitalWriteFast (32, 0);
      digitalWriteFast (34, 1);
      digitalWriteFast (36, 1);
      stepNumber[15] = 0;
      break;
  }
  break; // upDown1
case 1:
  // if (upDown == 2) { // Retract (wind up) looking at shaft
  switch (stepNumber[15]) {         // 0011, 1001, 1100, 0110
    case 0:
      digitalWriteFast (30, 0); // 0011
      digitalWriteFast (32, 0);
      digitalWriteFast (34, 1);
      digitalWriteFast (36, 1);
      stepNumber[15] = 1;
      break;
    case 1:
      digitalWriteFast (30, 1); // 1001
      digitalWriteFast (32, 0);
      digitalWriteFast (34, 0);
      digitalWriteFast (36, 1);
      stepNumber[15] = 2;
      break;
    case 2:
      digitalWriteFast (30, 1); // 1100
      digitalWriteFast (32, 1);
      digitalWriteFast (34, 0);
      digitalWriteFast (36, 0);
      stepNumber[15] = 3;
      break;
    case 3:
      digitalWriteFast (30, 0); // 0110
      digitalWriteFast (32, 1);
      digitalWriteFast (34, 1);
      digitalWriteFast (36, 0);
      stepNumber[15] = 0;
      break;
  }
  break; // upDown2
    case 3: // stop                 << this section is in different places for 2-char command vs 7-char command for example
      digitalWriteFast (30, 0);
      digitalWriteFast (32, 0);
      digitalWriteFast (34, 0);
      digitalWriteFast (36, 0);
      //          stepNumber[0] = 0;
      break; // stop
} // motor 15

And then there's various lines commented out from different things I was testing and later determined were not needed, but haven't fully cleaned out yet. It's getting better the more I work at it.

After I test out the 2/7 parsing today that I wrote up and incorporated into the motor moving code yesterday, then I'll do another attempt at separating out the common motor sequence above from the 2 sets of position tracking and direction control commands, and take some big chunks out of the code.

OK

Wouldn't the Arduino AccelStepper library library simplify your work? it Supports multiple simultaneous steppers, with independent concurrent stepping on each stepper. (I don't know how many though)

side note: your code would gain in readability if you were to use #define or enums for the magic values in the switch/case

enum t_motorCommand : uint8_t {mc_ retract=1, mc_unwind=2, mc_stop=3};

CrossRoads:
I might get to something like that eventually. For now I am working on getting one Mega working with some simple commands so that I can proceed to check six sets of hardware and make sure all are wired and working correctly.

I think if this was my project I would set up a second Mega with a simple program to send the commands.

...R