SI4460 incorrectly assumes packed structs

[simonhaines]

The SI4460 code assumes the hw_radio_packet_t struct is packed, but the packed attribute was removed in commit 761cf7c in September 2015. As a result, three padding bytes are added to the end of the struct, and members of an enclosing struct (e.g. packet_struct_t in the radio_ping app) begin on a word boundary.

The SI4460 calculates the length of data to transmit correctly, but assumes that hw_radio_packet_t is packed and loads the Tx FIFO with the contents of the data flexible array member, which points to the length byte, the three padding bytes, then the remainder of the enclosing struct. The last three bytes of data are not transmitted.

When the packed attribute was removed, it was replaced with a comment regarding alignment on Cortex-M0, so this might be a tricky one to fix. It should also be noted that the requirement for hw_radio_packet_t to be packed also requires all enclosing structs to be packed, which includes structs defined at the app layer. As the requirement for packing is implicit, this may be a source of bugs for the unaware.

[glennergeerts]

Thanks for bringing this up!
As you said packed was removed for Cortex-M0 compatibility. And indeed this breaks the radio_ping app, i was not yet aware of this. In applications using the D7AP stack on top of the PHY this is not a problem since the packet struct which encapsulates the hw_radio_packet_t defines an array of explicit size right after the flexible array member. This size is fixed for now, and the plan was to make this a cmake option, to allow optimizing the memory usage for application which don't need big packet sizes. It is never really dynamically allocated though.
I'm not sure the C spec even allows/specifies adding struct members which are not arrays with defined size after the flexible array member, like radio_ping does. I propose to keep the struct non packed, state this more clearly in the hw_radio_packet_t comments and fix radio_ping (which can actually be removed coming to think of it). What do you think?

[simonhaines]

Only after I lodged this issue I realised that it did not matter for the D7AP stack. I stumbled over this as I was testing a new radio against each of the apps, including radio_ping.

I agree it is better to keep the struct unpacked, even if only for aligned access. A quick check of the C99 standard does not mention anything about using flexible array members in this way so it seems to be undefined behaviour. As for making packet sizes a CMake option, while nice to have, it's only an issue for memory-constrained systems and the current configuration is fine.

In this case to avoid further confusion, the radio_ping app should be removed--the phy_test app does pretty much the same thing anyway. Thanks for the clarification.

[maartenweyn]

Hi Simon,
Thanks for all the work and feedback. Really appreciate it!!!

Maarten

Sent from my Samsung Galaxy smartphone.

-------- Original message --------
From: Simon Haines notifications@github.com
Date: 26/05/2016 02:42 (GMT+01:00)
To: MOSAIC-LoPoW/dash7-ap-open-source-stack dash7-ap-open-source-stack@noreply.github.com
Subject: Re: [MOSAIC-LoPoW/dash7-ap-open-source-stack] SI4460 incorrectly assumes packed structs (#34)

Only after I lodged this issue I realised that it did not matter for the D7AP stack. I stumbled over this as I was testing a new radio against each of the apps, including radio_ping.

I agree it is better to keep the struct unpacked, even if only for aligned access. A quick check of the C99 standard does not mention anything about using flexible array members in this way so it seems to be undefined behaviour. As for making packet sizes a CMake option, while nice to have, it's only an issue for memory-constrained systems and the current configuration is fine.

In this case to avoid further confusion, the radio_ping app should be removed--the phy_test app does pretty much the same thing anyway. Thanks for the clarification.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHubhttps://github.com//issues/34#issuecomment-221747906