fix(esp-vocat): tolerate peripheral I2C read failures (#2287)

Co-authored-by: Wei Li <2232328+weilinear@users.noreply.github.com>

Author: weilinear

main/boards/espressif/esp-vocat/README.md

@@ -83,3 +83,28 @@ idf.py build
 ```bash
 idf.py flash
 ```
+
+## Peripheral I2C error handling
+
+Battery register reads run on the battery worker, with at most three attempts
+for timeout/invalid-response/transfer failures, a 50 ms transfer timeout and
+10/20 ms retry delays. Other errors return immediately. Successful sampling
+runs every two seconds; a failed cycle backs off for five seconds and does not
+trigger battery emotes. The worker's own failure warning is limited to once
+per ten seconds; driver logging is unchanged.
+
+Battery status queries use a mutex-protected, complete last-good snapshot
+instead of performing transfers in UI callbacks. Samples expire after ten
+seconds; absent/stale readings return `false`, never a fabricated empty battery.
+Touch performs one checked transfer and skips gesture processing on failure,
+so a failed read cannot synthesize a release. The first valid battery sample
+establishes the initial charging state without a false charger-insertion event.
+
+This contains peripheral read errors; it does not diagnose power instability,
+reset the shared bus, change codec startup or disable watchdog/brownout checks.
+Temperature sensor handling is unchanged. Host regression tests use an I2C
+failure double and do not replace physical fault injection:
+
+```bash
+python3 -m unittest discover -s scripts/tests -p test_esp_vocat_i2c.py -v
+```

main/boards/espressif/esp-vocat/esp_vocat.cc

@@ -12,6 +12,7 @@
 #include <esp_system.h>
 #include <esp_timer.h>
 #include <cinttypes>
+#include <mutex>
 #include "esp_idf_version.h"
 
 #define ESP_VOCAT_ENABLE_CAP_TOUCH_SENSOR (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(6, 0, 0))
@@ -289,6 +290,23 @@ static RTC_NOINIT_ATTR uint8_t s_pcb_version_cached;
 
 class EspVocat;
 
+// Battery retries run only on its worker. Touch uses one checked transfer so
+// a failed sample cannot synthesize a press/release or stall other callbacks.
+static esp_err_t ReadPeripheralRegs(i2c_master_dev_handle_t device, uint8_t reg, uint8_t* data,
+                                    size_t length, int attempts) {
+    esp_err_t error = ESP_FAIL;
+    for (int attempt = 0; attempt < attempts; ++attempt) {
+        error = i2c_master_transmit_receive(device, &reg, 1, data, length, 50);
+        if (error == ESP_OK)
+            return ESP_OK;
+        if (error != ESP_ERR_TIMEOUT && error != ESP_ERR_INVALID_RESPONSE && error != ESP_FAIL)
+            break;
+        if (attempt + 1 < attempts)
+            vTaskDelay(pdMS_TO_TICKS(10 << attempt));
+    }
+    return error;
+}
+
 class Charge : public I2cDevice {
 public:
     static constexpr uint8_t kRegVoltage = 0x08;
@@ -299,55 +317,61 @@ class Charge : public I2cDevice {
     static constexpr int kChargingCurrentMa = 30;
     static constexpr uint16_t kBatteryStatusDsg = BIT0;
 
-    Charge(i2c_master_bus_handle_t i2c_bus, uint8_t addr) : I2cDevice(i2c_bus, addr) {
-        read_buffer_ = new uint8_t[8];
-    }
-    ~Charge() { delete[] read_buffer_; }
+    Charge(i2c_master_bus_handle_t i2c_bus, uint8_t addr) : I2cDevice(i2c_bus, addr) {}
 
-    int16_t ReadWord(uint8_t reg) {
+    bool ReadWord(uint8_t reg, uint16_t& value) {
         uint8_t data[2] = {0};
-        ReadRegs(reg, data, 2);
-        return static_cast<int16_t>(static_cast<uint16_t>(data[0]) |
-                                    (static_cast<uint16_t>(data[1]) << 8));
-    }
-
-    int GetBatteryLevel() {
-        int level = ReadWord(kRegStateOfCharge);
-        if (level < 0) {
-            return 0;
-        }
-        if (level > 100) {
-            return 100;
-        }
-        return level;
-    }
-
-    bool IsCharging() {
-        const uint16_t status = static_cast<uint16_t>(ReadWord(kRegBatteryStatus));
-        if ((status & kBatteryStatusDsg) == 0) {
-            return true;
+        const esp_err_t error = ReadPeripheralRegs(i2c_device_, reg, data, sizeof(data), 3);
+        if (error != ESP_OK) {
+            const int64_t now = esp_timer_get_time() / 1000;
+            if (now >= next_error_log_ms_) {
+                ESP_LOGW(TAG, "Battery read unavailable reg=0x%02x error=%s", reg,
+                         esp_err_to_name(error));
+                next_error_log_ms_ = now + 10000;
+            }
+            return false;
         }
-
-        const int16_t avg_current_ma = ReadWord(kRegAverageCurrent);
-        const int16_t current_ma = ReadWord(kRegCurrent);
-        // Current is a fallback only: AverageCurrent can lag charger insertion noticeably.
-        return avg_current_ma > kChargingCurrentMa || current_ma > kChargingCurrentMa;
+        value = static_cast<uint16_t>(data[0]) | (static_cast<uint16_t>(data[1]) << 8);
+        return true;
     }
 
-    bool IsDischarging() {
-        return (static_cast<uint16_t>(ReadWord(kRegBatteryStatus)) & kBatteryStatusDsg) != 0;
+    bool GetSnapshot(int& level, bool& charging, bool& discharging) {
+        std::lock_guard<std::mutex> lock(sample_mutex_);
+        const int64_t now = esp_timer_get_time() / 1000;
+        if (!sample_valid_ || now - sample_at_ms_ > 10000)
+            return false;
+        level = level_;
+        charging = charging_;
+        discharging = discharging_;
+        return true;
     }
 
-    void Update() {
-        const int16_t voltage = ReadWord(kRegVoltage);
-        const int16_t current = ReadWord(kRegCurrent);
+    bool Update() {
+        uint16_t level, status, average, current;
+        if (!ReadWord(kRegStateOfCharge, level) || !ReadWord(kRegBatteryStatus, status) ||
+            !ReadWord(kRegAverageCurrent, average) || !ReadWord(kRegCurrent, current) ||
+            level > 100)
+            return false;
         ESP_ERROR_CHECK(temperature_sensor_get_celsius(temp_sensor, &tsens_value));
-        (void)voltage;
-        (void)current;
+        std::lock_guard<std::mutex> lock(sample_mutex_);
+        level_ = level;
+        discharging_ = (status & kBatteryStatusDsg) != 0;
+        // Current is a fallback: AverageCurrent can lag charger insertion.
+        charging_ = !discharging_ || static_cast<int16_t>(average) > kChargingCurrentMa ||
+                    static_cast<int16_t>(current) > kChargingCurrentMa;
+        sample_at_ms_ = esp_timer_get_time() / 1000;
+        sample_valid_ = true;
+        return true;
     }
 
 private:
-    uint8_t* read_buffer_ = nullptr;
+    std::mutex sample_mutex_;
+    int level_ = 0;
+    bool charging_ = false;
+    bool discharging_ = false;
+    bool sample_valid_ = false;
+    int64_t sample_at_ms_ = 0;
+    int64_t next_error_log_ms_ = 0;
 };
 
 class Cst816s : public I2cDevice {
@@ -382,11 +406,13 @@ class Cst816s : public I2cDevice {
         }
     }
 
-    void UpdateTouchPoint() {
-        ReadRegs(0x02, read_buffer_, 6);
+    bool UpdateTouchPoint() {
+        if (ReadPeripheralRegs(i2c_device_, 0x02, read_buffer_, 6, 1) != ESP_OK)
+            return false;
         tp_.num = read_buffer_[0] & 0x0F;
         tp_.x = ((read_buffer_[1] & 0x0F) << 8) | read_buffer_[2];
         tp_.y = ((read_buffer_[3] & 0x0F) << 8) | read_buffer_[4];
+        return true;
     }
 
     const TouchPoint_t& GetTouchPoint() { return tp_; }
@@ -470,6 +496,7 @@ class EspVocat : public WifiBoard {
     esp_timer_handle_t emotion_reset_timer_ = nullptr;
     bool bmi270_ready_ = false;
     bool was_charging_ = false;
+    bool battery_status_initialized_ = false;
     uint8_t low_battery_alert_mask_ = 0;
     int low_battery_plays_left_ = 0;
     int64_t next_low_battery_play_ms_ = 0;
@@ -534,8 +561,14 @@ class EspVocat : public WifiBoard {
             return;
         }
 
-        const int level = charge_->GetBatteryLevel();
-        const bool charging = charge_->IsCharging();
+        int level;
+        bool charging, discharging;
+        if (!charge_->GetSnapshot(level, charging, discharging))
+            return;
+        if (!battery_status_initialized_) {
+            was_charging_ = charging;
+            battery_status_initialized_ = true;
+        }
 
         if (charging && !was_charging_) {
             PlayBatteryEmotion("battery_connected", 4000);
@@ -580,11 +613,13 @@ class EspVocat : public WifiBoard {
     static void battery_task(void* arg) {
         auto* self = static_cast<EspVocat*>(arg);
         while (true) {
+            bool healthy = false;
             if (self != nullptr && self->charge_ != nullptr) {
-                self->charge_->Update();
-                self->HandleBatteryEmotions();
+                healthy = self->charge_->Update();
+                if (healthy)
+                    self->HandleBatteryEmotions();
             }
-            vTaskDelay(pdMS_TO_TICKS(300));
+            vTaskDelay(pdMS_TO_TICKS(healthy ? 2000 : 5000));
         }
     }
 
@@ -774,7 +809,8 @@ class EspVocat : public WifiBoard {
                 auto& board = (EspVocat&)Board::GetInstance();
 
                 ESP_LOGD(TAG, "Touch event, TP_PIN_NUM_INT: %d", gpio_get_level(TP_PIN_NUM_INT));
-                touchpad->UpdateTouchPoint();
+                if (!touchpad->UpdateTouchPoint())
+                    continue;
                 auto touch_event = touchpad->CheckTouchEvent();
 
                 if (touch_event == Cst816s::TOUCH_RELEASE) {
@@ -790,7 +826,6 @@ class EspVocat : public WifiBoard {
 
     void InitializeCharge() {
         charge_ = new Charge(i2c_bus_, 0x55);
-        was_charging_ = charge_->IsCharging();
         xTaskCreatePinnedToCore(battery_task, "batteryTask", 3 * 1024, this, 6,
                                 &charge_task_handle_, 0);
     }
@@ -1170,10 +1205,7 @@ class EspVocat : public WifiBoard {
         if (charge_ == nullptr) {
             return false;
         }
-        level = charge_->GetBatteryLevel();
-        charging = charge_->IsCharging();
-        discharging = charge_->IsDischarging();
-        return true;
+        return charge_->GetSnapshot(level, charging, discharging);
     }
 };
 

scripts/tests/test_esp_vocat_i2c.py

@@ -0,0 +1,134 @@
+"""Run ESP-VoCat's peripheral reader against a host-side I2C failure double."""
+import pathlib
+import shutil
+import subprocess
+import tempfile
+import unittest
+
+ROOT = pathlib.Path(__file__).resolve().parents[2]
+BOARD = ROOT / "main/boards/espressif/esp-vocat/esp_vocat.cc"
+
+
+def function(source, signature):
+    start = source.index(signature)
+    brace = source.index("{", start)
+    depth, end = 1, brace + 1
+    while depth:
+        depth += (source[end] == "{") - (source[end] == "}")
+        end += 1
+    return source[start:end]
+
+
+class EspVocatI2cTests(unittest.TestCase):
+    @unittest.skipUnless(shutil.which("c++"), "requires a host C++ compiler")
+    def test_actual_reader_cache_and_touch_with_failed_transfers(self):
+        board = BOARD.read_text()
+        reader = function(board, "static esp_err_t ReadPeripheralRegs(")
+        charge = "class Charge :" + board.split("class Charge :", 1)[1].split("class Cst816s", 1)[0]
+        source = r'''
+#include <cassert>
+#include <cstdint>
+#include <cstddef>
+#include <cstring>
+#include <mutex>
+#include <vector>
+using esp_err_t = int;
+using i2c_master_dev_handle_t = void*;
+using i2c_master_bus_handle_t = void*;
+constexpr int ESP_OK=0, ESP_ERR_TIMEOUT=1, ESP_ERR_INVALID_RESPONSE=2, ESP_FAIL=3;
+#define BIT0 1
+#define ESP_LOGW(...)
+#define ESP_LOGI(...)
+#define ESP_LOGD(...)
+#define ESP_ERROR_CHECK(result) assert((result)==ESP_OK)
+#define pdMS_TO_TICKS(ms) (ms)
+int64_t now_ms=100;
+int reads=0, permanent_error=0;
+std::vector<int> errors, waits;
+uint16_t registers[256] = {};
+uint8_t touch_data[6] = {1,0,42,0,24,0};
+void* temp_sensor=nullptr; float tsens_value=0;
+int temperature_sensor_get_celsius(void*,float*) { return 0; }
+void vTaskDelay(int ms) { waits.push_back(ms); }
+int64_t esp_timer_get_time() { return now_ms*1000; }
+const char* esp_err_to_name(int) { return "fake"; }
+int i2c_master_transmit_receive(void*,const uint8_t* reg,size_t,uint8_t* data,size_t len,int timeout) {
+ assert(timeout==50);
+ const int error=reads < static_cast<int>(errors.size()) ? errors[reads] : permanent_error;
+ ++reads;
+ if(error) { std::memset(data,0xff,len); return error; }
+ if(len==6) std::memcpy(data,touch_data,6);
+ else { data[0]=registers[*reg]&255; data[1]=registers[*reg]>>8; }
+ return 0;
+}
+class I2cDevice {
+ public: I2cDevice(void*,uint8_t) {}
+ protected: void* i2c_device_=nullptr;
+};
+'''
+        source += reader + charge + r'''
+class Touch {
+ public:
+ struct Point { int num=0,x=-1,y=-1; } tp_;
+ enum TouchEvent { TOUCH_NONE,TOUCH_PRESS,TOUCH_RELEASE,TOUCH_HOLD };
+ bool was_touched_=false; int press_count_=0;
+ uint8_t data_[6] = {}; uint8_t* read_buffer_=data_; void* i2c_device_=nullptr;
+'''
+        source += function(board, "bool UpdateTouchPoint()")
+        source += function(board, "TouchEvent CheckTouchEvent()") + "};\n"
+        source += r'''
+int main() {
+ Charge charge(nullptr,0x55);
+ int level=99; bool charging=true,discharging=false;
+ assert(!charge.GetSnapshot(level,charging,discharging) && reads==0);
+ registers[0x2c]=50; registers[0x0a]=1;
+ registers[0x14]=static_cast<uint16_t>(-40); registers[0x0c]=static_cast<uint16_t>(-50);
+ errors={ESP_ERR_TIMEOUT,ESP_ERR_INVALID_RESPONSE,0};
+ assert(charge.Update() && reads==6 && waits==std::vector<int>({10,20}));
+ assert(charge.GetSnapshot(level,charging,discharging) && reads==6);
+ assert(level==50 && !charging && discharging);
+ errors.clear(); reads=0; waits.clear(); permanent_error=ESP_ERR_TIMEOUT;
+ assert(!charge.Update() && reads==3 && waits==std::vector<int>({10,20}));
+ assert(charge.GetSnapshot(level,charging,discharging) && level==50);
+ reads=0; waits.clear(); permanent_error=42;
+ assert(!charge.Update() && reads==1 && waits.empty());
+ now_ms=10100; assert(charge.GetSnapshot(level,charging,discharging));
+ now_ms=10101; assert(!charge.GetSnapshot(level,charging,discharging));
+ permanent_error=0; registers[0x2c]=60; registers[0x0a]=0;
+ assert(charge.Update() && charge.GetSnapshot(level,charging,discharging));
+ assert(level==60 && charging && !discharging);
+ registers[0x2c]=65535;
+ assert(!charge.Update() && charge.GetSnapshot(level,charging,discharging) && level==60);
+ registers[0x2c]=0; registers[0x0a]=1;
+ assert(charge.Update() && charge.GetSnapshot(level,charging,discharging) && level==0);
+
+ Touch touch; reads=0; waits.clear();
+ assert(touch.UpdateTouchPoint() && touch.CheckTouchEvent()==Touch::TOUCH_PRESS);
+ assert(touch.tp_.x==42 && touch.tp_.y==24 && touch.press_count_==1);
+ permanent_error=ESP_ERR_INVALID_RESPONSE;
+ assert(!touch.UpdateTouchPoint() && reads==2 && waits.empty());
+ assert(touch.tp_.num==1 && touch.was_touched_ && touch.press_count_==1);
+ permanent_error=0; touch_data[0]=0;
+ assert(touch.UpdateTouchPoint() && touch.CheckTouchEvent()==Touch::TOUCH_RELEASE);
+ assert(touch.press_count_==1);
+}
+'''
+        with tempfile.TemporaryDirectory() as tmp:
+            binary = str(pathlib.Path(tmp) / "peripheral-test")
+            subprocess.run(["c++", "-std=c++17", "-fsanitize=undefined", "-x", "c++", "-",
+                            "-o", binary], input=source, text=True, check=True)
+            subprocess.run([binary], check=True)
+
+    def test_failed_samples_do_not_dispatch_touch_or_battery_alerts(self):
+        board = BOARD.read_text()
+        self.assertIn("if (!touchpad->UpdateTouchPoint())", board)
+        touch = function(board, "static void touch_event_task(")
+        self.assertLess(touch.index("if (!touchpad->UpdateTouchPoint())"),
+                        touch.index("CheckTouchEvent()"))
+        battery = function(board, "static void battery_task(")
+        self.assertIn("if (healthy)", battery)
+        self.assertIn("healthy ? 2000 : 5000", battery)
+        getter = function(board, "virtual bool GetBatteryLevel(")
+        self.assertIn("GetSnapshot", getter)
+        self.assertNotIn("ReadWord", getter)
+        self.assertNotIn("ReadPeripheralRegs", getter)