How to make this code function as efficient as possible

Using Arduino Programming Questions
1

The following functions receive and send serial data from the RS485 bus network. Packet Data is stored in a global byte array (one for send and another for received) for processing in other parts of the program.
It works as expected but I would like to make it as efficient as possible since it is being called all the time Any suggestions are appreciated :slight_smile:

boolean  CheckIncData()
{
  byte inByte1=0;
  byte inByte2=0;
  
  if (Serial1.available()<2) return false;
  
  inByte1 = Serial1.read(); 
  inByte2 = Serial1.read(); 
  if (((inByte1 == 0xAA)  and (inByte2 == 0xAA))== false)return false;  // First two bytes should be 0xAA
     
  IncPacket[0]=0xAA;
  
  while (IncPacket[0]==0xAA)  {                            // while loop rejects additional AA bytes. 
    while(Serial1.available()==0) { };     
    IncPacket[0]= Serial1.read();                         // Read Length of incoming packet

    }
  if  (IncPacket[0] > 78) {                                 // IncPacket[0] holds the length (number of bytes to be expected in packet. Next byte is the length and shouldn't be more than 78
  
     IncPacket[0]=0;
     return false;}
  
  for (int x=1; x<IncPacket[0]; x++)
    {
     while(Serial1.available()==0) { };
     IncPacket[x]=Serial1.read();
    };
    
   
  //  ****** Do Authentication CRC check *******
     
  bool passed=Check_crc(IncPacket, IncPacket[0]-2);                  // function to check CRC
      
  return passed;
  

   
}

And the following is the corresponding function to send a packet :

void SendFeedback() {

 

  if (Serial1.available()>0){                                                     // if someone else is talking postpone transmission 
   SendPending++;
   if (SendPending>10) {SendPending=0;Serial.println (F("Bus Busy. Packet  Dropped"));DroppedPackets++;return;}                                     // Try upto 10 times
   Serial.print (F("BUS Tx postponed:"));
   Serial.println (SendPending);
   if (SendPending>7){delay(10);}                                           // Add additional delay for waiting
   return;}



   
    digitalWrite (DEpin, HIGH);                  // Enable rs485 chip

    Serial1.write(0xAA);                           // send two start bytes
    Serial1.write(0xAA);  
    Serial1.write (OutPacket,OutPacket[0]);   // Send rest of packet
    Serial1.flush();  

    digitalWrite (DEpin, LOW);                       // disable chip
    
 


if (SendPending) {
 Serial.println (F("BUS Tx complete"));             // If had to wait for someone to stop talking 
 SendPending=0;                                         // now reset counter
}

else {TotalSent++;}                                     // This is for statistics on packets sent
}
2 
boolean  CheckIncData()
{
  byte inByte1=0;
  byte inByte2=0;

You're either going to return immediately, or overwrite those values; why bother initialising them?

3

You're either going to return immediately, or overwrite those values; why bother initialising them?

Thanks OWOL. Effectively, what you are suggesting is this instead :

if (Serial1.available()<2) return false;
  
  if (((Serial1.read() == 0xAA)  and (Serial1.read() == 0xAA))== false)return false;  // First two bytes should be 0xAA


...

Wouldn't this however, at the end result in the same amount of compiled code and use of memory ?

4

I think the code looks pretty good.

You COULD change the initial Serial.available check to:

if (Serial1.available) < 4) return false;

the minimal count where it can actually DO anything is "0xAA, 0xAA, Length, ", right?
When there IS data, you're going to be spending most of your time waiting for it to arrive on the serial port, so YOUR code doesn't matter very much.

I don't very much like:

if (((inByte1 == 0xAA)  and (inByte2 == 0xAA))== false)return false;

and would prefer:

if ( (inByte1 != 0xAA)  or (inByte2 != 0xAA)) return false;

But I think the compiler will produce basically the same code.

You might gain a bit of efficiency by calculating the CRC on the fly, which would overlap the CRC calculation with some of the time spent waiting for new data:

  for (int x=1; x<IncPacket[0]; x++)
    {
     while(Serial1.available()==0) { };
     IncPacket[x]=Serial1.read();
     crc = crc_update(crc, IncPacket[x]);
    };
    bool passed = crc_finalize(crc);

But unless your CRC function is substantially complicated, this would have a pretty small payback.
5

You COULD change the initial Serial.available check to:

But, OP's code does not always read exactly 4 bytes.

OP: I think it is pointless to talk about making blocking code as efficient as possible.

Efficient code does all it can with the data that is available, and then moves on to do something else. If it does all that needs to be done, great. If it takes many (thousands of) iterations of loop() for all the data to arrive, so be it.

6

OP: I think it is pointless to talk about making blocking code as efficient as possible.

Are you saying that this could also be done in non blocking way?

7

Are you saying that this could also be done in non blocking way?

Yes. There is, or is not, data in the serial buffer to read. If there is, read and deal with as much as is available, keeping track of how much you have done.

8

westfw:
would prefer:

if ( (inByte1 != 0xAA)  or (inByte2 != 0xAA)) return false;

it seems that there is a problem if the stream contains the first byte of a new message in the 2nd position, OP is annihilating the whole message... it may take a while to sync that out, it seems to me, though I didn't really look that deep on the logic.
why is a message (certainly a transient thing) a global? typically receiving and interpreting a message is an event (perhaps resulting in a state change or some action) so why keep it around?
instead of returning a boolean, why not just have the function return a pointer to the message or nullptr if no message? this would give good encapsulation and modularization, I would think.
something like this:

// look Mom, no globals!

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

void loop(void) 
{
  byte* newPacket = checkIncomingPacket(&Serial);  // change to Serial1 for Mega hardware Serial1
  if(newPacket)
  {
    Serial.println("Just got new packet: ");
    for (int i = 0; i < 4; i++)
    {
      Serial.print(newPacket[i], HEX);
      Serial.print(" ");
    }
    Serial.println();
  }
}

byte* checkIncomingPacket(HardwareSerial* stream)
{
  static byte incomingPacket[4];  
  if (stream->available() < 4) 
  {
    return nullptr;
  }
  byte inByte1 = stream->read();
  if(inByte1 != 0xAA)
  {
    return nullptr;
  }
  byte inByte2 = stream->peek();
  if(inByte2 != 0xAA)
  {
    (void)stream->read();
    return nullptr;
  }
  inByte2 = stream->read();
  incomingPacket[0] = inByte1;
  incomingPacket[1] = inByte2;
  incomingPacket[2] = stream->read(); // edited to correct index numbers!
  incomingPacket[3] = stream->read();
  if (Check_crc(incomingPacket, incomingPacket[0] - 2))
  {
    return incomingPacket;
  }
  return nullptr;
}

I think that may do it...

9

Watcher:
Are you saying that this could also be done in non blocking way?

Everything can always be done in a non-blocking way unless it has to spin in a loop doing
some heavy calculations. Just rewrite as a state-machine.

10 
byte inByte2 = stream->peek();

This i dont understand! :slight_smile:

Anyone care to explain?

11

Anyone care to explain?

Certainly. What don't you understand? There is a method, peek(). There is an instance of a class, stream. There is a dereference operator, ->. There is an assignment operator, =. Each of them are documented out the wazoo. Mr. Google is just waiting for you to ask.

12

You COULD change the initial Serial.available check to:

But, OP's code does not always read exactly 4 bytes.

No, but it will never return success unless there are at least 4 bytes...

OP: I think it is pointless to talk about making blocking code as efficient as possible.

That depends. You can certainly have a situation where "if there is a message arriving, then I need to receive it."
But non-blocking code would be preferable if you actually have other things to do - "do things unless a complete message has been received." See "state machine."

byte inByte2 = stream->peek();

This i dont understand!

Making the reference to "Serial" be indirect, so you can potentially read the message on Serial1, Serial2, etc...
Pointlessly complicated (and NOT efficient) if you're only going to be reading from Serial. (Unless compiler optimization saves you.)

why is a message (certainly a transient thing) a global? typically receiving and interpreting a message is an event (perhaps resulting in a state change or some action) so why keep it around?

Your "static incommingPacket[]" occupies just as much permanent storage as the global implementation, but is less "shareable." (encapsulation and modularity would say that having the buffer be shareable is a BAD thing, and there's some truth to that. But ... limited RAM...) Elimination of globals is overrated, on small microcontrollers, IMO.

13

westfw:
Making the reference to "Serial" be indirect, so you can potentially read the message on Serial1, Serial2, etc...
Pointlessly complicated (and NOT efficient) if you're only going to be reading from Serial. (Unless compiler optimization saves you.)

I understand your argument, but disagree with your "Pointlessly" comment. i prefer to know something about the program and its functions by reading it. For example:

CheckIncData()

tells me very little about what that function does. When you look at the function itself, you still don't get everything you need about that function... you still have to go back to see where the globals are defined. but no you are not done yet... you still have to find all the places those global variables are used and how/why... yuk!

on the other hand... when you look at something like this:

byte* newPacket = checkIncomingPacket(&Serial);

it tells anyone looking at the code what's happening. "I'm operating over Serial to return a pointer to a byte (array)." the function is a self contained thing that operates on something, does something and gives something back. it is just good practice; a function should do something to a thing. spaghetti code is still spaghetti code even on an MCU. some of the programs i've worked on have 3000 lines and that doesn't include the libraries. globals inherently create complexity

westfw:
Your "static incommingPacket[]" occupies just as much permanent storage as the global implementation, but is less "shareable." (encapsulation and modularity would say that having the buffer be shareable is a BAD thing, and there's some truth to that. But ... limited RAM...) Elimination of globals is overrated, on small microcontrollers, IMO.

i did not say that this was more data efficient. no matter what, you need space for the buffer. my point was encapsulation, the modularity/portability, ease of understanding and (hugely) debug.

There isn't enough emphasis towards a functional design on this forum, IMHO. over-reliance on globals today causes problems tomorrow.

but I appreciate the comments, it is nice to be read. :slight_smile:

14

Certainly. What don't you understand? There is a method, peek(). There is an instance of a class, stream. There is a dereference operator, ->. There is an assignment operator, =. Each of them are documented out the wazoo. Mr. Google is just waiting for you to ask.

Dear PaulS, Firstly thanks for your input and continuous effort and assistance on this forum in a number of questions that I asked over the years that I ve been a member of this forum - Karma points added. I cant help noticing however a certain tone of .. maybe arrogance in your comment above and having noticed similar tones in other occasions I thought I d point it out. You are right: I could google -> and find out what it is and indeed in most cases this is what I and most people here do. On the other hand however, I assume that some contributors might be willing to go the extra mile and even explain matters that are available on the Net in other forms especially in the context of discussion of a particular application. At the same time, not everybody here has the same background and experience on programming. Myself being an electrical engineer, although I ve had exposure in programming in various stages of my carrier, you can appreciate that programming and C might not exactly be my area of expertise. I do hope this comment is not taken the wrong way as its only intention was to make this forum an even more pleasant experience for everyone.

15

Watcher:
I cant help noticing however a certain tone of .. maybe arrogance...

What? PaulS? No....
:roll_eyes:

16

As I always say, it isn't arrogance if you actually are superior.

17

As I always say, it isn't arrogance if you actually are superior.

Who can argue with that ! :slight_smile:

18

Yes. There is, or is not, data in the serial buffer to read. If there is, read and deal with as much as is available, keeping track of how much you have done.

One thing I should point out which might not be clear from the code posted is that the whole packet needs to be received before CRC check can be carried out. CRC bytes are the two last bytes in the packet. Hence, there isn't anything to do with the packet other than just store it till it has been all received. And once received and successfully CRC verified, it can then be processed. If CRC fails, the packet is dropped.

19

One thing I should point out which might not be clear from the code posted is that the whole packet needs to be received before CRC check can be carried out.

Yes, but it does not have to be received on one pass through loop(). The loop() function could iterate 10,000 times between the first character arriving and the last one. You need to know how many, or which ones, to expect, so you know when you have gotten the last one, so you can do the CRC check.

20

PaulS:
Yes, but it does not have to be received on one pass through loop(). The loop() function could iterate 10,000 times between the first character arriving and the last one. You need to know how many, or which ones, to expect, so you know when you have gotten the last one, so you can do the CRC check.

Likewise, there is no reason to wait for a small packet to arrive in the buffer and then handle it.

Watcher:
...the whole packet needs to be received ...

I understand it is always led by two 0xAA bytes, is it always four bytes total?