Two additional bytes of padding before strip flags?

[ssilverman]

PixelPusher-java/src/com/heroicrobot/dropbit/devices/pixelpusher/PixelPusher.java

Line 328 in dc8603f

stripFlags = Arrays.copyOfRange(packet, 32, 32+stripFlagSize);

If the strip flags start at index 32, wouldn’t that imply there’s another 2-byte pad?
See this issue: hzeller/pixelpusher-server#3

cc: @jasstrong @hzeller

[jasstrong]

The struct in the firmware follows the conventional C rules for member alignment, and PixelPusher is a 32-bit machine, so the strip flag array is aligned on 4.

…

On Jan 15, 2022, at 21:44, Shawn Silverman ***@***.***> wrote: https://github.com/robot-head/PixelPusher-java/blob/dc8603fc5a600d209dd5187023fc36aab40aca75/src/com/heroicrobot/dropbit/devices/pixelpusher/PixelPusher.java#L328 <https://github.com/robot-head/PixelPusher-java/blob/dc8603fc5a600d209dd5187023fc36aab40aca75/src/com/heroicrobot/dropbit/devices/pixelpusher/PixelPusher.java#L328> If the strip flags start at index 32, wouldn’t that imply there’s another 2-byte pad? See this issue: hzeller/pixelpusher-server#3 <hzeller/pixelpusher-server#3> cc: @jasstrong <https://github.com/jasstrong> @hzeller <https://github.com/hzeller> — Reply to this email directly, view it on GitHub <#17>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWOVE2OXNROVF2IRYIWB63UWJLKPANCNFSM5MB7IITA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[ssilverman]

There's two outstanding questions here:

1. Does the alignment in memory appear on the wire? For example, if there was padding before the strip flags because of struct alignment rules, does that mean there will be padding on the wire? Just because a struct is aligned a certain way in memory, doesn't mean the protocol does the same thing when transmitting those bytes. i.e. does the struct in memory appear identically on the wire?

2. From my readings, 2-byte values only need an even address alignment and 1-byte values (including arrays) can be anywhere. This would seem to imply that the strip flags could still be at byte 30 in its part of the struct.
   See https://developer.arm.com/documentation/dui0472/i/compiler-coding-practices/advantages-of-natural-data-alignment?lang=en: "halfwords that lie on addresses that are multiples of two, and single bytes that lie at any byte address"

Let me elaborate more on point # 2. We have this first part:

struct DiscoveryHeader {
    uint8_t mac_address[6];    // 0
    uint8_t ip_address[4];     // 6
    uint8_t device_type;       // 10
    uint8_t protocol_version;  // 11
    uint16_t vendor_id;        // 12
    uint16_t product_id;       // 14
    uint16_t hw_revision;      // 16
    uint16_t sw_revision;      // 18
    uint32_t link_speed;       // 20
}

That's 24 bytes, and I believe all the bytes will be placed in memory without padding because all 2-byte values are at even addresses and link_speed is at a divisible-by-4 address.

For the next part:

struct PixelPusherBase {
    uint8_t  strips_attached;        // 0
    uint8_t  max_strips_per_packet;  // 1
    uint16_t pixels_per_strip;       // 2
    uint32_t update_period;          // 4
    uint32_t power_total;            // 8
    uint32_t delta_sequence;         // 12
    int32_t controller_ordinal;      // 16
    int32_t group_ordinal;           // 20
    uint16_t artnet_universe;        // 24
    uint16_t artnet_channel;         // 26
    uint16_t my_port;                // 28
    // The following has dynamic length (but is at least 8 bytes):
    uint8_t strip_flags[8];          // 30
} 

Walking through this for all members before strip flags, all 2-byte and 4-byte values are aligned on even addresses and divisible-by-4 addresses, respectively. This means that with C-struct rules for memory layout, the strip_flags member is at byte 30.

If the struct is simply dumped onto the wire, then wouldn't strip_flags be at location 30 and not location 32? I believe that arrays of 1-byte values in structs can be aligned anywhere.

Here's my seemingly contradictory fact list:

1. According to both the PixelPusher.java source and Jas's comment above, strip_flags is at location 32.
2. The mention of struct alignment seems to imply that the struct in memory is simply sent directly on the wire when sending the discovery packet. This means that strip_flags would end up on byte 30.
3. Depending on which is true, the PixelPusher firmware either sends a 2-byte pad before strip_flags or it doesn't. If it's at location 32 then 2 padding bytes are sent. If it's at location 30 because the struct is dumped on the wire then no padding bytes are sent.

Assuming it's at location 32 (because I'm taking Jas's comment at the source of truth), then why isn't there a 16-bit padding member before strip_flags?

cc: @hzeller

[ssilverman]

I just plugged in a PixelPusher and recorded its announcement packets. It's an 84-byte packet and it looks like there's a 2-byte padding both before and after the strip flags. I think this is how to interpret the data, so it basically answers my question, but it doesn't address my theoretical question about why strip_flags needs to be at location 32 after the discovery header, even when considering struct alignment.

This leads to a third question (discussion first):
If the PixelPusher simply dumps memory onto the wire, then, in theory, if strip_flags is greater than 8, there might be anywhere between 0-3 bytes of padding before the pusher_flags field. If it doesn't write memory onto the wire, then will there always be two bytes of padding after strip_flags? On the other hand, since pusher_flags is at location 66 it would seem to imply that memory is not dumped onto the wire. It's a 4-byte value that needs to be 4-byte aligned. So here we have a case where strip_flags is aligned to location 32, even though it doesn't have to be, but the reason given is "struct alignment", but yet pusher_flags is not aligned.

Having said all that, the question is: In the protocol on the wire, how many bytes need to be before and after strip_flags? Is it always 2 bytes before and after, even if strip_flags is greater than 8 bytes? Or, given that these are all zeros, maybe there's no padding before strip_flags but 4 bytes of padding after? I'd have to see the code to know for sure. @jasstrong are you willing to paste here the part of the code that writes the struct values to the network? Is it a simple memcpy into the UDP buffer? Is it more than one, for the different parts? Alternatively, maybe someone has a packet dump that shows non-zero strip flags, then I'd know the answer for sure.

Received 84-byte packet:
----
0: mac: d8 80 39 66 29 b1
6: ip: 192.168.6.93 (c0 a8 06 5d)
10: device_type: 02
11: protocol_version: 01
12: vendor_id: 2 (02 00)
14: product_id: 1 (01 00)
16: hw_rev: 3 (03 00)
18: sw_rev: 143 (8f 00)
20: link speed: 100'000'000 (00 e1 f5 05)
----
0 (24): strips_attached: 08
1 (25): max strips per packet: 02
2 (26): pixels per strip: 240 (f0 00)
4 (28): update period: 100'000 (a0 86 01 00)
8 (32): power total: 00 00 00 00
12 (36): delta sequence: 00 00 00 00
16 (40): controller ord: 00 00 00 00
20 (44): group ord: 00 00 00 00
24 (48): artnet universe: 00 00
26 (50): artnet channel: 00 00
28 (52): port: 5078 (d6 13)
----
30 (54): PADDING[unsure]: 00 00
32 (56): strip_flags[UNSURE]: 00 00 00 00 00 00 00 00
40 (64): PADDING[unsure]: 00 00
42 (66): pusher_flags: 0c 00 00 00
46 (70): segments: 00 00 00 00
50 (74): power_domain: 00 00 00 00
54 (78): last_driven_ip: 00 00 00 00
58 (82): last_driven_port: 00 00

[rrrus]

Strip flags offset has been both +30 and +32 depending on the pusher firmware version. In rev 1.41 and earlier, strips flags offset was +30. In rev 1.42 and later it is +32.

[jasstrong]

It's literally just struct alignment. You have the source to this function in the firmware- you could take a look. Actual PixelPusher only ever supports eight strips, so it will never be larger than this.

…

-J.

On Jan 17, 2022, at 12:24, Shawn Silverman ***@***.***> wrote: I just plugged in a PixelPusher and recorded its announcement packets. It's an 84-byte packet and it looks like there's a 2-byte padding both before and after the strip flags. I think this is how to interpret the data, so it basically answers my question, but it doesn't address my theoretical question about why strip_flags needs to be at location 32 after the discovery header, even when considering struct alignment. This leads to a third question (discussion first): If the PixelPusher simply dumps memory onto the wire, then, in theory, if strip_flags is greater than 8, there might be anywhere between 0-3 bytes of padding before the pusher_flags field. If it doesn't write memory onto the wire, then will there always be two bytes of padding after strip_flags? On the other hand, since pusher_flags is at location 66 it would seem to imply that memory is not dumped onto the wire. It's a 4-byte value that needs to be 4-byte aligned. So here we have a case where strip_flags is aligned to location 32, even though it doesn't have to be, but the reason given is "struct alignment", but yet pusher_flags is not aligned. Having said all that, the question is: In the protocol on the wire, how many bytes need to be before and after strip_flags? Is it always 2 bytes before and after, even if strip_flags is greater than 8 bytes? Received 84-byte packet: ---- 0: mac: d8 80 39 66 29 b1 6: ip: 192.168.6.93 (c0 a8 06 5d) 10: device_type: 02 11: protocol_version: 01 12: vendor_id: 2 (02 00) 14: product_id: 1 (01 00) 16: hw_rev: 3 (03 00) 18: sw_rev: 143 (8f 00) 20: link speed: 100000000 (00 e1 f5 05) ---- 0 (24): strips_attached: 08 1 (25): max strips per packet: 02 2 (26): pixels per strip: 240 (f0 00) 4 (28): update period: 100000 (a0 86 01 00) 8 (32): power total: 00 00 00 00 12 (36): delta sequence: 00 00 00 00 16 (40): controller ord: 00 00 00 00 20 (44): group ord: 00 00 00 00 24 (48): artnet universe: 00 00 26 (50): artnet channel: 00 00 28 (52): port: 5078 (d6 13) ---- 30 (54): PADDING: 00 00 32 (56): strip_flags: 00 00 00 00 00 00 00 00 40 (64): PADDING: 00 00 42 (66): pusher_flags: 0c 00 00 00 46 (70): segments: 00 00 00 00 50 (74): power_domain: 00 00 00 00 54 (78): last_driven_ip: 00 00 00 00 58 (82): last_driven_port: 00 00 — Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWOVE5R564VCEXZZCI4FYTUWR3ITANCNFSM5MB7IITA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[jasstrong]

That's because the compiler version changed and the new compiler ignored the __packed annotation that RVCT used to respect...

…

On Jan 17, 2022, at 14:05, Rus Maxham ***@***.***> wrote: Strip flags offset has been both +30 and +32 depending on the pusher firmware version. In rev 1.41 and earlier, strips flags offset was +30. In rev 1.42 and later it is +32. — Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWOVE6XFD6ZBYAQHUTRR5LUWSHBDANCNFSM5MB7IITA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[ssilverman]

That's the thing. "Struct alignment" dictates that the strip flags should be at location 30 and not location 32. For both packed and unpacked. I'll contact you offline about the source.

[jasstrong]

It does not, because the element before it is not word-aligned.

…

On Jan 17, 2022, at 20:28, Shawn Silverman ***@***.***> wrote: That's the thing. "Struct alignment" dictates that the strip flags should be at location 30 and not location 32. For both packed and unpacked. I'll contact you offline about the source. — Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWOVE4ZPKIMYWXPAQZ64E3UWTUAVANCNFSM5MB7IITA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[ssilverman]

Here’s how I understand this; what am I missing:

The element before strip_flags is my_port (see PixelPusherBase above) and is a uint16_t, and that’s 16-bit-aligned in the struct. Byte arrays in a struct don’t need to be aligned, as far as I understand.

If we assume you’re correct and strip_flags is at location 32 in the memory layout of its struct, which of my statements above is incorrect? Did GCC change or something and made byte arrays in a struct align at 32-bit boundaries?

Update: I wrote a little test program with GCC 5.4.1 and showed that strip_flags is at location 30 and not 32 for both packed and unpacked. Which version of GCC changed this?

[ssilverman]

Here's the test program. Note that there's a 2-byte padding after strip_flags, as expected (size=40). When the struct is changed to packed, the size changes to 38, but strip_flags stays at 30. (Results below the program.)

#include <cstdint>
#include <cstdio>

struct PixelPusherBase {
  uint8_t  strips_attached;        // 0
  uint8_t  max_strips_per_packet;  // 1
  uint16_t pixels_per_strip;       // 2
  uint32_t update_period;          // 4
  uint32_t power_total;            // 8
  uint32_t delta_sequence;         // 12
  uint32_t controller_ordinal;     // 16
  uint32_t group_ordinal;          // 20
  uint16_t artnet_universe;        // 24
  uint16_t artnet_channel;         // 26
  uint16_t my_port;                // 28
  // The following has dynamic length (but is at least 8 bytes):
  uint8_t strip_flags[8];          // 30
};

PixelPusherBase s;

int main(int argc, const char * argv[]) {
  printf("sizeof(s): %lu\n", sizeof(s));
  printf("s: %p\n", &s);
  printf("\n");
  unsigned char *ps = (unsigned char *)&s;
  printf("strips_attached: %ld\n", (unsigned char *)&s.strips_attached - ps);
  printf("max_strips_per_packet: %ld\n", (unsigned char *)&s.max_strips_per_packet - ps);
  printf("pixels_per_strip: %ld\n", (unsigned char *)&s.pixels_per_strip - ps);
  printf("update_period: %ld\n", (unsigned char *)&s.update_period - ps);
  printf("power_total: %ld\n", (unsigned char *)&s.power_total - ps);
  printf("delta_sequence: %ld\n", (unsigned char *)&s.delta_sequence - ps);
  printf("controller_ordinal: %ld\n", (unsigned char *)&s.controller_ordinal - ps);
  printf("group_ordinal: %ld\n", (unsigned char *)&s.group_ordinal - ps);
  printf("artnet_universe: %ld\n", (unsigned char *)&s.artnet_universe - ps);
  printf("artnet_channel: %ld\n", (unsigned char *)&s.artnet_channel -ps);
  printf("my_port: %ld\n", (unsigned char *)&s.my_port - ps);
  printf("strip_flags: %ld\n", (unsigned char *)&s.strip_flags - ps);

  return 0;
}

Results on a Mac (latest Xcode, v13.2.1, clang 13.0.0):

sizeof(s): 40
s: 0x100008010

strips_attached: 0
max_strips_per_packet: 1
pixels_per_strip: 2
update_period: 4
power_total: 8
delta_sequence: 12
controller_ordinal: 16
group_ordinal: 20
artnet_universe: 24
artnet_channel: 26
my_port: 28
strip_flags: 30

Results on a Teensy 4 (imxrt1062 processor, 32-bit ARM core, GCC v5.4.1, replacing printf with Serial.printf):

sizeof(s): 40
s: 0x20001910

strips_attached: 0
max_strips_per_packet: 1
pixels_per_strip: 2
update_period: 4
power_total: 8
delta_sequence: 12
controller_ordinal: 16
group_ordinal: 20
artnet_universe: 24
artnet_channel: 26
my_port: 28
strip_flags: 30

[jasstrong]

Byte arrays are four-byte aligned because sizeof(uint8_t *) == 4

…

On Jan 18, 2022, at 08:38, Shawn Silverman ***@***.***> wrote: Here’s how I understand this; what am I missing? The element before strip_flags is my_port (see PixelPusherBase) and is a uint16_t, and that’s 16-bit-aligned in the struct. Byte arrays in a struct don’t need to be aligned, as far as I understand. If we assume you’re correct and strip_flags is at location 32 in the memory layout of its struct, which of my statements above is incorrect? Did GCC change or something and made byte arrays in a struct align at 32-bit boundaries? — Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWOVE4IZX4BR6Y3JVUMZWTUWWJR7ANCNFSM5MB7IITA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[ssilverman]

Byte arrays inside structs are not stored as pointers. sizeof(uint8_t[8]) is 8. If instead the member was actually a uint8_t* then it would be word-aligned. uint8_t[8], however, is a different type and is byte-aligned in the struct. If you were to run the program from my previous comment on the hardware of your choice, it will show this.

[jasstrong]

sure, but that's not the behaviour that RVCT has. RVCT isn't (or at least wasn't, as of 3.0 or whatever version I was using in 2013) GCC or Clang.

…

On Jan 18, 2022, at 09:10, Shawn Silverman ***@***.***> wrote: Here's the test program. Note that there's a 2-byte padding after strip_flags, as expected (size=40). When the struct is changed to packed, the size changes to 38, but strip_flags stays at 30. (Results below the program.) #include <cstdint> #include <cstdio> struct PixelPusherBase { uint8_t strips_attached; // 0 uint8_t max_strips_per_packet; // 1 uint16_t pixels_per_strip; // 2 uint32_t update_period; // 4 uint32_t power_total; // 8 uint32_t delta_sequence; // 12 uint32_t controller_ordinal; // 16 uint32_t group_ordinal; // 20 uint16_t artnet_universe; // 24 uint16_t artnet_channel; // 26 uint16_t my_port; // 28 // The following has dynamic length (but is at least 8 bytes): uint8_t strip_flags[8]; // 30 }; PixelPusherBase s; int main(int argc, const char * argv[]) { printf("sizeof(s): %lu\n", sizeof(s)); printf("s: %p\n", &s); printf("\n"); unsigned char *ps = (unsigned char *)&s; printf("strips_attached: %ld\n", (unsigned char *)&s.strips_attached - ps); printf("max_strips_per_packet: %ld\n", (unsigned char *)&s.max_strips_per_packet - ps); printf("pixels_per_strip: %ld\n", (unsigned char *)&s.pixels_per_strip - ps); printf("update_period: %ld\n", (unsigned char *)&s.update_period - ps); printf("power_total: %ld\n", (unsigned char *)&s.power_total - ps); printf("delta_sequence: %ld\n", (unsigned char *)&s.delta_sequence - ps); printf("controller_ordinal: %ld\n", (unsigned char *)&s.controller_ordinal - ps); printf("group_ordinal: %ld\n", (unsigned char *)&s.group_ordinal - ps); printf("artnet_universe: %ld\n", (unsigned char *)&s.artnet_universe - ps); printf("artnet_channel: %ld\n", (unsigned char *)&s.artnet_channel -ps); printf("my_port: %ld\n", (unsigned char *)&s.my_port - ps); printf("strip_flags: %ld\n", (unsigned char *)&s.strip_flags - ps); return 0; } Results on a Mac (latest Xcode, v13.2.1, clang 13.0.0): sizeof(s): 40 s: 0x100008010 strips_attached: 0 max_strips_per_packet: 1 pixels_per_strip: 2 update_period: 4 power_total: 8 delta_sequence: 12 controller_ordinal: 16 group_ordinal: 20 artnet_universe: 24 artnet_channel: 26 my_port: 28 strip_flags: 30 Results on a Teensy 4 (imxrt1062 processor, 32-bit ARM core, GCC v5.4.1, replacing printf with Serial.printf): sizeof(s): 40 s: 0x20001910 strips_attached: 0 max_strips_per_packet: 1 pixels_per_strip: 2 update_period: 4 power_total: 8 delta_sequence: 12 controller_ordinal: 16 group_ordinal: 20 artnet_universe: 24 artnet_channel: 26 my_port: 28 strip_flags: 30 — Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWOVE4ECCM7UCJHNNEZBRTUWWNG7ANCNFSM5MB7IITA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[ssilverman]

Ok, I hear you. Sounds like RVCT doesn’t follow the traditional C struct alignment rules and word-aligns byte arrays?