struct and memcpy problem maybe?

Hi All,

To keep things simple I wont explain my whole project just yet, but you need to know that I'm trying to write a library to control some LIFX WiFi bulbs.

They have a LAN protocol that uses udp packets with little-endian encoding.

The udp packet is broken up into header and payload, for now I'm only working on getting the header added to the udp packet correctly and I'am working on a Arduino UNO (WiFi shield in the post).

what I have so far (Disclaimer: I'm not a c++ programmer and am learning this as I go, Things could be very wrong.)

Main.ino

#include "device.h"

device Device(Serial);

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);

  /* Find Devices */
  Device.GetService();

  /* Control Light */

}

void loop() {
  // put your main code here, to run repeatedly:

}

device.h

  #ifndef device_h
  #define device_h
  
  /* includes */
  #include <Stdint.h>
  #if defined(ARDUINO) && ARDUINO >= 100
    #include "Arduino.h"
  #else
    #include "WProgram.h"
  #endif

class device{
  public:
    /* Member Functions */
    device(Stream& serial);
    void GetService();
  
    /* Members */
    #pragma pack(push, 1)
      typedef struct {
      /* frame */
      uint16_t size;
      uint8_t  origin:2;
      uint8_t  tagged:1;
      uint8_t  addressable:1;
      uint16_t protocol:12;
      uint32_t source;
      /* frame address */
      uint8_t  target[8];
      uint8_t  reservedA[6];
      uint8_t  reservedB:6;
      uint8_t  ack_required:1;
      uint8_t  res_required:1;
      uint8_t  sequence;
      /* protocol header */
      uint64_t reservedC;
      uint16_t type;
      uint16_t reservedD;
    } Header;
    #pragma pack(pop)

  private:
    /* Members */ 
    // _Udp;
    Stream *_serial;
  
    enum msg{
      _GetService = 2,
      _StateService = 3,
      _GetHostInfo = 12,
      _StateHostInfo = 13,
      _GetHostFirmware = 14,
      _StateHostFirmware = 15,
      _GetWifiInfo = 16,
      _StateWifiInfo = 17,
      _GetWifiFirmware = 18,
      _StateWifiFirmware = 19,
      _GetPower = 20,
      _SetPower = 21,
      _StatePower = 22,
      _GetLabel = 23,
      _SetLabel = 24,
      _StateLabel = 25,
      _GetVersion = 32,
      _StateVersion = 33,
      _GetInfo = 34,
      _StateInfo = 35,
      _Acknowledgement = 45,
      _EchoRequest = 58,
      _EchoResponse = 59
    };

};

#endif

device.cpp

#include "device.h"

device::device(Stream& serial) : _serial(&serial)
 {
  /* debug serial interface */
 };

void device::GetService()
{
  /* header */
  Header header;
  int headerSize = sizeof(header);

	/* payload */
	// N/A

  /* UDP Packet */
  uint8_t udpPacket[sizeof(header)];

  /* Debug */
  byte *ptr = (byte*) &header;
  _serial->print("Header Before:");
  for (int i = 0; i < sizeof(header) ; i++){
      //_serial->print(i);
      //_serial->print(":");
      //_serial->print("0x");
      if (*ptr < 16)
        {
          _serial->print("0");
          _serial->print(*ptr++,HEX);
        }
       else
       {
          _serial->print(*ptr++,HEX);
       }
      _serial->print(" ");
    }
  _serial->println("");
  _serial->println("");
  
  /* build header */
  header.size = sizeof(header); 
  header.origin = 0;            
  header.tagged = 1;            
  header.addressable = 1;       
  header.protocol = 1024;       
  header.source = 0;
  header.target[0] = 0;
  header.target[1] = 0;
  header.target[2] = 0;
  header.target[3] = 0; 
  header.target[4] = 0; 
  header.target[5] = 0; 
  header.target[6] = 0; 
  header.target[7] = 0;
  header.reservedA[0] = 0;
  header.reservedA[1] = 0;
  header.reservedA[2] = 0;
  header.reservedA[3] = 0;
  header.reservedA[4] = 0;
  header.reservedA[5] = 0;
  header.reservedB = 0;
  header.ack_required = 0;
  header.res_required = 0;
  header.sequence = 0;
  header.reservedC = 0;
  header.type = _GetService;
  header.reservedD = 0;

   _serial->print("Header After: ");
  for (int i = 0; i < sizeof(header) ; i++){
      //_serial->print(i);
      //_serial->print(":");
      //_serial->print("0x");
       if (*ptr < 16)
        {
          _serial->print("0");
          _serial->print(*ptr++,HEX);
        }
       else
       {
          _serial->print(*ptr++,HEX);
       }
      _serial->print(" ");
    }
  _serial->println("");
  _serial->println("");
  /* build payload */
  // N/A

  /* build udp packet */
  memcpy(&udpPacket, &header, sizeof(header));

  /* Debug */
  _serial->print("Device UDP Packet:");
  for (int i = 0; i < sizeof(udpPacket) ; i++){
     // _serial->print(i);
      //_serial->print(":");
      //_serial->print("0x");
      if (udpPacket[i] < 16)
        {
          _serial->print("0");
          _serial->print(udpPacket[i],HEX);
        }
       else
        {
          _serial->print(udpPacket[i],HEX);
        }
      _serial->print(" ");
    }
  _serial->println("");
  _serial->println("");

};

Debug Log (Added in the Expecting lines):

Header Before:24 00 0C 40 00 03 48 01 C5 01 C5 03 04 01 0A 00 02 00 00 23 A3 00 02 65 00 01 49 00 01 03 48 00 C1 01 C5 00 
(Expecting: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)

Header After: 00 68 00 80 7F 00 00 00 03 05 11 01 03 9B 00 5C 48 65 61 64 65 72 20 42 65 66 6F 72 65 3A 00 30 00 48 65 61 
(Expecting: 24 00 00 34 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00)

Device UDP Packet:24 00 0C 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00
(Expecting: 24 00 00 34 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00)

From what I have read the UNO is little-endian by default (don't know if you can change it?) so storing a uint16_t of 0xABCD in memory should store as 0xCDAB.

so working on that theory if I build a struct with defined types I should get the little-endian encoding I'm looking for and should be able to copy it to the udp packet without any changes.

so my questions:
Why is the before header print showing values?
Why is the after header print not showing the correct values?
Why is the UDP packet so close but this not correct?

I have a theory on the UDP been close, it has something to do with the way I have done the bit packing in the strut e.g.

Struct:
  uint8_t  origin:2;
  uint8_t  tagged:1;
  uint8_t  addressable:1;
  uint16_t protocol:12;

Set to:
  header.origin = 0;            
  header.tagged = 1;            
  header.addressable = 1;       
  header.protocol = 1024;

I thought in memory that would be 0x00 0x34 but it is coming out as 0x0C 0x40, so I don't think it's packing the way I think it should.

As for the other two problems I just don't know yet, maybe it's the way I'm printing the debug??

any insight would be greatly appreciated, again I'm learning this on the fly give me all the pointers you have :).

header appears to be an uninitialized local variable. What makes you think it should contain anything but garbage?

As for the rest, since you don't seem to have shown us where you've defined Header.....

Regards,
Ray L.

Thanks Ray,

In device.cpp I changed this

/* header */
  Header header;
  int headerSize = sizeof(header);

to this

/* header */
  Header header = Header();
  int headerSize = sizeof(header);

and the garbage is gone.
Did not realise that you needed to initialise structs, Thanks for that.

Now I get:

Header Before:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

Header After: 00 68 00 00 7F 00 00 00 03 05 11 01 03 A0 00 5C 48 65 61 64 65 72 20 42 65 66 6F 72 65 3A 00 30 00 48 65 61 

Device UDP Packet:24 00 0C 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00

So any clues to the other two?

RayLivingston:
As for the rest, since you don't seem to have shown us where you've defined Header.....

Regards,
Ray L.

Sorry if I'm misunderstanding, But I thought I was defining it in device.h

    /* Members */
    #pragma pack(push, 1)
      typedef struct {
      /* frame */
      uint16_t size;
      uint8_t  origin:2;
      uint8_t  tagged:1;
      uint8_t  addressable:1;
      uint16_t protocol:12;
      uint32_t source;
      /* frame address */
      uint8_t  target[8];
      uint8_t  reservedA[6];
      uint8_t  reservedB:6;
      uint8_t  ack_required:1;
      uint8_t  res_required:1;
      uint8_t  sequence;
      /* protocol header */
      uint64_t reservedC;
      uint16_t type;
      uint16_t reservedD;
    } Header;
    #pragma pack(pop)

is there other way I should be defining it?

Ok kind of worked out the UDP Packet.

In device.h changed

    #pragma pack(push, 1)
      typedef struct {
      /* frame */
      uint16_t size;
      uint8_t  origin:2;
      uint8_t  tagged:1;
      uint8_t  addressable:1;
      uint16_t protocol:12;
      uint32_t source;
      /* frame address */
      uint8_t  target[8];
      uint8_t  reservedA[6];
      uint8_t  reservedB:6;
      uint8_t  ack_required:1;
      uint8_t  res_required:1;
      uint8_t  sequence;
      /* protocol header */
      uint64_t reservedC;
      uint16_t type;
      uint16_t reservedD;
    } Header;
    #pragma pack(pop)

to

    struct Header
    {
      /* frame */
      uint16_t size;
      uint16_t protocol:12;
      uint8_t  addressable:1;
      uint8_t  tagged:1;
      uint8_t  origin:2;
      uint32_t source;
      /* frame address */
      uint8_t  target[8];
      uint8_t  reservedA[6];
      uint8_t  res_required:1;
      uint8_t  ack_required:1;
      uint8_t  reservedB:6;
      uint8_t  sequence;
      /* protocol header */
      uint64_t reservedC;
      uint16_t type;
      uint16_t reservedD;
    }__attribute__ ((__packed__));

note also rearranged this

//      /* frame */
      uint16_t size;
      uint8_t  origin:2;
      uint8_t  tagged:1;
      uint8_t  addressable:1;
      uint16_t protocol:12;
      uint32_t source;

to

      /* frame */
      uint16_t size;
      uint16_t protocol:12;
      uint8_t  addressable:1;
      uint8_t  tagged:1;
      uint8_t  origin:2;
      uint32_t source;

and rearranged this

      /* frame address */
      uint8_t  target[8];
      uint8_t  reservedA[6];
      uint8_t  reservedB:6;
      uint8_t  ack_required:1;
      uint8_t  res_required:1;
      uint8_t  sequence;

to

      /* frame address */
      uint8_t  target[8];
      uint8_t  reservedA[6];
      uint8_t  res_required:1;
      uint8_t  ack_required:1;
      uint8_t  reservedB:6;
      uint8_t  sequence;

all the little-endian line up the way I need them to, still really don't understand why (more reading is needed I think)

Now I get:

Header Before:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

Header After: 00 68 00 00 7F 00 00 00 03 05 11 01 03 9F 00 5C 48 65 61 64 65 72 20 42 65 66 6F 72 65 3A 00 30 00 48 65 61 

Device UDP Packet:24 00 00 34 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00

Header Before: Good
UDP Packet: Good

Still don't understand Header After??

You forgot to reset ptr before dumping "Header After" and "Device UDP Packet".