pbio/sys/storage: Split settings to dedicated module.

Author: laurensvalk

bricks/_common/sources.mk

@@ -233,6 +233,7 @@ PBIO_SRC_C = $(addprefix lib/pbio/,\
 	sys/program_stop.c \
 	sys/status.c \
 	sys/storage.c \
+	sys/storage_settings.c \
 	sys/supervisor.c \
 	sys/user_program.c \
 	)

lib/pbio/include/pbsys/bluetooth.h

@@ -32,8 +32,6 @@ uint32_t pbsys_bluetooth_rx_get_available(void);
 pbio_error_t pbsys_bluetooth_rx(uint8_t *data, uint32_t *size);
 pbio_error_t pbsys_bluetooth_tx(const uint8_t *data, uint32_t *size);
 bool pbsys_bluetooth_tx_is_idle(void);
-bool pbsys_bluetooth_is_user_enabled(void);
-void pbsys_bluetooth_is_user_enabled_request_toggle(void);
 
 #else // PBSYS_CONFIG_BLUETOOTH
 
@@ -51,11 +49,6 @@ static inline pbio_error_t pbsys_bluetooth_tx(const uint8_t *data, uint32_t *siz
 static inline bool pbsys_bluetooth_tx_is_idle(void) {
     return false;
 }
-static inline void pbsys_bluetooth_is_user_enabled_request_toggle(void) {
-}
-static inline bool pbsys_bluetooth_is_user_enabled(void) {
-    return true;
-}
 
 #endif // PBSYS_CONFIG_BLUETOOTH
 

lib/pbio/include/pbsys/storage.h

@@ -1,10 +1,10 @@
 // SPDX-License-Identifier: MIT
-// Copyright (c) 2022 The Pybricks Authors
+// Copyright (c) 2022-2024 The Pybricks Authors
 
 /**
- * @addtogroup SysProgramLoad System: Load user programs.
+ * @addtogroup SysStorage System: Load user programs, data, and settings.
  *
- * Configuration for loading data.
+ * Load user programs, data, and settings.
  *
  * @{
  */
@@ -15,16 +15,7 @@
 #include <stdint.h>
 
 #include <pbsys/config.h>
-
-/**
- * System settings. All data types are little-endian.
- */
-typedef struct _pbsys_storage_settings_t {
-    /**
-     * User has enabled Bluetooth Low Energy.
-     */
-    bool bluetooth_ble_user_enabled : 1;
-} pbsys_storage_settings_t;
+#include <pbsys/storage_settings.h>
 
 #if PBSYS_CONFIG_STORAGE
 
@@ -82,31 +73,18 @@ pbio_error_t pbsys_storage_set_user_data(uint32_t offset, const uint8_t *data, u
 
 pbio_error_t pbsys_storage_get_user_data(uint32_t offset, uint8_t **data, uint32_t size);
 
-pbio_error_t pbsys_storage_get_settings(pbsys_storage_settings_t **settings);
-
-void pbsys_storage_request_write(void);
-
 #else
 
 #define PBSYS_STORAGE_MAX_PROGRAM_SIZE (0)
 
 static inline pbio_error_t pbsys_storage_set_user_data(uint32_t offset, const uint8_t *data, uint32_t size) {
     return PBIO_ERROR_NOT_SUPPORTED;
 }
-
 static inline pbio_error_t pbsys_storage_get_user_data(uint32_t offset, uint8_t **data, uint32_t size) {
     *data = NULL;
     return PBIO_ERROR_NOT_SUPPORTED;
 }
 
-static inline pbio_error_t pbsys_storage_get_settings(pbsys_storage_settings_t **settings) {
-    *settings = NULL;
-    return PBIO_ERROR_NOT_SUPPORTED;
-}
-
-static inline void pbsys_storage_request_write(void) {
-}
-
 #endif // PBSYS_CONFIG_STORAGE
 
 #endif // _PBSYS_STORAGE_H_

lib/pbio/include/pbsys/storage_settings.h

@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2024 The Pybricks Authors
+
+/**
+ * @addtogroup SysStorageSettings System: Load user settings.
+ *
+ * Interface for reading and storing user system settings.
+ *
+ * @{
+ */
+
+#ifndef _PBSYS_STORAGE_SETTINGS_H_
+#define _PBSYS_STORAGE_SETTINGS_H_
+
+#include <stdint.h>
+
+#include <pbio/error.h>
+
+#include <pbsys/config.h>
+
+/**
+ * System settings. All data types are little-endian.
+ */
+typedef struct _pbsys_storage_settings_t {
+    #if PBSYS_CONFIG_BLUETOOTH_TOGGLE
+    /**
+     * User has enabled Bluetooth Low Energy.
+     */
+    bool bluetooth_ble_user_enabled : 1;
+    #endif // PBSYS_CONFIG_BLUETOOTH_TOGGLE
+} pbsys_storage_settings_t;
+
+#if PBSYS_CONFIG_STORAGE
+
+void pbsys_storage_set_default_settings(pbsys_storage_settings_t *settings);
+
+bool pbsys_storage_settings_bluetooth_enabled(void);
+
+void pbsys_storage_settings_bluetooth_enabled_request_toggle(void);
+
+#else
+
+static inline void pbsys_storage_set_default_settings(pbsys_storage_settings_t *settings) {
+}
+static inline bool pbsys_storage_settings_bluetooth_enabled(void) {
+    return true;
+}
+static inline void pbsys_storage_settings_bluetooth_enabled_request_toggle(void) {
+}
+
+#endif // PBSYS_CONFIG_STORAGE
+
+#endif // _PBSYS_STORAGE_SETTINGS_H_
+
+/** @} */

lib/pbio/sys/bluetooth.c

@@ -24,6 +24,7 @@
 #include <pbsys/storage.h>
 
 #include "light.h"
+#include "storage.h"
 
 // REVISIT: this can be the negotiated MTU - 3 to allow for better throughput
 #define MAX_CHAR_SIZE 20
@@ -46,7 +47,7 @@ static bool send_busy;
 
 PROCESS(pbsys_bluetooth_process, "Bluetooth");
 
-static void pbsys_bluetooth_process_poll(void) {
+void pbsys_bluetooth_process_poll(void) {
     process_poll(&pbsys_bluetooth_process);
 }
 
@@ -203,45 +204,6 @@ bool pbsys_bluetooth_tx_is_idle(void) {
     return !send_busy && lwrb_get_full(&stdout_ring_buf) == 0;
 }
 
-#if PBSYS_CONFIG_STORAGE && PBSYS_CONFIG_BLUETOOTH_TOGGLE
-static pbsys_storage_settings_t *settings;
-
-bool pbsys_bluetooth_is_user_enabled(void) {
-    if (!settings) {
-        return false;
-    }
-    return settings->bluetooth_ble_user_enabled;
-}
-
-void pbsys_bluetooth_is_user_enabled_request_toggle(void) {
-
-    // Ignore toggle request in all but idle system status.
-    if (pbsys_status_test(PBIO_PYBRICKS_STATUS_USER_PROGRAM_RUNNING)
-        || pbsys_status_test(PBIO_PYBRICKS_STATUS_POWER_BUTTON_PRESSED)
-        || pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN)
-        || pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN_REQUEST)
-        // Ignore toggle is Bluetooth is currently being used in a connection.
-        || pbdrv_bluetooth_is_connected(PBDRV_BLUETOOTH_CONNECTION_LE)
-        || pbdrv_bluetooth_is_connected(PBDRV_BLUETOOTH_CONNECTION_PERIPHERAL)
-        // Ignore if last request not yet finished processing.
-        || settings->bluetooth_ble_user_enabled != pbdrv_bluetooth_is_ready()
-        ) {
-        return;
-    }
-
-    // Toggle the user enabled state and poll process to take action.
-    settings->bluetooth_ble_user_enabled = !settings->bluetooth_ble_user_enabled;
-    pbsys_storage_request_write();
-    pbsys_bluetooth_process_poll();
-}
-#else // PBSYS_CONFIG_STORAGE && PBSYS_CONFIG_BLUETOOTH_TOGGLE
-bool pbsys_bluetooth_is_user_enabled(void) {
-    return true;
-}
-void pbsys_bluetooth_is_user_enabled_request_toggle(void) {
-}
-#endif // PBSYS_CONFIG_STORAGE && PBSYS_CONFIG_BLUETOOTH_TOGGLE
-
 // Contiki process
 
 static pbio_pybricks_error_t handle_receive(pbdrv_bluetooth_connection_t connection, const uint8_t *data, uint32_t size) {
@@ -329,8 +291,9 @@ PROCESS_THREAD(pbsys_bluetooth_process, ev, data) {
 
     PROCESS_BEGIN();
 
+    // Ensures Bluetooth preferences are loaded before we read them.
     #if PBSYS_CONFIG_STORAGE && PBSYS_CONFIG_BLUETOOTH_TOGGLE
-    PROCESS_WAIT_EVENT_UNTIL(pbsys_storage_get_settings(&settings) == PBIO_SUCCESS);
+    PROCESS_WAIT_EVENT_UNTIL(pbsys_storage_get_settings() != NULL);
     #endif // PBSYS_CONFIG_STORAGE && PBSYS_CONFIG_BLUETOOTH_TOGGLE
 
     pbdrv_bluetooth_set_on_event(pbsys_bluetooth_process_poll);
@@ -340,7 +303,7 @@ PROCESS_THREAD(pbsys_bluetooth_process, ev, data) {
 
         // Show inactive status only if user requested Bluetooth as disabled to
         // avoid always flashing red in between program runs when disconnected.
-        if (!pbsys_bluetooth_is_user_enabled()) {
+        if (!pbsys_storage_settings_bluetooth_enabled()) {
             pbsys_status_light_bluetooth_set_color(PBIO_COLOR_RED);
         }
 
@@ -349,7 +312,7 @@ PROCESS_THREAD(pbsys_bluetooth_process, ev, data) {
         PROCESS_WAIT_EVENT_UNTIL(ev == PROCESS_EVENT_TIMER && etimer_expired(&timer));
 
         // Wait until Bluetooth enabled requested by user, but stop waiting on shutdown.
-        PROCESS_WAIT_UNTIL(pbsys_bluetooth_is_user_enabled() || pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN));
+        PROCESS_WAIT_UNTIL(pbsys_storage_settings_bluetooth_enabled() || pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN));
         if (pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN)) {
             break;
         }
@@ -370,7 +333,7 @@ PROCESS_THREAD(pbsys_bluetooth_process, ev, data) {
             pbdrv_bluetooth_is_connected(PBDRV_BLUETOOTH_CONNECTION_LE)
             || pbsys_status_test(PBIO_PYBRICKS_STATUS_USER_PROGRAM_RUNNING)
             || pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN)
-            || !pbsys_bluetooth_is_user_enabled());
+            || !pbsys_storage_settings_bluetooth_enabled());
 
         // Now change the state depending on which of the above was triggered.
 
@@ -389,7 +352,7 @@ PROCESS_THREAD(pbsys_bluetooth_process, ev, data) {
         // The Bluetooth enabled flag can only change while disconnected and
         // while no program is running. So here it just serves to skip the
         // Bluetooth loop below and go directly to the disable step below it.
-        while (pbsys_bluetooth_is_user_enabled()
+        while (pbsys_storage_settings_bluetooth_enabled()
                && pbdrv_bluetooth_is_connected(PBDRV_BLUETOOTH_CONNECTION_LE)
                && !pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN)) {
 

lib/pbio/sys/bluetooth.h

@@ -8,5 +8,6 @@
 
 uint32_t pbsys_bluetooth_rx_get_free(void);
 void pbsys_bluetooth_rx_write(const uint8_t *data, uint32_t size);
+void pbsys_bluetooth_process_poll(void);
 
 #endif // _PBSYS_SYS_BLUETOOTH_H_

lib/pbio/sys/command.c

@@ -7,6 +7,8 @@
 #include <pbdrv/reset.h>
 #include <pbio/protocol.h>
 
+#include <pbsys/storage.h>
+
 #include "./bluetooth.h"
 #include "./storage.h"
 #include "./program_stop.h"

lib/pbio/sys/hmi.c

@@ -19,9 +19,9 @@
 #include <pbio/color.h>
 #include <pbio/event.h>
 #include <pbio/light.h>
-#include <pbsys/bluetooth.h>
 #include <pbsys/config.h>
 #include <pbsys/status.h>
+#include <pbsys/storage_settings.h>
 
 #include "light_matrix.h"
 #include "light.h"
@@ -81,7 +81,7 @@ static PT_THREAD(update_bluetooth_button_wait_state(bool button_pressed)) {
         // button may still be pressed during user program
         PT_WAIT_UNTIL(pt, !button_pressed);
         PT_WAIT_UNTIL(pt, button_pressed);
-        pbsys_bluetooth_is_user_enabled_request_toggle();
+        pbsys_storage_settings_bluetooth_enabled_request_toggle();
     }
 
     PT_END(pt);

lib/pbio/sys/storage.c

@@ -33,6 +33,13 @@ static pbsys_storage_data_map_t *map = &pbsys_user_ram_data_map.data_map;
 
 static bool data_map_is_loaded = false;
 
+pbsys_storage_settings_t *pbsys_storage_get_settings(void) {
+    if (!data_map_is_loaded) {
+        return NULL;
+    }
+    return &map->settings;
+}
+
 /**
  * Requests that storage (program, user data, settings) will be saved (some
  * time before shutdown). Should be called by functions that change data.
@@ -82,30 +89,6 @@ pbio_error_t pbsys_storage_get_user_data(uint32_t offset, uint8_t **data, uint32
     return PBIO_SUCCESS;
 }
 
-/**
- * Gets the stored system settings.
- *
- * @param [out] settings   The settings.
- * @returns                ::PBIO_ERROR_AGAIN if settings still being loaded.
- *                         Otherwise, ::PBIO_SUCCESS.
- */
-pbio_error_t pbsys_storage_get_settings(pbsys_storage_settings_t **settings) {
-    if (!data_map_is_loaded) {
-        return PBIO_ERROR_AGAIN;
-    }
-
-    *settings = &map->settings;
-    return PBIO_SUCCESS;
-}
-
-/**
- * Sets the default settings after an erase.
- */
-static void pbsys_storage_set_default_settings(void) {
-    map->settings.bluetooth_ble_user_enabled = true;
-    pbsys_storage_request_write();
-}
-
 #if PBSYS_CONFIG_STORAGE_OVERLAPS_BOOTLOADER_CHECKSUM
 // Updates checksum in data map to satisfy bootloader requirements.
 static void pbsys_storage_update_checksum(void) {
@@ -261,13 +244,13 @@ PROCESS_THREAD(pbsys_storage_process, ev, data) {
         // Reset storage except for program data. It is sufficient to set its
         // size to 0, which is what happens here since it is in the map.
         memset(map, 0, sizeof(pbsys_storage_data_map_t));
+        pbsys_storage_set_default_settings(&map->settings);
 
         // Set firmware version used to create current storage map.
         map->stored_firmware_version = PBIO_HEXVERSION;
 
-        // Set defaults user settings. This also raises the write flag to save
-        // on shutdown.
-        pbsys_storage_set_default_settings();
+        // Ensure new firmware version and default settings are written.
+        pbsys_storage_request_write();
     }
 
     // Poke processes that await on system settings to become available.

lib/pbio/sys/storage.h

@@ -9,6 +9,7 @@
 #include <pbio/error.h>
 #include <pbsys/config.h>
 #include <pbsys/main.h>
+#include <pbsys/storage_settings.h>
 
 #if PBSYS_CONFIG_STORAGE
 
@@ -18,12 +19,17 @@ pbio_error_t pbsys_storage_set_program_size(uint32_t size);
 pbio_error_t pbsys_storage_set_program_data(uint32_t offset, const void *data, uint32_t size);
 pbio_error_t pbsys_storage_assert_program_valid(void);
 void pbsys_storage_get_program_data(pbsys_main_program_t *program);
+pbsys_storage_settings_t *pbsys_storage_get_settings(void);
+void pbsys_storage_request_write(void);
 
 #else
 static inline void pbsys_storage_init(void) {
 }
 static inline void pbsys_storage_deinit(void) {
 }
+static inline pbsys_storage_settings_t *pbsys_storage_get_settings(void) {
+    return NULL;
+}
 static inline pbio_error_t pbsys_storage_set_program_size(uint32_t size) {
     return PBIO_ERROR_NOT_SUPPORTED;
 }
@@ -35,6 +41,8 @@ static inline pbio_error_t pbsys_storage_assert_program_valid(void) {
 }
 static inline void pbsys_storage_get_program_data(pbsys_main_program_t *program) {
 }
+static inline void pbsys_storage_request_write(void) {
+}
 
 #endif // PBSYS_CONFIG_STORAGE