SI4460 incorrectly assumes packed structs about sub-iot-stack HOT 3 CLOSED

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?

from sub-iot-stack.

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.

from sub-iot-stack.

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

Maarten

Sent from my Samsung Galaxy smartphone.

-------- Original message --------
From: Simon Haines [email protected]
Date: 26/05/2016 02:42 (GMT+01:00)
To: MOSAIC-LoPoW/dash7-ap-open-source-stack [email protected]
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

from sub-iot-stack.