display backlight and bluetooth reference counting (#639)

Documentation/ideas.md

@@ -7,11 +7,7 @@
 
 ## Higher Priority
 
-- Get rid of TactilityC in favour of TactilityKernel and kernel modules
-- Add tests for app stdin/stdout
 - CrashDiagnostics shouldn't show a QR when there's no callstack
-- Apps currently have a `Context` object with an `appInstanceId` in it, purely for being able to close the app.
-  Change it so that the app has its own termination signal that it waits for in the loop, it should subscribe to the event group.
 - stopAppFromToolbar() in Tactility.cpp stops the top-most app. Change it so the toolbar knows for which app id it is created, so it can rely on that.
 - Warn if file operations are done from prohibited tasks (e.g. lvgl task)
 - Move USB host task stacks to SPIRAM when available: esp32_usbhost*.cpp
@@ -20,8 +16,6 @@
 - Add bold fonts for e-ink readability improvement
 - Improve Setup: Show "Step done" screen
 - Improve Setup: Add keyboard/keypad navigation explanation
-- display.h API: get_backlight does not change ref counting, but it should
-- bluetooth: various getters for child devices do not change ref counting, but they should (e.g. bluetooth_hid_device_get_device())
 - Improve kernel_init.cpp (and other modules): create driver_ensure_added() and driver_ensure_destructed()
 - Drivers/audio-codec-module is not a module. Move it somewhere else. Or make it an actual module.
 - LilyGO T-Dongle S3: 1 button control, stop auto-launching web server
@@ -51,7 +45,7 @@
 - Make USB host driver disabled by default, so it doesn't consume memory
 - TactilityTool: Make API compatibility table (and check for compatibility in the tool itself)
 - Bug: Crash handling app cannot be exited with an EncoderDevice. (current work-around is to manually reset the device)
-- Move HttpServer implementation to http-module
+- Refactor HttpServer into C code and move implementation to http-module
 - Use GPS time to set/update the current time
 
 ## Lower Priority

Tactility/Source/app/boot/Boot.cpp

@@ -99,6 +99,7 @@ void setupDisplay() {
         } else {
             LOG_E(TAG, "Failed to set brightness of %s", backlight->name);
         }
+        device_put(backlight);
     } else {
         LOG_I(TAG, "No backlight for %s", display->name);
     }

Tactility/Source/app/display/Display.cpp

@@ -74,6 +74,7 @@ void onBacklightSliderEvent(lv_event_t* event) {
     ctx->displaySettings.backlightDuty = static_cast<uint8_t>(slider_value);
     ctx->displaySettingsUpdated = true;
     backlight_set_brightness(backlight, ctx->displaySettings.backlightDuty);
+    device_put(backlight);
 }
 
 void onOrientationSet(lv_event_t* event) {
@@ -184,6 +185,8 @@ void createWidgets(lv_obj_t* parent, void* userData) {
 
             lv_slider_set_value(brightness_slider, ctx->displaySettings.backlightDuty, LV_ANIM_OFF);
         }
+        // Only compared against nullptr below, never dereferenced again, so releasing it here is safe.
+        device_put(backlight);
     }
 
     // Orientation

Tactility/Source/bluetooth/Bluetooth.cpp

@@ -138,20 +138,23 @@ static void bt_event_bridge(BtEvent event) {
                             }
                         } else if (has_hid_device_auto) {
                             LOG_I(TAG, "HID device auto-start (bonded peer found)");
-                            if (Device* dev = bluetooth_hid_device_get_device()) {
+                            if (Device* dev = bluetooth_hid_device_get()) {
                                 bluetooth_hid_device_start(dev, BT_HID_DEVICE_MODE_KEYBOARD);
+                                device_put(dev);
                             }
                         } else {
                             if (settings::shouldSppAutoStart()) {
                                 LOG_I(TAG, "Auto-starting SPP server");
-                                if (Device* dev = bluetooth_serial_get_device()) {
+                                if (Device* dev = bluetooth_serial_get()) {
                                     bluetooth_serial_start(dev);
+                                    device_put(dev);
                                 }
                             }
                             if (settings::shouldMidiAutoStart()) {
                                 LOG_I(TAG, "Auto-starting MIDI server");
-                                if (Device* dev = bluetooth_midi_get_device()) {
+                                if (Device* dev = bluetooth_midi_get()) {
                                     bluetooth_midi_start(dev);
+                                    device_put(dev);
                                 }
                             }
                         }
@@ -476,18 +479,21 @@ void connect(const std::array<uint8_t, 6>& addr, int profileId) {
     if (profileId == BT_PROFILE_HID_HOST) {
         hidHostConnect(addr);
     } else if (profileId == BT_PROFILE_HID_DEVICE) {
-        if (Device* dev = bluetooth_hid_device_get_device()) {
+        if (Device* dev = bluetooth_hid_device_get()) {
             bluetooth_hid_device_start(dev, BT_HID_DEVICE_MODE_KEYBOARD);
+            device_put(dev);
         }
     } else if (profileId == BT_PROFILE_SPP) {
-        if (Device* dev = bluetooth_serial_get_device()) {
+        if (Device* dev = bluetooth_serial_get()) {
             bluetooth_serial_start(dev);
             settings::setSppAutoStart(true);
+            device_put(dev);
         }
     } else if (profileId == BT_PROFILE_MIDI) {
-        if (Device* dev = bluetooth_midi_get_device()) {
+        if (Device* dev = bluetooth_midi_get()) {
             bluetooth_midi_start(dev);
             settings::setMidiAutoStart(true);
+            device_put(dev);
         }
     }
 }
@@ -497,8 +503,9 @@ void disconnect(const std::array<uint8_t, 6>& addr, int profileId) {
     if (profileId == BT_PROFILE_HID_HOST) {
         hidHostDisconnect();
     } else if (profileId == BT_PROFILE_HID_DEVICE) {
-        if (Device* dev = bluetooth_hid_device_get_device()) {
+        if (Device* dev = bluetooth_hid_device_get()) {
             bluetooth_hid_device_stop(dev);
+            device_put(dev);
         }
     } else {
         Device* dev;

Tactility/Source/service/displayidle/DisplayIdle.cpp

@@ -38,7 +38,6 @@ static void setBacklightBrightness(uint8_t brightness) {
     if (device_get_first_active_by_type(&DISPLAY_TYPE, &display) == ERROR_NONE) {
         ::Device* backlight;
         if (display_get_backlight(display, &backlight) == ERROR_NONE) {
-            device_get(backlight);
             backlight_set_brightness(backlight, brightness);
             device_put(backlight);
         }
@@ -53,6 +52,7 @@ static bool hasDisplayWithBacklight() {
         ::Device* backlight;
         if (display_get_backlight(display, &backlight) == ERROR_NONE) {
             result = true;
+            device_put(backlight);
         }
         device_put(display);
     }

Tactility/Source/service/statusbar/Statusbar.cpp

@@ -178,10 +178,16 @@ class StatusbarService final : public Service {
             }
         }
 
-        Device* serial_dev = bluetooth_serial_get_device();
-        Device* midi_dev = bluetooth_midi_get_device();
+        Device* serial_dev = bluetooth_serial_get();
+        Device* midi_dev = bluetooth_midi_get();
         bool connected = (serial_dev && bluetooth_serial_is_connected(serial_dev)) ||
                          (midi_dev && bluetooth_midi_is_connected(midi_dev));
+        if (serial_dev) {
+            device_put(serial_dev);
+        }
+        if (midi_dev) {
+            device_put(midi_dev);
+        }
         const char* desired_icon = getBluetoothStatusIcon(radio_state, scanning, connected);
         if (bt_last_icon != desired_icon) {
             if (desired_icon != nullptr) {

TactilityKernel/include/tactility/drivers/bluetooth_hid_device.h

@@ -108,8 +108,13 @@ struct BtHidDeviceApi {
 
 extern const struct DeviceType BLUETOOTH_HID_DEVICE_TYPE;
 
-/** Find the first ready BLE HID device child device. Returns NULL if unavailable. */
-struct Device* bluetooth_hid_device_get_device(void);
+/**
+ * @brief Find the first ready BLE HID device child device.
+ * @warning On success, the returned device has an added reference (see device_get()).
+ * The caller must call device_put() on it once done.
+ * @return the device, or NULL if unavailable
+ */
+struct Device* bluetooth_hid_device_get(void);
 
 error_t bluetooth_hid_device_start(struct Device* device, enum BtHidDeviceMode mode);
 error_t bluetooth_hid_device_stop(struct Device* device);

TactilityKernel/include/tactility/drivers/bluetooth_midi.h

@@ -52,8 +52,13 @@ struct BtMidiApi {
 
 extern const struct DeviceType BLUETOOTH_MIDI_TYPE;
 
-/** Find the first ready BLE MIDI child device. Returns NULL if unavailable. */
-struct Device* bluetooth_midi_get_device(void);
+/**
+ * @brief Find the first ready BLE MIDI child device.
+ * @warning On success, the returned device has an added reference (see device_get()).
+ * The caller must call device_put() on it once done.
+ * @return the device, or NULL if unavailable
+ */
+struct Device* bluetooth_midi_get(void);
 
 error_t bluetooth_midi_start(struct Device* device);
 error_t bluetooth_midi_stop(struct Device* device);

TactilityKernel/include/tactility/drivers/bluetooth_serial.h

@@ -63,8 +63,13 @@ struct BtSerialApi {
 
 extern const struct DeviceType BLUETOOTH_SERIAL_TYPE;
 
-/** Find the first ready BLE serial child device. Returns NULL if unavailable. */
-struct Device* bluetooth_serial_get_device(void);
+/**
+ * @brief Find the first ready BLE serial child device.
+ * @warning On success, the returned device has an added reference (see device_get()).
+ * The caller must call device_put() on it once done.
+ * @return the device, or NULL if unavailable
+ */
+struct Device* bluetooth_serial_get(void);
 
 error_t bluetooth_serial_start(struct Device* device);
 error_t bluetooth_serial_stop(struct Device* device);

TactilityKernel/include/tactility/drivers/display.h

@@ -210,6 +210,8 @@ struct DisplayApi {
     /**
      * @brief Gets the backlight device associated with this display, if any.
      * @warning Function pointer should be null if capability not available.
+     * @warning Implementations must not take a reference on *backlight: display_get_backlight()
+     * adds it on their behalf, once, so drivers don't each have to remember to.
      * @param[in] device the display device
      * @param[out] backlight the associated backlight device
      * @retval ERROR_NONE when a backlight is available and *backlight was set
@@ -336,6 +338,8 @@ uint8_t display_get_frame_buffer_count(struct Device* device);
 
 /**
  * @brief Gets the backlight device associated with the specified display, if any.
+ * @warning On success, *backlight is returned with an added reference (see device_get()).
+ * The caller must call device_put() on it once done.
  * @retval ERROR_NONE when a backlight is available and *backlight was set
  * @retval ERROR_NOT_SUPPORTED when the display has no associated backlight
  */

TactilityKernel/include/tactility/module.h

@@ -16,6 +16,8 @@
 /** For an overloaded symbol (e.g. libc++/libstdc++'s float/double/long double math overloads),
  * where a bare `&symbol` is ambiguous - `type` picks the overload to take the address of. */
 #define DEFINE_MODULE_SYMBOL_SIGNATURE(symbol, type) { #symbol, (void*)(type)&symbol }
+/** Export `symbol` under a different exported `name`, for renamed symbols the loader must still resolve by their old name. */
+#define DEFINE_MODULE_SYMBOL_ALIAS(name, symbol) { name, (void*)&symbol }
 
 #ifdef __cplusplus
 extern "C" {

TactilityKernel/source/drivers/bluetooth_hid_device.cpp

@@ -10,10 +10,11 @@ const struct DeviceType BLUETOOTH_HID_DEVICE_TYPE = {
     .name = "bluetooth-hid-device",
 };
 
-struct Device* bluetooth_hid_device_get_device() {
+struct Device* bluetooth_hid_device_get() {
     struct Device* found = nullptr;
     device_for_each_of_type(&BLUETOOTH_HID_DEVICE_TYPE, &found, [](struct Device* dev, void* ctx) -> bool {
-        if (device_is_ready(dev)) {
+        // device_get() while still inside the ledger-locked callback, same as device_get_by_name().
+        if (device_is_ready(dev) && device_get(dev) == ERROR_NONE) {
             *static_cast<struct Device**>(ctx) = dev;
             return false;
         }

TactilityKernel/source/drivers/bluetooth_midi.cpp

@@ -10,10 +10,11 @@ const struct DeviceType BLUETOOTH_MIDI_TYPE = {
     .name = "bluetooth-midi",
 };
 
-struct Device* bluetooth_midi_get_device() {
+struct Device* bluetooth_midi_get() {
     struct Device* found = nullptr;
     device_for_each_of_type(&BLUETOOTH_MIDI_TYPE, &found, [](struct Device* dev, void* ctx) -> bool {
-        if (device_is_ready(dev)) {
+        // device_get() while still inside the ledger-locked callback, same as device_get_by_name().
+        if (device_is_ready(dev) && device_get(dev) == ERROR_NONE) {
             *static_cast<struct Device**>(ctx) = dev;
             return false;
         }

TactilityKernel/source/drivers/bluetooth_serial.cpp

@@ -10,10 +10,11 @@ const struct DeviceType BLUETOOTH_SERIAL_TYPE = {
     .name = "bluetooth-serial",
 };
 
-struct Device* bluetooth_serial_get_device() {
+struct Device* bluetooth_serial_get() {
     struct Device* found = nullptr;
     device_for_each_of_type(&BLUETOOTH_SERIAL_TYPE, &found, [](struct Device* dev, void* ctx) -> bool {
-        if (device_is_ready(dev)) {
+        // device_get() while still inside the ledger-locked callback, same as device_get_by_name().
+        if (device_is_ready(dev) && device_get(dev) == ERROR_NONE) {
             *static_cast<struct Device**>(ctx) = dev;
             return false;
         }

TactilityKernel/source/drivers/display.cpp

@@ -118,7 +118,16 @@ error_t display_get_backlight(Device* device, Device** backlight) {
     if (api->get_backlight == nullptr) {
         return ERROR_NOT_SUPPORTED;
     }
-    return api->get_backlight(device, backlight);
+    Device* found = nullptr;
+    error_t error = api->get_backlight(device, &found);
+    if (error != ERROR_NONE) {
+        return error;
+    }
+    error = device_get(found);
+    if (error == ERROR_NONE) {
+        *backlight = found;
+    }
+    return error;
 }
 
 const struct DeviceType DISPLAY_TYPE {

TactilityKernel/source/symbols.c

@@ -340,7 +340,8 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = {
     DEFINE_MODULE_SYMBOL(bluetooth_fire_event),
     DEFINE_MODULE_SYMBOL(BLUETOOTH_TYPE),
     // drivers/bluetooth_serial
-    DEFINE_MODULE_SYMBOL(bluetooth_serial_get_device),
+    DEFINE_MODULE_SYMBOL(bluetooth_serial_get),
+    DEFINE_MODULE_SYMBOL_ALIAS("bluetooth_serial_get_device", bluetooth_serial_get), // TODO remove in 2027
     DEFINE_MODULE_SYMBOL(bluetooth_serial_start),
     DEFINE_MODULE_SYMBOL(bluetooth_serial_stop),
     DEFINE_MODULE_SYMBOL(bluetooth_serial_write),
@@ -348,14 +349,16 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = {
     DEFINE_MODULE_SYMBOL(bluetooth_serial_is_connected),
     DEFINE_MODULE_SYMBOL(BLUETOOTH_SERIAL_TYPE),
     // drivers/bluetooth_midi
-    DEFINE_MODULE_SYMBOL(bluetooth_midi_get_device),
+    DEFINE_MODULE_SYMBOL(bluetooth_midi_get),
+    DEFINE_MODULE_SYMBOL_ALIAS("bluetooth_midi_get_device", bluetooth_midi_get), // TODO remove in 2027
     DEFINE_MODULE_SYMBOL(bluetooth_midi_start),
     DEFINE_MODULE_SYMBOL(bluetooth_midi_stop),
     DEFINE_MODULE_SYMBOL(bluetooth_midi_send),
     DEFINE_MODULE_SYMBOL(bluetooth_midi_is_connected),
     DEFINE_MODULE_SYMBOL(BLUETOOTH_MIDI_TYPE),
     // drivers/bluetooth_hid_device
-    DEFINE_MODULE_SYMBOL(bluetooth_hid_device_get_device),
+    DEFINE_MODULE_SYMBOL(bluetooth_hid_device_get),
+    DEFINE_MODULE_SYMBOL_ALIAS("bluetooth_hid_device_get_device", bluetooth_hid_device_get), // TODO remove in 2027
     DEFINE_MODULE_SYMBOL(bluetooth_hid_device_start),
     DEFINE_MODULE_SYMBOL(bluetooth_hid_device_stop),
     DEFINE_MODULE_SYMBOL(bluetooth_hid_device_send_key),