Metadata fields in rfxlink stack should have nx_ prefix

[ucolesanti]

This is what I understood from my case (I am not expert of these stuff, I apologize for possible errors):
The metadata fields of the message_t, as opposed to the other fields, are defined as struct and not nx_struct in the rfxlink radio stack. This is probably related to the fact that they are not actually transmitted, hence they do not need to be platform independent using external struct (check "nesc/doc/user/network-types.txt" in this repository for details).
In 32-bit architectures like my cortex m0+ this will produce some padding in the struct if it is not word-aligned. As an example, the rf230 driver defines the following (I omitted optional fields):

typedef struct rf230packet_metadata_t
{
timestamp_metadata_t timestamp;
flags_metadata_t flags;
rf230_metadata_t rf230;
}rf230packet_metadata_t;

Calling sizeof(rf230packet_metadata_t) will give 7 on an 8 bit AVR but will result in 8 bytes on my 32 bit architecture. However, functions in the rfxlink stack access to the metadata fields calling something like this (the example is from the Timestamping layer):

timestamp_metadata_t* getMeta(message_t* msg)
{
return ((void*)msg) + sizeof(message_t) - call RadioPacket.metadataLength(msg);
}

which will recursively call RadioPacket.metadataLength on all sublayers components and add the respective field size. In the TimeStamping layer this will result in:

sizeof(timestamp_metadata_t) + sizeof(flags_metadata_t) + sizeof(rf233_metadata_t) + 0 = 7

Called in TimestampingLayerP,MetadataFlagsLayerC,RF230DriverLayerP and RF230RadioP respectively.
This means that getMeta() will return a pointer to the end of the message_t - 7. But in 32 bit architectures this should be -8 . This will generate an unaligned access to the timestamp field that is not supported by the ARM architecture and generates an HardFault exception.
I solved the issue by putting the attribute((packed)) type attribute to the rf230packet_metadata_t definition, but since it depends on the architecture, this should be done to the rfxlink stack by putting the nx_prefix to all the metadata fields ( I already tested and it works). What do you think?

Regards,

Ugo

[sallai]

How about having a #define that allows for easily switching between struct and nx_struct on a per platform basis?

C structs should perform better on 8-bit MCUs, but the overhead of using nx_structs would outweigh the hassles with the packed and aligned GCC attributes on an ARM platform.

Alternatively, we could go ahead and use nx_structs uniformly for all platforms. That would make the code a bit easier to understand.

[mmaroti]

We should put attribute((packed)) attribute on all metadata field structs. I think that would solve our problem and I guess it generates better code than nx_structs. The nx_struct already has to enable the packed attribute but also potentially do endianness conversion. Ucolesanti, can you put together a patch to all architectures?

[ucolesanti]

I can do that. But I noticed that non-rfxlink based radios (cc1000,cc2420,tda5250,xe1205) already define the metadata field as nx_struct, while rfxlink based (cc2420x,cc2520,rfa1, rf212 and rf230) don't. Should I add the attributes to rfxlink-based only?

[mmaroti]

Only for rfxlink based radios. Search for flags_metadata_t in the tos directory, there are a lot of users, not just rf230. Thanks! Miklos

[ucolesanti]

Maybe I'm getting paranoid, but I did some further search and found these links:

http://stackoverflow.com/questions/1756811/does-gccs-attribute-packed/7956942#7956942
http://stackoverflow.com/questions/8568432/is-gccs-attribute-packed-pragma-pack-unsafe

Both say that accessing a packed struct member through a pointer that requires alignment is unsafe. But this is exactly the way the metadata fields are accessed through the getMeta functions in each component.
I thought that maybe my code was working just because the timestamp field I was checking had an offset of 40 bytes, that by chance was word aligned. I did a check by increasing the TOSH_DATA_LENGTH by 1 (now timestamp had 41 bytes offset from the beginning of message_t, but it was still working (or at least it was not generating hard faults).
However I checked the assembler and got something like this:
r0 is 20000264 -> word aligned
......
00000560 <TimeStampingLayerP$0$PacketTimeStampRadio$set>:
560: 3029 adds r0, #41 ; -> r0 becomes 20000305 -> not word aligned
562: 6001 str r1, [r0, #0] -> str on a non word aligned address -> should generate a fault!
564: 4770 bx lr

Honestly as for now I'am quite lost.... In any case it seems that the issue might become more complex than simply adding the packed attribute. Any thoughts?

[ucolesanti]

Sorry, my fault. I was thinking that timestamp_metadata_t was a typedef to uint32_t while it is another struct. By adding the packed attribute to it, as suggested by Miklos, the compiler seems to correctly manage the whole thing. Tomorrow I prepare the patch and check if it works on iris and telosb, as much as my cortex platform.