2 small sketches using an identical function to read serial react differently...

Hi everyone,

I have two small sketches that I’m using to debug a bigger sketch and, while both use the same piece of code to read the serial stream from an external source, one is very stable and always give the good readings but the other one sometimes work and sometimes (when resetting or just plain right from the download), will give completely wrong readings…

Also, (very important I guess), the second more complex sketch keeps on resetting every 30 seconds or so…
I put a serial.println(millis()); to see when it happened and it shows that around 30000 (but could be 25 or 40000), millis() just returns a ridicously high number a couple of times and then bam! Reset!
This doesn’t happen in the first one…

Here they are both :

First (very small) one :

/********************************************************
 * PID Basic Example
 * Reading analog input 0 to control analog PWM output 3
 ********************************************************/

#include <PID_v1.h>
#include <Servo.h>

//Define Variables we'll be connecting to
double Setpoint, Input, Output;

Servo servo1;
Servo motor1;

//Specify the links and initial tuning parameters
PID myPID(&Input, &Output, &Setpoint,2,5,1, DIRECT);

int read_state = 1;   //for RS232 Serial communication with FADEC
int readindex = 0;    //for RS232 Serial communication with FADEC
byte trame[48];       //for RS232 Serial communication with FADEC
int i;                //for RS232 Serial communication with FADEC
int egt=0;            //Exhaust gas temperature reading from FADEC
int RPMN2;
boolean auth=1;
long tempo;
int temp;

void setup()
{

  //initialize the variables we're linked to
  Input = RPMN2;
  Setpoint = 50;

  Serial.begin(19200);
  Serial2.begin(2400);

  pinMode(34,OUTPUT);
  pinMode(35,OUTPUT);
  servo1.attach(42);
  servo1.write(0);
  motor1.attach(43);
  motor1.write(0);
  //turn the PID on
  myPID.SetMode(AUTOMATIC);
  //myPID.SetOutputLimits(64,240);    //setting limits to PID outputs
}

void loop()
{
  read_fadec();

  if(digitalRead(14)==HIGH && auth==1)
  {
    servo1.write(95);
    motor1.write(50);
    digitalWrite(34,LOW);
    digitalWrite(35,LOW);
     }
  else
  {
    motor1.write(5);
    servo1.write(0);
    digitalWrite(34,HIGH);
    digitalWrite(35,HIGH);
  }

}


void read_fadec () //extracting temperature and speed informations from FADEC RS232 frames
{
  byte buffer;

  if (Serial2.available()>0)
  {
    buffer = Serial2.read();

    switch (read_state) {
    case 1:
      if (buffer == 252) {
        read_state = 2;
        // debut de trame, on se prepare a recevoir
        readindex = 0;
        break;
      }

    case 2:
      if (buffer == 253) {
        read_state = 3;
      }
      else {
        // mauvaise synchro, on reset 
        read_state = 2;
      }

      break;

    case 3:
      trame[readindex] = buffer;

      if (readindex >= 47) {
        // on a eu le dernier byte
        //for(int i=0;i<47;i++)
        //{
        //Serial.print("vitesse : ");
        //Serial.print(trame[37]); 
        //Serial.println("000 RPM");
        RPMN2 = word (trame[47],trame[46]);
        Serial.print(RPMN2);
        Serial.println("00 RPM");
        Serial.print("Temperature : "); 
        Serial.println(trame[43]*4);
        Serial.println(Output);
        egt=(trame[43]*4);
        Serial.println(millis());
        //Serial.println(Output);
        //Serial.println();
        //}
        read_state = 1;
      }

      readindex ++;
      break;

    }
  }
}

The second and problematic one (a bit bigger) is attached since it doesn’t fit here.

As you understood, the serial function is called read_fadec.

Any obvious things my newbie self overlooked?

Thanks!

Marc

SequenceSimple2_74.ino (9.61 KB)

I have two small sketches

For some definition of small, I guess.

  byte buffer;

Pretty unusual definition of a buffer.

Why does read_fadec() only read one byte of serial data per call? It should read ALL available data, every time it is called, at least up to the end of packet marker.

Ok so I checked and it always restarts after 34000.

@PaulS :

The baud rate of the serial link is 2400bauds so pretty slow and it should give the sketch the time to do a whole round before even a byte is received, but maybe that's wrong... It does work pretty good on the other sketch though.

And the byte buffer, I figured the info received will always byte by byte so why bother? Once again, am I mistaken?

Thanks!

Marc

And the byte buffer, I figured the info received will always byte by byte so why bother? Once again, am I mistaken?

Yes and no. Data is read one byte at a time. It is read from a storage mechanism that allows for multiple bytes to be held at one time. That mechanism is referred to as a buffer. One character is not a buffer.

The baud rate of the serial link is 2400bauds

2400 bauds? What the heck is that?

and it should give the sketch the time to do a whole round before even a byte is received

Should and does are not the same.

Your link to the larger sketch doesn't work for me so I only have your "small" sketch to look at. Reverse engineering the code, it appears to be waiting for a signature of two bytes (252,253) and then downloads 46 bytes of data.

Within the logic, however, I notice that it it not concerned whether the 253 directly follows 252. Once it's found the 252 it will simply discard everything recieved until it finds 253. Is this the correct behaviour?

I also notice a piece of redundant code on line 92 you have:

  else {
        // mauvaise synchro, on reset 
        read_state = 2;
      }

readstate is ALREADY 2. it's the very condition that got us to that line in the switch statement. Should this have perhaps been resetting read_state to 1 so that it goes back to looking for the next incidence of 252?

@KenF,

It is actually not a link (don't know why I underlined that?), the sketch is attached to the first post.

I will take a look at this line but the piece of code does give good results in this sketch (but maybe bad programming makes it not robust enough and not working in the other one, true...).

@PaulS

I understand that the real serial buffer holds 64 bytes if I'm correct but a Serial.read() will only return one byte at a time, right? Since we're only comparing one byte at a time to check if it's the beginning / end marker and then putting each byte into an array, I think it's fine, right?

I'm definitely still learning but I thought I had this figured out.

Thanks for your help guys,

Marc

I think it's fine, right?

I don't know whether you think it's fine, or not. But, the name is not fine.

It's like using:

char strings;
float myInt;
int myFloat;

While the names are all perfectly valid, they are nonsense. Like your use of a name that implies plural byte storage, but that can actually hold only one character.

But, that's just my opinion.

@KenF

Okay so I looked at this error and yes it was supposed to put read_state to 1.
I corrected it and put it into the arduino and it now seems to be working perfectly! What an embarassment!!! :blush: :blush:

AND it doesn’t reset anymore, but actually, I’d like to understand this one, why was it resetting, did something was overlowing? I don’t understand…

@PaulS,

Ok I understand why it's confusing now, I'll name it another way then. :)

marc426: @PaulS,

Ok I understand why it's confusing now, I'll name it another way then. :)

Thank you. And, I suspect that you'll find the code easier to understand in 6 months if all the variables have meaningful names.

You might me interested in learning about enums. Having names for the numbers you are using might have made the mistake more obvious.

For instance,

    case 1:
      if (buffer == 252) {
        read_state = 2;

conveys some information. But,

    case 1:
      if (buffer == 252) {
        read_state = START_OF_PACKET_RECEIVED;

conveys some information, too.

The question is which snippet conveys more information? Does the same answer apply if you come back to the code after not looking at it for 6 months?

Something that may be causing the erratic behaviour in your larger sketch may be that you are running out of memory.

try adding this function

int get_free_memory()
{
 extern int __heap_start, *__brkval;
 int v;
 return (int) &v - (__brkval == 0 ? (int) &__heap_start : (int) __brkval);
}

Then at strategic points in your code you can add the following to find out how you're doing.

Serial.print("Available Memory ");
Serial.println(get_free_memory(),DEC);

Then at strategic points in your code you can add the following to find out how you’re doing.

Using even more memory. The F() macro needs to be used here:
Serial.print(F("Available Memory "));

Thank you Paul. That's a darned useful macro to know about.

Can you imagine the number of "PROGMEM" and "(char*) pgm_read_word(&(menuPointer[index]))" type statements I have in my code? ]:)

where a simple (f("system time is ")) achieves the job better and far more simply.

Thanks again.

          if (sequence_running==true && sequence_auth == true)

Personally I don't like comparing booleans to true/false because that is the only value they can have. Put it another way, isn't this easier to read?

          if (sequence_running && sequence_auth)

And:

    if(sequence_running==false)

Could be:

    if(!sequence_running)

Thanks Nick,

I actually changed every boolean to the simpler writing.

One question I have though :

Does it work with DI/DO, for instance : does :

if(digitalRead(14))

is equivalent to

if(digitalRead(14)==HIGH)

It does work, and probably always will. HIGH is defined as 1, LOW as 0. However, because those are arbitrary constants that could change at the whim of the Arduino team in a future release, experienced developers usually suggest that you should stick to:

if(digitalRead(14)==HIGH)

In practice, if you use the shortened form, you’ll be fine - just don’t tell anyone :wink:

Technically wildbill is correct. However it would be weird if the developers changed digitalRead to return something other than 0 for LOW (to say nothing of breaking a lot of existing code).

In C only zero is considered false (and everything else true), so even if HIGH was changed (eg., to 0xFF) it is still valid to use:

if(digitalRead(14))