refactor(display): Output/layer/battery thread safety.

* Submit widget updates to display queue.
* Use mutex to control access to shared state for widgets.

app/include/zmk/display.h

@@ -4,8 +4,57 @@
  * SPDX-License-Identifier: MIT
  */
 
+/** @file display.h
+ *  @brief Display functions and macros.
+ */
+
 #pragma once
 
 struct k_work_q *zmk_display_work_q();
 
-int zmk_display_init();
+bool zmk_display_is_initialized();
+int zmk_display_init();
+
+/**
+ * @brief Macro to define a ZMK event listener that handles the thread safety of fetching
+ * the necessary state from the system work queue context, invoking a work callback
+ * in the display queue context, and properly accessing that state safely when performing
+ * display/LVGL updates.
+ *
+ * @param listener THe ZMK Event manager listener name.
+ * @param state_type The struct/enum type used to store/transfer state.
+ * @param cb The callback to invoke in the dispaly queue context to update the UI. Should be `void
+ * func(state_type)` signature.
+ * @param state_func The callback function to invoke to fetch the updated state from ZMK core.
+ * Should be `state type func(const zmk_event_t *eh)` signature.
+ * @retval listner##_init Generates a function `listener##_init` that should be called by the widget
+ * once ready to be updated.
+ **/
+#define ZMK_DISPLAY_WIDGET_LISTENER(listener, state_type, cb, state_func)                          \
+    K_MUTEX_DEFINE(listener##_mutex);                                                              \
+    static state_type __##listener##_state;                                                        \
+    static state_type listener##_get_local_state() {                                               \
+        k_mutex_lock(&listener##_mutex, K_FOREVER);                                                \
+        state_type copy = __##listener##_state;                                                    \
+        k_mutex_unlock(&listener##_mutex);                                                         \
+        return copy;                                                                               \
+    };                                                                                             \
+    static void listener##_work_cb(struct k_work *work) { cb(listener##_get_local_state()); };     \
+    K_WORK_DEFINE(listener##_work, listener##_work_cb);                                            \
+    static void listener##_refresh_state(const zmk_event_t *eh) {                                  \
+        k_mutex_lock(&listener##_mutex, K_FOREVER);                                                \
+        __##listener##_state = state_func(eh);                                                     \
+        k_mutex_unlock(&listener##_mutex);                                                         \
+    };                                                                                             \
+    static void listener##_init() {                                                                \
+        listener##_refresh_state(NULL);                                                            \
+        listener##_work_cb(NULL);                                                                  \
+    }                                                                                              \
+    static int listener##_cb(const zmk_event_t *eh) {                                              \
+        if (zmk_display_is_initialized()) {                                                        \
+            listener##_refresh_state(eh);                                                          \
+            k_work_submit_to_queue(zmk_display_work_q(), &listener##_work);                        \
+        }                                                                                          \
+        return ZMK_EV_EVENT_BUBBLE;                                                                \
+    }                                                                                              \
+    ZMK_LISTENER(listener, listener##_cb);

app/src/display/main.c

@@ -22,6 +22,7 @@ LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);
 #define ZMK_DISPLAY_NAME CONFIG_LVGL_DISPLAY_DEV_NAME
 
 static const struct device *display;
+static bool initialized = false;
 
 static lv_obj_t *screen;
 
@@ -82,6 +83,8 @@ static void stop_display_updates() {
     k_timer_stop(&display_timer);
 }
 
+int zmk_display_is_initialized() { return initialized; }
+
 int zmk_display_init() {
     LOG_DBG("");
 
@@ -108,6 +111,8 @@ int zmk_display_init() {
 
     start_display_updates();
 
+    initialized = true;
+
     LOG_DBG("");
     return 0;
 }

app/src/display/widgets/battery_status.c

@@ -4,11 +4,13 @@
  * SPDX-License-Identifier: MIT
  */
 
+#include <kernel.h>
 #include <bluetooth/services/bas.h>
 
 #include <logging/log.h>
 LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);
 
+#include <zmk/display.h>
 #include <zmk/display/widgets/battery_status.h>
 #include <zmk/usb.h>
 #include <zmk/events/usb_conn_state_changed.h>
@@ -33,12 +35,20 @@ void battery_status_init() {
     lv_style_set_text_line_space(&label_style, LV_STATE_DEFAULT, 1);
 }
 
-void set_battery_symbol(lv_obj_t *label) {
+struct battery_status_state {
+    uint8_t level;
+#if IS_ENABLED(CONFIG_USB)
+    bool usb_present;
+#endif
+};
+
+static void set_battery_symbol(lv_obj_t *label, struct battery_status_state state) {
     char text[2] = "  ";
-    uint8_t level = bt_bas_get_battery_level();
+
+    uint8_t level = state.level;
 
 #if IS_ENABLED(CONFIG_USB)
-    if (zmk_usb_is_powered()) {
+    if (state.usb_present) {
         strcpy(text, LV_SYMBOL_CHARGE);
     }
 #endif /* IS_ENABLED(CONFIG_USB) */
@@ -57,32 +67,41 @@ void set_battery_symbol(lv_obj_t *label) {
     lv_label_set_text(label, text);
 }
 
+void battery_status_update_cb(struct battery_status_state state) {
+    struct zmk_widget_battery_status *widget;
+    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_battery_symbol(widget->obj, state); }
+}
+
+static struct battery_status_state battery_status_get_state(const zmk_event_t *eh) {
+    return (struct battery_status_state) {
+        .level = bt_bas_get_battery_level(),
+#if IS_ENABLED(CONFIG_USB)
+        .usb_present = zmk_usb_is_powered(),
+#endif /* IS_ENABLED(CONFIG_USB) */
+    };
+}
+
+ZMK_DISPLAY_WIDGET_LISTENER(widget_battery_status, struct battery_status_state,
+                            battery_status_update_cb, battery_status_get_state)
+
+ZMK_SUBSCRIPTION(widget_battery_status, zmk_battery_state_changed);
+#if IS_ENABLED(CONFIG_USB)
+ZMK_SUBSCRIPTION(widget_battery_status, zmk_usb_conn_state_changed);
+#endif /* IS_ENABLED(CONFIG_USB) */
+
 int zmk_widget_battery_status_init(struct zmk_widget_battery_status *widget, lv_obj_t *parent) {
     battery_status_init();
     widget->obj = lv_label_create(parent, NULL);
     lv_obj_add_style(widget->obj, LV_LABEL_PART_MAIN, &label_style);
 
     lv_obj_set_size(widget->obj, 40, 15);
-    set_battery_symbol(widget->obj);
 
     sys_slist_append(&widgets, &widget->node);
 
+    widget_battery_status_init();
     return 0;
 }
 
 lv_obj_t *zmk_widget_battery_status_obj(struct zmk_widget_battery_status *widget) {
-    LOG_DBG("Label: %p", widget->obj);
     return widget->obj;
 }
-
-int battery_status_listener(const zmk_event_t *eh) {
-    struct zmk_widget_battery_status *widget;
-    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_battery_symbol(widget->obj); }
-    return ZMK_EV_EVENT_BUBBLE;
-}
-
-ZMK_LISTENER(widget_battery_status, battery_status_listener)
-ZMK_SUBSCRIPTION(widget_battery_status, zmk_battery_state_changed);
-#if IS_ENABLED(CONFIG_USB)
-ZMK_SUBSCRIPTION(widget_battery_status, zmk_usb_conn_state_changed);
-#endif /* IS_ENABLED(CONFIG_USB) */

app/src/display/widgets/layer_status.c

@@ -4,9 +4,11 @@
  * SPDX-License-Identifier: MIT
  */
 
+#include <kernel.h>
 #include <logging/log.h>
 LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);
 
+#include <zmk/display.h>
 #include <zmk/display/widgets/layer_status.h>
 #include <zmk/events/layer_state_changed.h>
 #include <zmk/event_manager.h>
@@ -18,7 +20,12 @@ static lv_style_t label_style;
 
 static bool style_initialized = false;
 
-void layer_status_init() {
+struct layer_status_state {
+    uint8_t index;
+    const char *label;
+};
+
+static void layer_status_init() {
     if (style_initialized) {
         return;
     }
@@ -31,49 +38,50 @@ void layer_status_init() {
     lv_style_set_text_line_space(&label_style, LV_STATE_DEFAULT, 1);
 }
 
-void set_layer_symbol(lv_obj_t *label) {
-    int active_layer_index = zmk_keymap_highest_layer_active();
-
-    LOG_DBG("Layer changed to %i", active_layer_index);
-
-    const char *layer_label = zmk_keymap_layer_label(active_layer_index);
-    if (layer_label == NULL) {
+static void set_layer_symbol(lv_obj_t *label, struct layer_status_state state) {
+    if (state.label == NULL) {
         char text[6] = {};
 
-        sprintf(text, LV_SYMBOL_KEYBOARD "%i", active_layer_index);
+        sprintf(text, LV_SYMBOL_KEYBOARD "%i", state.index);
 
         lv_label_set_text(label, text);
     } else {
         char text[12] = {};
 
-        snprintf(text, 12, LV_SYMBOL_KEYBOARD "%s", layer_label);
+        snprintf(text, 12, LV_SYMBOL_KEYBOARD "%s", state.label);
 
         lv_label_set_text(label, text);
     }
 }
 
+static void layer_status_update_cb(struct layer_status_state state) {
+    struct zmk_widget_layer_status *widget;
+    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_layer_symbol(widget->obj, state); }
+}
+
+static struct layer_status_state layer_status_get_state(const zmk_event_t *eh) {
+    uint8_t index = zmk_keymap_highest_layer_active();
+    return (struct layer_status_state){.index = index, .label = zmk_keymap_layer_label(index)};
+}
+
+ZMK_DISPLAY_WIDGET_LISTENER(widget_layer_status, struct layer_status_state, layer_status_update_cb,
+                            layer_status_get_state)
+
+ZMK_SUBSCRIPTION(widget_layer_status, zmk_layer_state_changed);
+
 int zmk_widget_layer_status_init(struct zmk_widget_layer_status *widget, lv_obj_t *parent) {
     layer_status_init();
     widget->obj = lv_label_create(parent, NULL);
     lv_obj_add_style(widget->obj, LV_LABEL_PART_MAIN, &label_style);
 
     lv_obj_set_size(widget->obj, 40, 15);
-    set_layer_symbol(widget->obj);
 
     sys_slist_append(&widgets, &widget->node);
 
+    widget_layer_status_init();
     return 0;
 }
 
 lv_obj_t *zmk_widget_layer_status_obj(struct zmk_widget_layer_status *widget) {
     return widget->obj;
-}
-
-int layer_status_listener(const zmk_event_t *eh) {
-    struct zmk_widget_layer_status *widget;
-    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_layer_symbol(widget->obj); }
-    return 0;
-}
-
-ZMK_LISTENER(widget_layer_status, layer_status_listener)
-ZMK_SUBSCRIPTION(widget_layer_status, zmk_layer_state_changed);
+}

app/src/display/widgets/output_status.c

@@ -4,11 +4,13 @@
  * SPDX-License-Identifier: MIT
  */
 
+#include <kernel.h>
 #include <bluetooth/services/bas.h>
 
 #include <logging/log.h>
 LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);
 
+#include <zmk/display.h>
 #include <zmk/display/widgets/output_status.h>
 #include <zmk/event_manager.h>
 #include <zmk/events/usb_conn_state_changed.h>
@@ -23,7 +25,14 @@ static lv_style_t label_style;
 
 static bool style_initialized = false;
 
-void output_status_init() {
+struct output_status_state {
+    enum zmk_endpoint selected_endpoint;
+    bool active_profile_connected;
+    bool active_profile_bonded;
+    uint8_t active_profile_index;
+};
+
+static void output_status_init() {
     if (style_initialized) {
         return;
     }
@@ -36,26 +45,34 @@ void output_status_init() {
     lv_style_set_text_line_space(&label_style, LV_STATE_DEFAULT, 1);
 }
 
-void set_status_symbol(lv_obj_t *label) {
-    enum zmk_endpoint selected_endpoint = zmk_endpoints_selected();
-    bool active_profile_connected = zmk_ble_active_profile_is_connected();
-    bool active_profie_bonded = !zmk_ble_active_profile_is_open();
-    uint8_t active_profile_index = zmk_ble_active_profile_index();
+static struct output_status_state get_state(const zmk_event_t *_eh) {
+    return (struct output_status_state){.selected_endpoint = zmk_endpoints_selected(),
+                                        .active_profile_connected =
+                                            zmk_ble_active_profile_is_connected(),
+                                        .active_profile_bonded = !zmk_ble_active_profile_is_open(),
+                                        .active_profile_index = zmk_ble_active_profile_index()};
+    ;
+}
+
+static void set_status_symbol(lv_obj_t *label, struct output_status_state state) {
     char text[6] = {};
 
-    switch (selected_endpoint) {
+    switch (state.selected_endpoint) {
     case ZMK_ENDPOINT_USB:
         strcat(text, LV_SYMBOL_USB "   ");
         break;
     case ZMK_ENDPOINT_BLE:
-        if (active_profie_bonded) {
-            if (active_profile_connected) {
-                sprintf(text, LV_SYMBOL_WIFI "%i " LV_SYMBOL_OK, active_profile_index);
+        if (state.active_profile_bonded) {
+            if (state.active_profile_connected) {
+                snprintf(text, sizeof(text), LV_SYMBOL_WIFI "%i " LV_SYMBOL_OK,
+                         state.active_profile_index);
             } else {
-                sprintf(text, LV_SYMBOL_WIFI "%i " LV_SYMBOL_CLOSE, active_profile_index);
+                snprintf(text, sizeof(text), LV_SYMBOL_WIFI "%i " LV_SYMBOL_CLOSE,
+                         state.active_profile_index);
             }
         } else {
-            sprintf(text, LV_SYMBOL_WIFI "%i " LV_SYMBOL_SETTINGS, active_profile_index);
+            snprintf(text, sizeof(text), LV_SYMBOL_WIFI "%i " LV_SYMBOL_SETTINGS,
+                     state.active_profile_index);
         }
         break;
     }
@@ -63,35 +80,36 @@ void set_status_symbol(lv_obj_t *label) {
     lv_label_set_text(label, text);
 }
 
+static void output_status_update_cb(struct output_status_state state) {
+    struct zmk_widget_output_status *widget;
+    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_status_symbol(widget->obj, state); }
+}
+
+ZMK_DISPLAY_WIDGET_LISTENER(widget_output_status, struct output_status_state,
+                            output_status_update_cb, get_state)
+ZMK_SUBSCRIPTION(widget_output_status, zmk_endpoint_selection_changed);
+
+#if defined(CONFIG_USB)
+ZMK_SUBSCRIPTION(widget_output_status, zmk_usb_conn_state_changed);
+#endif
+#if defined(CONFIG_ZMK_BLE)
+ZMK_SUBSCRIPTION(widget_output_status, zmk_ble_active_profile_changed);
+#endif
+
 int zmk_widget_output_status_init(struct zmk_widget_output_status *widget, lv_obj_t *parent) {
     output_status_init();
+
     widget->obj = lv_label_create(parent, NULL);
     lv_obj_add_style(widget->obj, LV_LABEL_PART_MAIN, &label_style);
 
     lv_obj_set_size(widget->obj, 40, 15);
-    set_status_symbol(widget->obj);
 
     sys_slist_append(&widgets, &widget->node);
 
+    widget_output_status_init();
     return 0;
 }
 
 lv_obj_t *zmk_widget_output_status_obj(struct zmk_widget_output_status *widget) {
     return widget->obj;
 }
-
-int output_status_listener(const zmk_event_t *eh) {
-    struct zmk_widget_output_status *widget;
-    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_status_symbol(widget->obj); }
-    return ZMK_EV_EVENT_BUBBLE;
-}
-
-ZMK_LISTENER(widget_output_status, output_status_listener)
-ZMK_SUBSCRIPTION(widget_output_status, zmk_endpoint_selection_changed);
-
-#if defined(CONFIG_USB)
-ZMK_SUBSCRIPTION(widget_output_status, zmk_usb_conn_state_changed);
-#endif
-#if defined(CONFIG_ZMK_BLE)
-ZMK_SUBSCRIPTION(widget_output_status, zmk_ble_active_profile_changed);
-#endif

app/src/display/widgets/wpm_status.c

@@ -7,6 +7,7 @@
 #include <logging/log.h>
 LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);
 
+#include <zmk/display.h>
 #include <zmk/display/widgets/wpm_status.h>
 #include <zmk/events/wpm_state_changed.h>
 #include <zmk/event_manager.h>
@@ -18,6 +19,10 @@ static lv_style_t label_style;
 
 static bool style_initialized = false;
 
+struct wpm_status_state {
+    uint8_t wpm;
+};
+
 void wpm_status_init() {
     if (style_initialized) {
         return;
@@ -31,37 +36,41 @@ void wpm_status_init() {
     lv_style_set_text_line_space(&label_style, LV_STATE_DEFAULT, 1);
 }
 
-void set_wpm_symbol(lv_obj_t *label, int wpm) {
+struct wpm_status_state wpm_status_get_state(const zmk_event_t *eh) {
+    return (struct wpm_status_state){.wpm = zmk_wpm_get_state()};
+};
+
+void set_wpm_symbol(lv_obj_t *label, struct wpm_status_state state) {
     char text[4] = {};
 
-    LOG_DBG("WPM changed to %i", wpm);
-    sprintf(text, "%i ", wpm);
+    LOG_DBG("WPM changed to %i", state.wpm);
+    snprintf(text, sizeof(text), "%i ", state.wpm);
 
     lv_label_set_text(label, text);
 }
 
+void wpm_status_update_cb(struct wpm_status_state state) {
+    struct zmk_widget_wpm_status *widget;
+    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_wpm_symbol(widget->obj, state); }
+}
+
+ZMK_DISPLAY_WIDGET_LISTENER(widget_wpm_status, struct wpm_status_state, wpm_status_update_cb,
+                            wpm_status_get_state)
+ZMK_SUBSCRIPTION(widget_wpm_status, zmk_wpm_state_changed);
+
 int zmk_widget_wpm_status_init(struct zmk_widget_wpm_status *widget, lv_obj_t *parent) {
     wpm_status_init();
+
     widget->obj = lv_label_create(parent, NULL);
     lv_obj_add_style(widget->obj, LV_LABEL_PART_MAIN, &label_style);
     lv_label_set_align(widget->obj, LV_LABEL_ALIGN_RIGHT);
 
     lv_obj_set_size(widget->obj, 40, 15);
-    set_wpm_symbol(widget->obj, 0);
 
     sys_slist_append(&widgets, &widget->node);
 
+    widget_wpm_status_init();
     return 0;
 }
 
 lv_obj_t *zmk_widget_wpm_status_obj(struct zmk_widget_wpm_status *widget) { return widget->obj; }
-
-int wpm_status_listener(const zmk_event_t *eh) {
-    struct zmk_wpm_state_changed *ev = as_zmk_wpm_state_changed(eh);
-    struct zmk_widget_wpm_status *widget;
-    SYS_SLIST_FOR_EACH_CONTAINER(&widgets, widget, node) { set_wpm_symbol(widget->obj, ev->state); }
-    return 0;
-}
-
-ZMK_LISTENER(widget_wpm_status, wpm_status_listener)
-ZMK_SUBSCRIPTION(widget_wpm_status, zmk_wpm_state_changed);