Improve I2C locking and implement I2C for TactilityC (#147)

I2C:
- Lock timeout set to reasonable times
- Check lock status in all functions
- Refactor lock/unlock to return `bool` values
- Implement functions in TactilityC
Other:
- Updated screenshots

App/CMakeLists.txt

@@ -28,7 +28,6 @@ else()
     add_executable(AppSim ${SOURCES})
     target_link_libraries(AppSim
         PRIVATE Tactility
-        PRIVATE TactilityC
         PRIVATE TactilityCore
         PRIVATE TactilityHeadless
         PRIVATE Simulator

App/Source/Main.cpp

@@ -1,12 +1,9 @@
 #include "Boards.h"
-
-// Apps
 #include "Tactility.h"
-#include "tt_init.h"
 
-namespace tt::service::wifi {
+#ifdef ESP_PLATFORM
-    extern void wifi_task(void*);
+#include "tt_init.h"
-}
+#endif
 
 extern const tt::app::AppManifest hello_world_app;
 
@@ -26,7 +23,9 @@ void app_main() {
         .autoStartAppId = nullptr
     };
 
+#ifdef ESP_PLATFORM
     tt_init_tactility_c(); // ELF bindings for side-loading on ESP32
+#endif
 
     tt::run(config);
 }

Boards/LilygoTdeck/Source/hal/TdeckKeyboard.cpp

@@ -59,7 +59,7 @@ bool TdeckKeyboard::stop() {
 }
 
 bool TdeckKeyboard::isAttached() const {
-    return tt::hal::i2c::masterCheckAddressForDevice(TDECK_KEYBOARD_I2C_BUS_HANDLE, TDECK_KEYBOARD_SLAVE_ADDRESS, 100);
+    return tt::hal::i2c::masterHasDeviceAtAddress(TDECK_KEYBOARD_I2C_BUS_HANDLE, TDECK_KEYBOARD_SLAVE_ADDRESS, 100);
 }
 
 tt::hal::Keyboard* createKeyboard() {

Documentation/ideas.md

@@ -1,17 +1,15 @@
 # Bugs
-- I2C Scanner is on M5Stack devices is broken
 - WiFi bug: when pressing disconnect while between `WIFI_EVENT_STA_START` and `IP_EVENT_STA_GOT_IP`, then auto-connect becomes active again.
 - ESP32 (CYD) memory issues (or any device without PSRAM):
   - Boot app doesn't show logo 
   - WiFi is on and navigating back to Desktop makes desktop icons disappear
-  - WiFi might fail quiety when trying to enable it: this shows no feedback (force it by increasing LVGL buffers to 100kB)
+  - WiFi might fail quietly when trying to enable it: this shows no feedback (force it by increasing LVGL buffers to 100kB)
   Possible mitigations: 
   - When no PSRAM is available, use simplified desktop buttons
   - Add statusbar icon for memory pressure.
   - Show error in WiFi screen (e.g. AlertDialog when SPI is not enabled and available memory is below a certain amount)
 - Clean up static_cast when casting to base class.
 - M5Stack CoreS3 SD card mounts, but cannot be read. There is currently a notice about it [here](https://github.com/espressif/esp-bsp/blob/master/bsp/m5stack_core_s3/README.md).
-- hal::I2c has inconsistent return values for read/write functions
 
 # TODOs
 - Call tt::lvgl::isSyncSet after HAL init and show error (and crash?) when it is not set.

Tactility/Source/app/i2cscanner/I2cScannerThread.cpp

@@ -49,7 +49,7 @@ static void onScanTimer(TT_UNUSED std::shared_ptr<void> context) {
     for (uint8_t address = 0; address < 128; ++address) {
         i2c_port_t port;
         if (getPort(data, &port)) {
-            if (hal::i2c::masterCheckAddressForDevice(port, address, 10 / portTICK_PERIOD_MS)) {
+            if (hal::i2c::masterHasDeviceAtAddress(port, address, 10 / portTICK_PERIOD_MS)) {
                 TT_LOG_I(TAG, "Found device at address %d", address);
                 if (!shouldStopScanTimer(data)) {
                     addAddressToList(data, address);

TactilityC/Source/tt_hal_i2c.cpp

@@ -0,0 +1,50 @@
+#include "tt_hal_i2c.h"
+#include "hal/i2c/I2c.h"
+
+extern "C" {
+
+bool tt_hal_i2c_start(i2c_port_t port) {
+    return tt::hal::i2c::start(port);
+}
+
+bool tt_hal_i2c_stop(i2c_port_t port) {
+    return tt::hal::i2c::stop(port);
+}
+
+bool tt_hal_i2c_is_started(i2c_port_t port) {
+    return tt::hal::i2c::isStarted(port);
+}
+
+bool tt_hal_i2c_master_read(i2c_port_t port, uint8_t address, uint8_t* data, size_t dataSize, TickType_t timeout) {
+    return tt::hal::i2c::masterRead(port, address, data, dataSize, timeout);
+}
+
+bool tt_hal_i2c_master_read_register(i2c_port_t port, uint8_t address, uint8_t reg, uint8_t* data, size_t dataSize, TickType_t timeout) {
+    return tt::hal::i2c::masterRead(port, address, reg, data, dataSize, timeout);
+}
+
+bool tt_hal_i2c_master_write(i2c_port_t port, uint16_t address, const uint8_t* data, uint16_t dataSize, TickType_t timeout) {
+    return tt::hal::i2c::masterWrite(port, address, data, dataSize, timeout);
+}
+
+bool tt_hal_i2c_master_write_register(i2c_port_t port, uint16_t address, uint8_t reg, const uint8_t* data, uint16_t dataSize, TickType_t timeout) {
+    return tt::hal::i2c::masterWrite(port, address, reg, data, dataSize, timeout);
+}
+
+bool tt_hal_i2c_master_write_read(i2c_port_t port, uint8_t address, const uint8_t* writeData, size_t writeDataSize, uint8_t* readData, size_t readDataSize, TickType_t timeout) {
+    return tt::hal::i2c::masterWriteRead(port, address, writeData, writeDataSize, readData, readDataSize, timeout);
+}
+
+bool tt_hal_i2c_master_has_device_at_address(i2c_port_t port, uint8_t address, TickType_t timeout) {
+    return tt::hal::i2c::masterHasDeviceAtAddress(port, address, timeout);
+}
+
+bool tt_hal_i2c_lock(i2c_port_t port, TickType_t timeout) {
+    return tt::hal::i2c::lock(port, timeout);
+}
+
+bool tt_hal_i2c_unlock(i2c_port_t port) {
+    return tt::hal::i2c::unlock(port);
+}
+
+}

TactilityC/Source/tt_hal_i2c.h

@@ -0,0 +1,27 @@
+#pragma once
+
+#include <freertos/FreeRTOS.h>
+#include <hal/i2c_types.h>
+#include <stddef.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+bool tt_hal_i2c_start(i2c_port_t port);
+bool tt_hal_i2c_stop(i2c_port_t port);
+bool tt_hal_i2c_is_started(i2c_port_t port);
+
+bool tt_hal_i2c_master_read(i2c_port_t port, uint8_t address, uint8_t* data, size_t dataSize, TickType_t timeout);
+bool tt_hal_i2c_master_read_register(i2c_port_t port, uint8_t address, uint8_t reg, uint8_t* data, size_t dataSize, TickType_t timeout);
+bool tt_hal_i2c_master_write(i2c_port_t port, uint16_t address, const uint8_t* data, uint16_t dataSize, TickType_t timeout);
+bool tt_hal_i2c_master_write_register(i2c_port_t port, uint16_t address, uint8_t reg, const uint8_t* data, uint16_t dataSize, TickType_t timeout);
+bool tt_hal_i2c_master_write_read(i2c_port_t port, uint8_t address, const uint8_t* writeData, size_t writeDataSize, uint8_t* readData, size_t readDataSize, TickType_t timeout);
+bool tt_hal_i2c_master_has_device_at_address(i2c_port_t port, uint8_t address, TickType_t timeout);
+
+bool tt_hal_i2c_lock(i2c_port_t port, TickType_t timeout);
+bool tt_hal_i2c_unlock(i2c_port_t port);
+
+#ifdef __cplusplus
+}
+#endif

TactilityC/Source/tt_init.cpp

@@ -7,6 +7,7 @@
 #include "tt_app_alertdialog.h"
 #include "tt_app_selectiondialog.h"
 #include "tt_bundle.h"
+#include "tt_hal_i2c.h"
 #include "tt_lvgl_spinner.h"
 #include "tt_lvgl_toolbar.h"
 #include "tt_message_queue.h"
@@ -39,6 +40,17 @@ const struct esp_elfsym elf_symbols[] {
     ESP_ELFSYM_EXPORT(tt_bundle_put_int32),
     ESP_ELFSYM_EXPORT(tt_bundle_put_string),
     ESP_ELFSYM_EXPORT(tt_set_app_manifest),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_start),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_stop),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_is_started),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_master_read),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_master_read_register),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_master_write),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_master_write_register),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_master_write_read),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_master_has_device_at_address),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_lock),
+    ESP_ELFSYM_EXPORT(tt_hal_i2c_unlock),
     ESP_ELFSYM_EXPORT(tt_lvgl_toolbar_create),
     ESP_ELFSYM_EXPORT(tt_lvgl_toolbar_create_simple),
     ESP_ELFSYM_EXPORT(tt_message_queue_alloc),

TactilityHeadless/Source/hal/i2c/I2c.cpp

@@ -58,7 +58,7 @@ bool init(const std::vector<i2c::Configuration>& configurations) {
    return true;
 }
 
-static bool configure_locked(i2c_port_t port, const i2c_config_t& configuration) {
+static bool configureLocked(i2c_port_t port, const i2c_config_t& configuration) {
     Data& data = dataArray[port];
     if (data.isStarted) {
         TT_LOG_E(TAG, "(%d) Cannot reconfigure while interface is started", port);
@@ -73,10 +73,14 @@ static bool configure_locked(i2c_port_t port, const i2c_config_t& configuration)
 }
 
 bool configure(i2c_port_t port, const i2c_config_t& configuration) {
-    lock(port);
+    if (lock(port)) {
-    bool result = configure_locked(port, configuration);
+        bool result = configureLocked(port, configuration);
-    unlock(port);
+        unlock(port);
-    return result;
+        return result;
+    } else {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 }
 
 static bool startLocked(i2c_port_t port) {
@@ -113,10 +117,14 @@ static bool startLocked(i2c_port_t port) {
 }
 
 bool start(i2c_port_t port) {
-    lock(port);
+    if (lock(port)) {
-    bool result = startLocked(port);
+        bool result = startLocked(port);
-    unlock(port);
+        unlock(port);
-    return result;
+        return result;
+    } else {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 }
 
 static bool stopLocked(i2c_port_t port) {
@@ -146,30 +154,45 @@ static bool stopLocked(i2c_port_t port) {
 }
 
 bool stop(i2c_port_t port) {
-    lock(port);
+    if (lock(port)) {
-    bool result = stopLocked(port);
+        bool result = stopLocked(port);
-    unlock(port);
+        unlock(port);
-    return result;
+        return result;
+    } else {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 }
 
 bool isStarted(i2c_port_t port) {
-    lock(port);
+    if (lock(port, 50 / portTICK_PERIOD_MS)) {
-    bool started = dataArray[port].isStarted;
+        bool started = dataArray[port].isStarted;
-    unlock(port);
+        unlock(port);
-    return started;
+        return started;
+    } else {
+        // If we can't get a lock, we assume the device is busy and thus has started
+        return true;
+    }
 }
 
 bool masterRead(i2c_port_t port, uint8_t address, uint8_t* data, size_t dataSize, TickType_t timeout) {
-    lock(port);
+    if (lock(port)) {
-    esp_err_t result = i2c_master_read_from_device(port, address, data, dataSize, timeout);
+        esp_err_t result = i2c_master_read_from_device(port, address, data, dataSize, timeout);
-    unlock(port);
+        unlock(port);
-    return result == ESP_OK;
+        return result == ESP_OK;
+    } else {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 }
 
-esp_err_t masterRead(i2c_port_t port, uint8_t address, uint8_t reg, uint8_t* data, size_t dataSize, TickType_t timeout) {
+bool masterRead(i2c_port_t port, uint8_t address, uint8_t reg, uint8_t* data, size_t dataSize, TickType_t timeout) {
     tt_check(reg != 0);
 
-    lock(port);
+    if (!lock(port)) {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 
     i2c_cmd_handle_t cmd = i2c_cmd_link_create();
     // Set address pointer
@@ -187,24 +210,32 @@ esp_err_t masterRead(i2c_port_t port, uint8_t address, uint8_t reg, uint8_t* dat
     esp_err_t result = i2c_master_cmd_begin(port, cmd, timeout);
     i2c_cmd_link_delete(cmd);
 
+    unlock(port);
+
     ESP_LOG_BUFFER_HEX_LEVEL(TAG, data, dataSize, ESP_LOG_DEBUG);
     ESP_ERROR_CHECK_WITHOUT_ABORT(result);
 
-    unlock(port);
-    return result;
-}
-
-bool masterWrite(i2c_port_t port, uint16_t address, const uint8_t* data, uint16_t dataSize, TickType_t timeout) {
-    lock(port);
-    esp_err_t result = i2c_master_write_to_device(port, address, data, dataSize, timeout);
-    unlock(port);
     return result == ESP_OK;
 }
 
-esp_err_t masterWrite(i2c_port_t port, uint16_t address, uint8_t reg, const uint8_t* data, uint16_t dataSize, TickType_t timeout) {
+bool masterWrite(i2c_port_t port, uint16_t address, const uint8_t* data, uint16_t dataSize, TickType_t timeout) {
+    if (lock(port)) {
+        esp_err_t result = i2c_master_write_to_device(port, address, data, dataSize, timeout);
+        unlock(port);
+        return result == ESP_OK;
+    } else {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
+}
+
+bool masterWrite(i2c_port_t port, uint16_t address, uint8_t reg, const uint8_t* data, uint16_t dataSize, TickType_t timeout) {
     tt_check(reg != 0);
 
-    lock(port);
+    if (!lock(port)) {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 
     i2c_cmd_handle_t cmd = i2c_cmd_link_create();
     i2c_master_start(cmd);
@@ -215,34 +246,42 @@ esp_err_t masterWrite(i2c_port_t port, uint16_t address, uint8_t reg, const uint
     esp_err_t result = i2c_master_cmd_begin(port, cmd, timeout);
     i2c_cmd_link_delete(cmd);
 
-    ESP_ERROR_CHECK_WITHOUT_ABORT(result);
-
     unlock(port);
 
-    return result;
+    ESP_ERROR_CHECK_WITHOUT_ABORT(result);
+
+    return result == ESP_OK;
 }
 
 bool masterWriteRead(i2c_port_t port, uint8_t address, const uint8_t* writeData, size_t writeDataSize, uint8_t* readData, size_t readDataSize, TickType_t timeout) {
-    lock(port);
+    if (lock(port)) {
-    esp_err_t result = i2c_master_write_read_device(port, address, writeData, writeDataSize, readData, readDataSize, timeout);
+        esp_err_t result = i2c_master_write_read_device(port, address, writeData, writeDataSize, readData, readDataSize, timeout);
-    unlock(port);
+        unlock(port);
-    return result == ESP_OK;
+        return result == ESP_OK;
+    } else {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 }
 
-bool masterCheckAddressForDevice(i2c_port_t port, uint8_t address, TickType_t timeout) {
+bool masterHasDeviceAtAddress(i2c_port_t port, uint8_t address, TickType_t timeout) {
-    lock(port);
+    if (lock(port)) {
-    uint8_t message[2] = { 0, 0 };
+        uint8_t message[2] = { 0, 0 };
-    esp_err_t result = i2c_master_write_to_device(port, address, message, 2, timeout);
+        esp_err_t result = i2c_master_write_to_device(port, address, message, 2, timeout);
-    unlock(port);
+        unlock(port);
-    return result == ESP_OK;
+        return result == ESP_OK;
+    } else {
+        TT_LOG_E(TAG, "(%d) Mutex timeout", port);
+        return false;
+    }
 }
 
-TtStatus lock(i2c_port_t port, TickType_t timeout) {
+bool lock(i2c_port_t port, TickType_t timeout) {
-    return dataArray[port].mutex.acquire(timeout);
+    return dataArray[port].mutex.lock(timeout);
 }
 
-TtStatus unlock(i2c_port_t port) {
+bool unlock(i2c_port_t port) {
-    return dataArray[port].mutex.release();
+    return dataArray[port].mutex.unlock();
 }
 
 } // namespace

TactilityHeadless/Source/hal/i2c/I2c.h

@@ -15,7 +15,7 @@ typedef enum {
     InitDisabled // Not initialized by default
 } InitMode;
 
-typedef struct {
+struct Configuration {
     std::string name;
     /** The port to operate on */
     i2c_port_t port;
@@ -27,20 +27,29 @@ typedef struct {
     bool hasMutableConfiguration;
     /** Configuration that must be valid when initAtBoot is set to true. */
     i2c_config_t config;
-} Configuration;
+};
+
+enum Status {
+    STARTED,
+    STOPPED,
+    UNKNOWN
+};
+
 
 bool init(const std::vector<i2c::Configuration>& configurations);
 
 bool start(i2c_port_t port);
 bool stop(i2c_port_t port);
 bool isStarted(i2c_port_t port);
+
 bool masterRead(i2c_port_t port, uint8_t address, uint8_t* data, size_t dataSize, TickType_t timeout);
-esp_err_t masterRead(i2c_port_t port, uint8_t address, uint8_t reg, uint8_t* data, size_t dataSize, TickType_t timeout);
+bool masterRead(i2c_port_t port, uint8_t address, uint8_t reg, uint8_t* data, size_t dataSize, TickType_t timeout);
 bool masterWrite(i2c_port_t port, uint16_t address, const uint8_t* data, uint16_t dataSize, TickType_t timeout);
-esp_err_t masterWrite(i2c_port_t port, uint16_t address, uint8_t reg, const uint8_t* data, uint16_t dataSize, TickType_t timeout);
+bool masterWrite(i2c_port_t port, uint16_t address, uint8_t reg, const uint8_t* data, uint16_t dataSize, TickType_t timeout);
 bool masterWriteRead(i2c_port_t port, uint8_t address, const uint8_t* writeData, size_t writeDataSize, uint8_t* readData, size_t readDataSize, TickType_t timeout);
-bool masterCheckAddressForDevice(i2c_port_t port, uint8_t address, TickType_t timeout);
+bool masterHasDeviceAtAddress(i2c_port_t port, uint8_t address, TickType_t timeout);
-TtStatus lock(i2c_port_t port, TickType_t timeout = UINT_MAX);
+
-TtStatus unlock(i2c_port_t port);
+bool lock(i2c_port_t port, TickType_t timeout = 10 / portTICK_PERIOD_MS);
+bool unlock(i2c_port_t port);
 
 } // namespace

TactilityHeadless/Source/hal/i2c/I2cMock.cpp

@@ -88,15 +88,15 @@ bool write(i2c_port_t port, uint16_t address, uint32_t reg, const uint8_t* buffe
     return false;
 }
 
-TtStatus lock(i2c_port_t port, TickType_t timeout) {
+bool lock(i2c_port_t port, TickType_t timeout) {
-    return dataArray[port].mutex.acquire(timeout);
+    return dataArray[port].mutex.lock(timeout);
 }
 
-TtStatus unlock(i2c_port_t port) {
+bool unlock(i2c_port_t port) {
-    return dataArray[port].mutex.release();
+    return dataArray[port].mutex.unlock();
 }
 
-bool masterCheckAddressForDevice(i2c_port_t port, uint8_t address, TickType_t timeout) {
+bool masterHasDeviceAtAddress(i2c_port_t port, uint8_t address, TickType_t timeout) {
     return (rand()) % 25 == 0;
 }