Creating a class (or template) to write dynamically sized bytes to serial, and the bytes are incorrect. Wrong size

Projects Programming
1

I'm working on a project where I'm sending various size packets to some equipment.
I'm trying to create a class or template or something that will send the packets, make a note for debugging, and calculate a checksum.

This is all point to point, so there is no header, strangely.

How can I do this? Why is this reporting wrong packet information? See log below..

#include <Arduino.h>
#include <Vector.h>

const unsigned int interPacketDelay = 500;
#define debugMode 1

//debugging
#if debugMode == 1
  #define debug(x) Serial.print(x)
  #define debugln(x) Serial.println(x)
  #define debuglnHEX(x) Serial.println(x, HEX)
#else
  #define debug(x)
  #define debugln(x)
  #define debuglnHEX(x)
#endif

class writeSerial {
  public:
    writeSerial(Stream &p) : port(p) {
    }

    void writePacket(byte data[], String identifier) {
      unsigned int length = sizeof(data);
      debug("Sending "); debug(length); debug(" byte packet for: "); debugln(identifier);
      appendChecksum(data, identifier, length);
      delay(interPacketDelay );
      //port.write(data, length);
      debug("--Sent "); debugln(identifier);
    }

    void appendChecksum(byte data[], String identifier, unsigned int length) {
      data[length - 1] = calculateChecksum(data, length - 2);
      debug("..Calculated Checksum for "); debug(identifier); debug(": "); debuglnHEX(data[length - 1]);
    }

    byte calculateChecksum(byte data[], unsigned int length){
      byte chk = 0;
      for (int i=0; i<length; i++) {
          chk += data[i];
      }
      chk = chk % 256;
      return chk;
    }

  private:
    Stream &port;
};

writeSerial xs(Serial);

void setup() {
  Serial.begin(115200);

  byte test10[10] = {0x00,0xFF,0x00,0xFF,0x00,0xFF,0x00,0xFF,0x00,0x00};
  xs.writePacket(test10, "Should be 10 bytes long");

  byte test3[3] = {0x00,0xFF,0x00};
  xs.writePacket(test3, "Should be 3 bytes long");

  byte test1[1] = {0x00};
  xs.writePacket(test1, "Should be 1 byte long");
}

void loop() {
}

Results:

Sending 2 byte packet for: Should be 10 bytes long  
..Calculated Checksum for Should be 10 bytes long: 0
--Sent Should be 10 bytes long
Sending 2 byte packet for: Should be 3 bytes long  
..Calculated Checksum for Should be 3 bytes long: 0
--Sent Should be 3 bytes long
Sending 2 byte packet for: Should be 1 byte long
..Calculated Checksum for Should be 1 byte long: 0
--Sent Should be 1 byte long
2

data is a pointer and on e.g. AVR a pointer is two bytes. You will have to specify the number of bytes in the function.

void writePacket(byte data[], uint8_t numBytes, String identifier) {

and pass the number of bytes as an argument when calling the method.

I think that there is another way with templates but I'm not a CPP programmer (although trying to learn :wink:).

1 Like
3

the old c-style way would be to hand over the size of the array:

//#include <Arduino.h>
//#include <Vector.h>

const unsigned int interPacketDelay = 500;
#define debugMode 1

//debugging
#if debugMode == 1
  #define debug(x) Serial.print(x)
  #define debugln(x) Serial.println(x)
  #define debuglnHEX(x) Serial.println(x, HEX)
#else
  #define debug(x)
  #define debugln(x)
  #define debuglnHEX(x)
#endif

class writeSerial {
  public:
    writeSerial(Stream &p) : port(p) {
    }

    void writePacket(byte data[], size_t length, String identifier) {
      //unsigned int length = sizeof(data);
      debug("Sending "); debug(length); debug(" byte packet for: "); debugln(identifier);
      appendChecksum(data, identifier, length);
      delay(interPacketDelay );
      //port.write(data, length);
      debug("--Sent "); debugln(identifier);
    }

    void appendChecksum(byte data[], String identifier, unsigned int length) {
      data[length - 1] = calculateChecksum(data, length - 2);
      debug("..Calculated Checksum for "); debug(identifier); debug(": "); debuglnHEX(data[length - 1]);
    }

    byte calculateChecksum(byte data[], unsigned int length){
      byte chk = 0;
      for (int i=0; i<length; i++) {
          chk += data[i];
      }
      chk = chk % 256;
      return chk;
    }

  private:
    Stream &port;
};

writeSerial xs(Serial);

void setup() {
  Serial.begin(115200);

  byte test10[10] = {0x00,0xFF,0x00,0xFF,0x00,0xFF,0x00,0xFF,0x00,0x00};
  xs.writePacket(test10, sizeof(test10), "Should be 10 bytes long");

  byte test3[3] = {0x00,0xFF,0x00};
  xs.writePacket(test3, sizeof(test3),"Should be 3 bytes long");

  byte test1[1] = {0x00};
  xs.writePacket(test1, sizeof(test1), "Should be 1 byte long");
}

void loop() {
}
4

Thanks!!

data is a pointer and on e.g. AVR a pointer is two bytes. You will have to specify the number of bytes in the function.

Ohhhhh... I'm still getting that straight and I've read about it many times.

the old c-style way would be to hand over the size of the array:

Trouble is, I wont know how long until i build the packet. That's why I have vectors included on top. It's the next step to figure out, I think. I'd like to send hardcoded packets or dynamically created packets using the same function or class... There is going to be so many declarations of
byte sendbyte[5]. sendbyte[8]. I should figure out how to get rid/undeclare them after they are sent.

5

but in the moment you write the packet (call of writePacket) you know the size...

6

Yep Yep! and I dont have to worry about 100s of useless variables hanging around after awhile if I mess with this scope. This looks promising
https://en.cppreference.com/w/cpp/language/scope

This is the first programming project I've done in awhile that can't use a single class!! Doesnt like classes.

7

has nothing to do with not liking classes, you have the same challenge when doing a function call.

8

Keeping variables in their own scope solves alot of problems because I can reuse them in the loop.

  {
    byte test10[] = {0x00,0xFF,0x23,0xFF,0x00,0xFF,0x00,0xFF,0x00,0x00};
    writePacket(test10, "test10");
  }

  {
    byte test10[] = {0x00,0xFF,0x23,0xFF,0xFF,0xFF,0x00,0xFF,0x00,0x00};
    writePacket(test10, "test10");
  }

vs

    byte test10A[] = {0x00,0xFF,0x23,0xFF,0x00,0xFF,0x00,0xFF,0x00,0x00};
    writePacket(test10, "test10");
 
    byte test10B[] = {0x00,0xFF,0x23,0xFF,0xFF,0xFF,0x00,0xFF,0x00,0x00};
    writePacket(test10, "test10");

You'd run out of packets! Here I only have two to work with..

Question is... Will the memory get eaten up by worthless variables used once?

9

With the first version they are released when you exit the local block.
With the second one they will stick around until the end of the function, eating up more bytes on the stack.

If they are constant, store them in flash, if not reuse just one buffer

10

Do you mean this?

template <size_t n> writePacket(byte (&data)[n], String identifier) {
  // ...

Usage:

byte data[2];
writePacket(data, identifier);
2 Likes
11

That does indeed kind-of look like I what I had in mind. "Kind-of" because I would not have been able to invent that myself with my current knowledge.

Thanks a million, study time.

12

To minimize the number of functions and code size created from the template, try to minimize the number of buffer sizes.

Also having an old way function with pointer and size being called by the template will minimize the code

template <size_t n> size_t writePacket(byte (&data)[n], const char* identifier) {
    return writePacket(data, n, identifier);
}

size_t  writePacket(byte * data, size_t n, const char * id) {
  // do all the hard work here 


 return n; // or whatever count of what was actually sent
}

The compiler will likely in-line the call and fill in the size for you

1 Like
13

Yes!! Thank you that works nicely! Make me wonder if I can template a whole class for writing packets and managing the serial. Functions are working and it doesn't need a class, because I don't need multiple instantiations. What's best practice?

Kicking the can down the road... Or up the road as it goes. I can now write packets but building them dynamically is another matter..

I had to declare

//a checksum as appended to the end of each packet,
// so each packet is only 3, 4 or 5 "data" bytes.
byte sendPacket2DataByte[3] = {0x00,0x00,0x00};  //2 data bytes + 1 checksum byte
byte sendPacket3DataByte[4] = {0x00,0x00,0x00,0x00}; //3 data bytes + 1 checksum byte
byte sendPacket4DataByte[5] = {0x00,0x00,0x00,0x00,0x00}; //4 data bytes + 1 checksum byte

then

    byte sendData[] = {0x41,0x05};
    if (buildPacketFromData(sendData)) {;
      writePacket(sendPacket2DataByte, "NEW DATA");
    }

bool buildPacketFromData(byte (&data)[N]) {
    switch(N)
         case 2:
              //choose sendPacket2DataByte and fill it. //2 data bytes + 1 checksum byte
         case 3:
             //choose sendPacket3DataByte and fill it. //3 data bytes + 1 checksum byte
         case 4:
            //choose sendPacket4DataByte and fill it. //4 data bytes + 1 checksum byte
}

Is there a better way to do that? This works but it's clunk. What if I find a 12 byte packet for some motor operation.
I messed with Vectors for a minute but could not create a function that represents a vector, pass it data and assign that vector to an array.

14

this looks so wrong for me:
forum_numbers

You haven't provided enough context. But when I understand your real problem, you want "build" / add your packet from smaller pieces. So I would assume that you have a larger send buffer and just concatenate your smaller parts one by one... for example a simple memcpy to add the new data to the end of your buffer. keep track of the used size.

15

This will return the size of a char pointer, not the size of the array.

16

You don't seem to understand the comments and advice that was given earlier :wink: Either the solution with the additional parameter for the function or the use of the template.

inputData is a pointer !!

You will need to use the same approach as given earlier for the writePacket() method.

17

Ha ha! You are, of course, correct! I was just so happened to be testing with a packet that is...2 bytes! I converted it. I had wondered why it was working.

Updated post 13. I'll try to think of some more context...

If I could use sendPacketNDataByte[N] = {hex}...
Instead of having to declare arrays with ever increasing length and then using an if N = 2 choose the 2DataByte to fill, N=3 choose the 3DataByte to fill..

byte sendPacket2DataByte[3] = {0x00,0x00,0x00};
byte sendPacket3DataByte[4] = {0x00,0x00,0x00,0x00};
byte sendPacket4DataByte[5] = {0x00,0x00,0x00,0x00,0x00};

use sendPacket<N>DataByte[N+1]
18

This is an extremely inefficient solution. Templating like this will create an entire copy of the writePacket function for every different n that the program uses. If the function is more than a one-liner (which could be optimized away), this will unnecessarily duplicate your code. Templates are great, but if you can solve your problem without them that's usually for the best.

You can declare local arrays with a size based on a variable.

bool sendPacket(uint8_t* data, size_t data_length)
{
  uint8_t packet_buffer[data_length+1];
  ......

I can't help you in anymore without seeing your code implementation in more detail, but I just have to say that the buildPacketFromData sample in #13 is atrocious. Yes there's better ways of doing it, but it's difficult to give better advice without seeing how all your code interacts with each other. There's some especially subtle things to watch out for if you're passing arrays into functions or returning arrays from one.

1 Like
19

Thanks for that. I will (for now) stay away fromthat. Back to the old-fashioned C way of doing things :smiley:

20

that was the basis for the suggestion in post #12