From afb8a20b6f56082c852673110eb423cfe68d5902 Mon Sep 17 00:00:00 2001 From: "Richard Flay (Hyper)" Date: Sun, 14 Apr 2013 11:52:56 +0930 Subject: [PATCH 1/2] OP-913: tweaked some UAVO related struct alignments to attempt to ensure that UAVO data fields end up being 4 byte aligned on the heap. Also fixed a memset issue in the initialisation code for multi-instance UAVOs . +review OPReview --- .../targets/UAVObjects/inc/uavobjectmanager.h | 2 +- flight/targets/UAVObjects/uavobjectmanager.c | 17 ++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/flight/targets/UAVObjects/inc/uavobjectmanager.h b/flight/targets/UAVObjects/inc/uavobjectmanager.h index 9383e2e84..53183f497 100644 --- a/flight/targets/UAVObjects/inc/uavobjectmanager.h +++ b/flight/targets/UAVObjects/inc/uavobjectmanager.h @@ -75,7 +75,7 @@ typedef enum { * 6-7 gcsTelemetryUpdateMode Update mode used by the GCS (UAVObjUpdateMode) */ typedef struct { - uint8_t flags; /** Defines flags for update and logging modes and whether an update should be ACK'd (bits defined above) */ + uint8_t flags; /** Defines flags for update and logging modes and whether an update should be ACK'd (bits defined above) */ uint16_t telemetryUpdatePeriod; /** Update period used by the telemetry module (only if telemetry mode is PERIODIC) */ uint16_t gcsTelemetryUpdatePeriod; /** Update period used by the GCS (only if telemetry mode is PERIODIC) */ uint16_t loggingUpdatePeriod; /** Update period used by the logging module (only if logging mode is PERIODIC) */ diff --git a/flight/targets/UAVObjects/uavobjectmanager.c b/flight/targets/UAVObjects/uavobjectmanager.c index 1f07e0c9c..2af6a7a05 100644 --- a/flight/targets/UAVObjects/uavobjectmanager.c +++ b/flight/targets/UAVObjects/uavobjectmanager.c @@ -60,10 +60,10 @@ for (struct UAVOData ** _uavo_slot = __start__uavo_handles; \ typedef void* InstanceHandle; struct ObjectEventEntry { + struct ObjectEventEntry * next; xQueueHandle queue; UAVObjEventCallback cb; uint8_t eventMask; - struct ObjectEventEntry * next; }; /* @@ -91,7 +91,6 @@ struct UAVOBase { bool isSingle : 1; bool isSettings : 1; } flags; - } __attribute__((packed)); /* Augmented type for Meta UAVO */ @@ -110,7 +109,7 @@ struct UAVOData { */ struct UAVOMeta metaObj; uint16_t instance_size; -} __attribute__((packed)); +} __attribute__((packed, aligned(4))); /* Augmented type for Single Instance Data UAVO */ struct UAVOSingle { @@ -136,9 +135,8 @@ struct UAVOMultiInst { /* Augmented type for Multi Instance Data UAVO */ struct UAVOMulti { struct UAVOData uavo; - uint16_t num_instances; - struct UAVOMultiInst instance0; + struct UAVOMultiInst instance0 __attribute__((aligned(4))); /* * Additional space will be malloc'd here to hold the * the data for instance 0. @@ -296,8 +294,8 @@ static struct UAVOData * UAVObjAllocMulti(uint32_t num_bytes) /* Set up the type-specific part of the UAVO */ uavo_multi->num_instances = 1; - /* Clear the instance data carried in the UAVO */ - memset (&(uavo_multi->instance0), 0, num_bytes); + /* Clear the multi instance data carried in the UAVO */ + memset (&(uavo_multi->instance0), 0, sizeof(struct UAVOMultiInst) + num_bytes); /* Give back the generic UAVO part */ return (&(uavo_multi->uavo)); @@ -1864,10 +1862,11 @@ static InstanceHandle createInstance(struct UAVOData * obj, uint16_t instId) } /* Create the actual instance */ - instEntry = (struct UAVOMultiInst *) pvPortMalloc(sizeof(struct UAVOMultiInst)+obj->instance_size); + uint32_t size = sizeof(struct UAVOMultiInst) + obj->instance_size; + instEntry = (struct UAVOMultiInst *) pvPortMalloc(size); if (!instEntry) return NULL; - memset(InstanceDataOffset(instEntry), 0, obj->instance_size); + memset(instEntry, 0, size); LL_APPEND(( (struct UAVOMulti*)obj )->instance0.next, instEntry); ( (struct UAVOMulti*)obj )->num_instances++; From 146e082e74c34ff17ebdcc72da740956f13aeac4 Mon Sep 17 00:00:00 2001 From: "Richard Flay (Hyper)" Date: Tue, 23 Apr 2013 22:32:38 +0930 Subject: [PATCH 2/2] OP-913: ensured that UAVO data structs in .bss/.data sections and on the stack have 4 byte alignment. Also changed linker options to sort common and sort sections by alignment, which reduces the amount of fill/padding in the .bss and .data sections. +review OPReview-444 --- flight/PiOS/inc/pios_debug.h | 6 ++++++ flight/targets/UAVObjects/inc/uavobjecttemplate.h | 12 ++++++++++-- flight/targets/UAVObjects/uavobjecttemplate.c | 5 +++++ make/common-defs.mk | 2 +- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/flight/PiOS/inc/pios_debug.h b/flight/PiOS/inc/pios_debug.h index 4bc1e0802..6cd151e30 100644 --- a/flight/PiOS/inc/pios_debug.h +++ b/flight/PiOS/inc/pios_debug.h @@ -68,6 +68,12 @@ void PIOS_DEBUG_Panic(const char *msg); #define PIOS_Assert(test) if (!(test)) while (1); #endif +/* Static (compile-time) assertion for use in a function. + If test evaluates to 0 (ie fails) at compile time then compilation will + fail with the error: "size of unnamed array is negative" */ +#define PIOS_STATIC_ASSERT(test) ((void)sizeof(int[1 - 2*!(test)])) + + #endif /* PIOS_DEBUG_H */ /** diff --git a/flight/targets/UAVObjects/inc/uavobjecttemplate.h b/flight/targets/UAVObjects/inc/uavobjecttemplate.h index 8aee9ded0..5734f5960 100644 --- a/flight/targets/UAVObjects/inc/uavobjecttemplate.h +++ b/flight/targets/UAVObjects/inc/uavobjecttemplate.h @@ -51,11 +51,19 @@ int32_t $(NAME)Initialize(); UAVObjHandle $(NAME)Handle(); void $(NAME)SetDefaults(UAVObjHandle obj, uint16_t instId); -// Object data +// Packed Object data (unaligned). +// Should only be used where 4 byte alignment can be guaranteed +// (eg a single instance on the heap) typedef struct { $(DATAFIELDS) -} __attribute__((packed)) $(NAME)Data; +} __attribute__((packed)) $(NAME)DataPacked; +// Packed Object data. +// Alignment is forced to 4 bytes so as to avoid the potential for CPU usage faults +// on Cortex M4F during load/store of float UAVO fields +typedef $(NAME)DataPacked __attribute__((aligned(4))) $(NAME)Data; + + // Typesafe Object access functions /** * @function $(NAME)Get(dataOut) diff --git a/flight/targets/UAVObjects/uavobjecttemplate.c b/flight/targets/UAVObjects/uavobjecttemplate.c index 808faf496..271cb5c3a 100644 --- a/flight/targets/UAVObjects/uavobjecttemplate.c +++ b/flight/targets/UAVObjects/uavobjecttemplate.c @@ -49,6 +49,11 @@ static UAVObjHandle handle __attribute__((section("_uavo_handles"))); */ int32_t $(NAME)Initialize(void) { + // Compile time assertion that the $(NAME)DataPacked and $(NAME)Data structs + // have the same size (though instances of $(NAME)Data + // should be placed in memory by the linker/compiler on a 4 byte alignment). + PIOS_STATIC_ASSERT(sizeof($(NAME)DataPacked) == sizeof($(NAME)Data)); + // Don't set the handle to null if already registered if(UAVObjGetByID($(NAMEUC)_OBJID) != NULL) return -2; diff --git a/make/common-defs.mk b/make/common-defs.mk index 97ee3a818..25b8f9bc1 100644 --- a/make/common-defs.mk +++ b/make/common-defs.mk @@ -153,7 +153,7 @@ ASFLAGS += -Wa,-adhlns=$(addprefix $(OUTDIR)/, $(notdir $(addsuffix .lst, $(base # -Map: create map file # --cref: add cross reference to map file LDFLAGS += -nostartfiles -LDFLAGS += -Wl,--warn-common,--fatal-warnings,--gc-sections +LDFLAGS += -Wl,--warn-common,--fatal-warnings,--sort-common,--sort-section=alignment,--gc-sections LDFLAGS += -Wl,-Map=$(OUTDIR)/$(TARGET).map,--cref LDFLAGS += $(patsubst %,-L%,$(EXTRA_LIBDIRS)) LDFLAGS += $(patsubst %,-l%,$(EXTRA_LIBS))