pbio/sys/storage_data: fix strict aliasing issues

lib/pbio/drv/block_device/block_device_ev3.c

@@ -20,13 +20,6 @@
 #include <stdint.h>
 #include <string.h>
 
-#include "../core.h"
-
-#include <pbdrv/block_device.h>
-#include <pbdrv/clock.h>
-#include <pbdrv/compiler.h>
-#include <pbdrv/gpio.h>
-
 #include <tiam1808/edma.h>
 #include <tiam1808/spi.h>
 #include <tiam1808/psc.h>
@@ -36,9 +29,15 @@
 #include <tiam1808/armv5/am1808/edma_event.h>
 #include <tiam1808/armv5/am1808/interrupt.h>
 
-#include "block_device_ev3.h"
+#include "../core.h"
 #include "../drv/gpio/gpio_ev3.h"
+#include "../sys/storage_data.h"
+#include "block_device_ev3.h"
 
+#include <pbdrv/block_device.h>
+#include <pbdrv/clock.h>
+#include <pbdrv/compiler.h>
+#include <pbdrv/gpio.h>
 #include <pbio/error.h>
 #include <pbio/int_math.h>
 #include <pbio/util.h>
@@ -831,7 +830,11 @@ static struct {
      * portion of this, up to pbdrv_block_device_get_writable_size() bytes,
      * gets saved to flash at shutdown.
      */
-    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    union {
+        // ensure that data is properly aligned for pbsys_storage_data_map_t
+        pbsys_storage_data_map_t data_map;
+        uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    };
 } ramdisk __attribute__((section(".noinit"), used));
 
 uint32_t pbdrv_block_device_get_writable_size(void) {
@@ -840,8 +843,8 @@ uint32_t pbdrv_block_device_get_writable_size(void) {
 
 static pbio_error_t pbdrv_block_device_load_err = PBIO_ERROR_FAILED;
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = ramdisk.data;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
 
     // Higher level code can use the ramdisk data if initialization completed
     // successfully. Otherwise it should reset to factory default data.

lib/pbio/drv/block_device/block_device_flash_stm32.c

@@ -10,9 +10,8 @@
 #include <stdint.h>
 #include <string.h>
 
-#include <contiki.h>
-
 #include "../core.h"
+#include "../sys/storage_data.h"
 
 #include <pbdrv/block_device.h>
 
@@ -41,7 +40,11 @@ static struct {
      * portion of this, up to pbdrv_block_device_get_writable_size() bytes,
      * gets saved to flash at shutdown.
      */
-    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    union {
+        // ensure that data is properly aligned for pbsys_storage_data_map_t
+        pbsys_storage_data_map_t data_map;
+        uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    };
 } ramdisk __attribute__((section(".noinit"), used));
 
 const uint32_t header_size = sizeof(ramdisk.saved_size) + sizeof(ramdisk.checksum_complement);
@@ -70,8 +73,8 @@ void pbdrv_block_device_init(void) {
     memcpy(&ramdisk, _pbdrv_block_device_storage_start, size);
 }
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = ramdisk.data;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
     return init_err;
 }
 

lib/pbio/drv/block_device/block_device_test.c

@@ -10,8 +10,9 @@
 #include <stdint.h>
 #include <string.h>
 
-#include <pbdrv/block_device.h>
+#include "../sys/storage_data.h"
 
+#include <pbio/os.h>
 #include <pbio/version.h>
 
 /**
@@ -59,33 +60,30 @@ static const uint8_t _program_data[] = {
 // version at the right place. FIXME: Move the git version to pybricks build
 // system, since it isn't actually the micropython git version.
 #include "genhdr/mpversion.h"
-#include <pbsys/storage.h>
+
 
 static struct {
-    uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE];
-    char stored_firmware_hash[8];
-    pbsys_storage_settings_t settings;
-    uint32_t program_offset;
-    uint32_t program_size;
-    uint8_t program_data[sizeof(_program_data)];
-} ramdisk __attribute__((section(".noinit"), used)) = { 0 };
+    // ensure that data is properly aligned for pbsys_storage_data_map_t
+    pbsys_storage_data_map_t data_map;
+    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+} ramdisk;
 
 uint32_t pbdrv_block_device_get_writable_size(void) {
     return 0;
 }
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = (void *)&ramdisk;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
 
     // Higher level code can use the ramdisk data if initialization completed
     // successfully. Otherwise it should reset to factory default data.
     return PBIO_SUCCESS;
 }
 
 void pbdrv_block_device_init(void) {
-    ramdisk.program_size = sizeof(_program_data);
-    memcpy(&ramdisk.stored_firmware_hash[0], MICROPY_GIT_HASH, sizeof(ramdisk.stored_firmware_hash));
-    memcpy(&ramdisk.program_data[0], _program_data, sizeof(_program_data));
+    ramdisk.data_map.slot_info[0].size = sizeof(_program_data);
+    memcpy(ramdisk.data_map.stored_firmware_hash, MICROPY_GIT_HASH, sizeof(ramdisk.data_map.stored_firmware_hash));
+    memcpy(ramdisk.data_map.program_data, _program_data, sizeof(_program_data));
 }
 
 // Don't store any data in this implementation.

lib/pbio/drv/block_device/block_device_w25qxx_stm32.c

@@ -13,9 +13,8 @@
 
 #include STM32_HAL_H
 
-#include <contiki.h>
-
 #include "../core.h"
+#include "../sys/storage_data.h"
 #include "block_device_w25qxx_stm32.h"
 
 #include <pbdrv/block_device.h>
@@ -36,15 +35,19 @@ static struct {
      * portion of this, up to pbdrv_block_device_get_writable_size() bytes,
      * gets saved to flash at shutdown.
      */
-    uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    union {
+        // ensure that data is properly aligned for pbsys_storage_data_map_t
+        pbsys_storage_data_map_t data_map;
+        uint8_t data[PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE];
+    };
 } ramdisk __attribute__((section(".noinit"), used));
 
 uint32_t pbdrv_block_device_get_writable_size(void) {
     return PBDRV_CONFIG_BLOCK_DEVICE_W25QXX_STM32_SIZE - sizeof(ramdisk.saved_size);
 }
 
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
-    *data = ramdisk.data;
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
+    *data = &ramdisk.data_map;
 
     // Higher level code can use the ramdisk data if initialization completed
     // successfully. Otherwise it should reset to factory default data.

lib/pbio/include/pbdrv/block_device.h

@@ -11,9 +11,10 @@
 
 #include <stdint.h>
 
+#include "../sys/storage_data.h"
+
 #include <pbdrv/config.h>
 #include <pbio/error.h>
-
 #include <pbio/os.h>
 
 #if PBDRV_CONFIG_BLOCK_DEVICE
@@ -28,7 +29,7 @@
  *                      ::PBIO_ERROR_IO (driver-specific error), though boot
  *                        does not proceed if this happens.
  */
-pbio_error_t pbdrv_block_device_get_data(uint8_t **data);
+pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data);
 
 /**
  * Writes the "RAM Disk" to storage. May erase entire disk prior to writing.
@@ -57,7 +58,7 @@ uint32_t pbdrv_block_device_get_writable_size(void);
 
 #else
 
-static inline pbio_error_t pbdrv_block_device_get_data(uint8_t **data) {
+static inline pbio_error_t pbdrv_block_device_get_data(pbsys_storage_data_map_t **data) {
     return PBIO_ERROR_NOT_SUPPORTED;
 }
 

lib/pbio/platform/nxt/pbdrvconfig.h

@@ -11,8 +11,6 @@
 #define PBDRV_CONFIG_BLOCK_DEVICE                   (1)
 #define PBDRV_CONFIG_BLOCK_DEVICE_RAM_SIZE          (10 * 1024)
 #define PBDRV_CONFIG_BLOCK_DEVICE_TEST              (1)
-#define PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE         (8 * 1024)
-#define PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE_USER    (512)
 
 #define PBDRV_CONFIG_BUTTON                         (1)
 #define PBDRV_CONFIG_BUTTON_NXT                     (1)

lib/pbio/platform/nxt/pbsysconfig.h

@@ -11,7 +11,7 @@
 #define PBSYS_CONFIG_MAIN                           (1)
 #define PBSYS_CONFIG_STORAGE                        (1)
 #define PBSYS_CONFIG_STORAGE_NUM_SLOTS              (1)
-#define PBSYS_CONFIG_STORAGE_USER_DATA_SIZE         (PBDRV_CONFIG_BLOCK_DEVICE_TEST_SIZE_USER)
+#define PBSYS_CONFIG_STORAGE_USER_DATA_SIZE         (512)
 #define PBSYS_CONFIG_STATUS_LIGHT                   (0)
 #define PBSYS_CONFIG_STATUS_LIGHT_BATTERY           (0)
 #define PBSYS_CONFIG_STATUS_LIGHT_BLUETOOTH         (0)

lib/pbio/sys/storage.c

@@ -22,62 +22,11 @@
 #include "core.h"
 #include "hmi.h"
 
-/**
- * Information about one code slot.
- *
- * A size of 0 means that this slot is not used. The offset indicates where
- * the program is stored in user storage.
- *
- * Code slots are *not* stored chronologically by slot id. Instead they are
- * stored consecutively as they are received, with the newest program last.
- *
- * If a slot is already in use and a new program should be loaded into it,
- * it is deleted by mem-moving any subsequent programs into its place, and
- * appending the new program to be last again. Since a user is typically only
- * iterating code in one slot, this is therefore usually the last stored
- * program. This means memmoves happen very little, only when needed.
- *
- */
-typedef struct {
-    uint32_t offset;
-    uint32_t size;
-} pbsys_storage_slot_info_t;
-
 /**
  * Slot at which incoming program data is currently being received.
  */
 static uint8_t incoming_slot = 0;
 
-/**
- * Map of loaded data. All data types are little-endian.
- */
-typedef struct {
-    /**
-     * End-user read-write accessible data. Everything after this is also
-     * user-readable but not writable.
-     */
-    uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE];
-    /**
-     * First 8 symbols of the git hash of the firmware version used to create
-     * this data map. If this does not match the version of the running
-     * firmware, user data will be reset to 0.
-     */
-    char stored_firmware_hash[8];
-    /**
-     * System settings. Settings will be reset to defaults when the firmware
-     * version changes due to an update.
-     */
-    pbsys_storage_settings_t settings;
-    /**
-     * Size and offset info for each slot.
-     */
-    pbsys_storage_slot_info_t slot_info[PBSYS_CONFIG_STORAGE_NUM_SLOTS];
-    /**
-     * Data of the application program (code + heap).
-     */
-    uint8_t program_data[] __attribute__((aligned(sizeof(void *))));
-} pbsys_storage_data_map_t;
-
 /**
  * Map of loaded data. This is kept in memory between successive runs of the
  * end user application (like MicroPython).
@@ -399,7 +348,7 @@ static pbio_error_t pbsys_storage_deinit_process_thread(pbio_os_state_t *state,
  */
 void pbsys_storage_init(void) {
 
-    pbio_error_t err = pbdrv_block_device_get_data((uint8_t **)&map);
+    pbio_error_t err = pbdrv_block_device_get_data(&map);
 
     // Test that storage successfully loaded and matches current firmware,
     // otherwise reset storage.

lib/pbio/sys/storage_data.h

@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2025 The Pybricks Authors
+
+// Data structures for non-volatile storage.
+
+// NB: Normally, pbsys code isn't referenced by drivers, however this is a
+// special case where the block device driver needs to have the actual struct
+// definition in order to ensure correct memory layout and alignment.
+
+#ifndef _PBSYS_SYS_STORAGE_DATA_H_
+#define _PBSYS_SYS_STORAGE_DATA_H_
+
+#include <stdint.h>
+
+#include <pbsys/config.h>
+#include <pbsys/storage_settings.h>
+
+/**
+ * Information about one code slot.
+ *
+ * A size of 0 means that this slot is not used. The offset indicates where
+ * the program is stored in user storage.
+ *
+ * Code slots are *not* stored chronologically by slot id. Instead they are
+ * stored consecutively as they are received, with the newest program last.
+ *
+ * If a slot is already in use and a new program should be loaded into it,
+ * it is deleted by mem-moving any subsequent programs into its place, and
+ * appending the new program to be last again. Since a user is typically only
+ * iterating code in one slot, this is therefore usually the last stored
+ * program. This means memmoves happen very little, only when needed.
+ *
+ */
+typedef struct {
+    uint32_t offset;
+    uint32_t size;
+} pbsys_storage_slot_info_t;
+
+/**
+ * Map of loaded data. All data types are little-endian.
+ */
+typedef struct {
+    /**
+     * End-user read-write accessible data. Everything after this is also
+     * user-readable but not writable.
+     */
+    uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE];
+    /**
+     * First 8 symbols of the git hash of the firmware version used to create
+     * this data map. If this does not match the version of the running
+     * firmware, user data will be reset to 0.
+     */
+    char stored_firmware_hash[8];
+    /**
+     * System settings. Settings will be reset to defaults when the firmware
+     * version changes due to an update.
+     */
+    pbsys_storage_settings_t settings;
+    /**
+     * Size and offset info for each slot.
+     */
+    pbsys_storage_slot_info_t slot_info[PBSYS_CONFIG_STORAGE_NUM_SLOTS];
+    /**
+     * Data of the application program (code + heap).
+     */
+    uint8_t program_data[] __attribute__((aligned(sizeof(void *))));
+} pbsys_storage_data_map_t;
+
+#endif // _PBSYS_SYS_STORAGE_DATA_H_