feat: trigger AHT20 sampling from native touch electrode and notify active agent#84
Open
IRONICBo wants to merge 8 commits intomemovai:mainfrom
Open
feat: trigger AHT20 sampling from native touch electrode and notify active agent#84IRONICBo wants to merge 8 commits intomemovai:mainfrom
IRONICBo wants to merge 8 commits intomemovai:mainfrom
Conversation
Signed-off-by: Asklv <boironic@gmail.com>
📝 WalkthroughWalkthroughAdds new hardware drivers, trigger logic, Telegram image support, and tools: AHT20 env sensor, lick touch trigger task, IMU temperature API, button event callback, message-bus latest-client context API, Telegram photo sending, and a new pixiviz/someacg tool with registry registration and CMake entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Touch as Touch Sensor
participant LickTask as Lick Trigger Task
participant Env as Env Sensor (AHT20)
participant IMU as IMU (QMI8658)
participant MsgBus as Message Bus
participant Telegram as Telegram/API
LickTask->>Touch: Poll & smooth readings
Touch-->>LickTask: Activity detected
alt Triggered & cooldown ok
LickTask->>Env: env_sensor_read()
Env-->>LickTask: temperature, humidity (or error)
alt env failed
LickTask->>IMU: QMI8658_Read_Temperature()
IMU-->>LickTask: temperature
end
LickTask->>MsgBus: message_bus_get_latest_client_context()
MsgBus-->>LickTask: channel, chat_id (or not found)
LickTask->>Telegram: send photo or message (via message bus routing)
Telegram-->>LickTask: API response
end
LickTask->>LickTask: apply cooldown / continue loop
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
main/buttons/button_driver.h (1)
14-16: Consider grouping the new typedef abovebutton_Init()for API readability.Placing the typedef and its associated setter after an unrelated function declaration (
button_Init) makes the header slightly harder to scan. Moving them beforebutton_Initwould keep logically related declarations together.♻️ Suggested reorder
+typedef void (*button_event_cb_t)(PressEvent event); +void button_set_event_callback(button_event_cb_t cb); + void button_Init(void); -typedef void (*button_event_cb_t)(PressEvent event); -void button_set_event_callback(button_event_cb_t cb);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/buttons/button_driver.h` around lines 14 - 16, Move the typedef and its setter above the initialization declaration so related API pieces are grouped: place the typedef button_event_cb_t and the function declaration void button_set_event_callback(button_event_cb_t cb) before void button_Init(void) in button_driver.h to improve readability and logical ordering.main/buttons/button_driver.c (1)
18-23:s_event_cbis not concurrency-safe on dual-core ESP32-S3.
button_set_event_callbackcan be called from any task while theesp_timertask readss_event_cbinside the callback chain. A C compiler also has no ordering guarantee for an unqualified pointer store/load pair across tasks. Using a critical section prevents a potential torn read on the timer task.🔒 Proposed fix using a portMUX critical section
+#include "freertos/FreeRTOS.h" +#include "freertos/portmacro.h" + static button_event_cb_t s_event_cb = NULL; +static portMUX_TYPE s_cb_mux = portMUX_INITIALIZER_UNLOCKED; void button_set_event_callback(button_event_cb_t cb) { - s_event_cb = cb; + portENTER_CRITICAL(&s_cb_mux); + s_event_cb = cb; + portEXIT_CRITICAL(&s_cb_mux); }Then in each event callback, replace bare reads:
- if (s_event_cb) s_event_cb(SINGLE_CLICK); + button_event_cb_t cb; + portENTER_CRITICAL(&s_cb_mux); + cb = s_event_cb; + portEXIT_CRITICAL(&s_cb_mux); + if (cb) cb(SINGLE_CLICK);(apply the same pattern for
DOUBLE_CLICKandLONG_PRESS_START)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/buttons/button_driver.c` around lines 18 - 23, s_event_cb is written without synchronization in button_set_event_callback and read concurrently by the timer callback chain, risking torn reads on dual-core ESP32-S3; fix by adding a portMUX critical section around all accesses: declare a static portMUX_TYPE (e.g. s_button_mux = portMUX_INITIALIZER_UNLOCKED), wrap the pointer store in button_set_event_callback with portENTER_CRITICAL(&s_button_mux)/portEXIT_CRITICAL(&s_button_mux), and wrap every bare read of s_event_cb inside the event callback functions (including the handlers for DOUBLE_CLICK and LONG_PRESS_START) with the same critical enter/exit to ensure atomic load/store ordering across tasks.main/sensors/env_sensor.c (1)
86-104: AHT20 measurement typically takes ~80 ms; the first few 20 ms polls will always see BUSY.The polling loop delays 20 ms per iteration, but the AHT20 measurement time is ~75–80 ms. This means the first 3–4 reads will always return BUSY, burning I2C transactions for no benefit. Consider adding an initial ~80 ms delay before entering the polling loop, then use shorter retry intervals for the remaining attempts.
This is a minor efficiency concern — the current logic is functionally correct as-is.
⏱️ Suggested optimization
+ vTaskDelay(pdMS_TO_TICKS(80)); /* AHT20 typical measurement time */ + uint8_t data[6] = {0}; - int retries = 5; + int retries = 3; do { - vTaskDelay(pdMS_TO_TICKS(20)); err = i2c_master_read_from_device( I2C_MASTER_NUM, AHT20_I2C_ADDR, data, sizeof(data), pdMS_TO_TICKS(I2C_MASTER_TIMEOUT_MS)); if (err != ESP_OK) { return err; } if ((data[0] & AHT20_STATUS_BUSY_MASK) == 0) { break; } + vTaskDelay(pdMS_TO_TICKS(10)); } while (--retries > 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/sensors/env_sensor.c` around lines 86 - 104, The polling loop that reads the AHT20 (the do { ... } while (--retries > 0) block that calls i2c_master_read_from_device and checks (data[0] & AHT20_STATUS_BUSY_MASK) burns several 20 ms polls even though measurement time is ~75–80 ms; add an initial delay of roughly pdMS_TO_TICKS(80) before entering that polling loop to wait out the measurement, then perform the existing shorter pdMS_TO_TICKS(20) read/retry cycle for the remaining retries (adjust retries accordingly) so you avoid unnecessary I2C transactions while keeping the same timeout/ESP_ERR_TIMEOUT behavior.main/trigger/lick_trigger.c (1)
20-27: Consider making touch thresholds/cooldown configurable.Hard-coded ratios and cooldown make tuning across hardware revisions harder; a config hook (e.g.,
mimi_configor Kconfig) would ease calibration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/trigger/lick_trigger.c` around lines 20 - 27, Replace the hard-coded macros LICK_TOUCH_ENTER_RATIO_PCT, LICK_TOUCH_RELEASE_RATIO_PCT, and LICK_TOUCH_COOLDOWN_US with configurable parameters by adding fields for touch_enter_ratio_pct, touch_release_ratio_pct, touch_cooldown_us (and optionally touch_channel) to your runtime config (e.g., mimi_config) or expose them via Kconfig; initialize module-level variables from that config during the lick trigger init path (e.g., in the lick/task init function) with the current macro values as defaults, and update code that references the macros (and s_touch_channel if made configurable) to use these variables so thresholds/cooldown can be tuned without changing source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/buttons/button_driver.c`:
- Line 35: The callback invocation inside the esp_timer context (s_event_cb
called from button_ticks/Timer_Callback) must not perform blocking operations
like I2C reads; change the design so s_event_cb only signals a lower-priority
consumer (e.g. give a FreeRTOS semaphore or push an event to a queue) and move
the blocking AHT20 I2C read into a dedicated task that waits on that
semaphore/queue; update the button registration and any Button_*_Callback
implementations to call xSemaphoreGive/xQueueSend (or FromISR variant if used in
true ISR) and implement a sensor_task that does the aht20_read when signalled.
In `@main/imu/QMI8658.c`:
- Around line 288-303: The temperature conversion in QMI8658_Read_Temperature
incorrectly adds a 25°C offset; update the assignment to temp_c to use the
datasheet formula by removing the +25.0f bias so that *temp_c is computed as raw
/ 256.0f (use the existing raw variable and temp_c pointer in
QMI8658_Read_Temperature).
In `@main/trigger/lick_trigger.c`:
- Around line 148-156: s_lick_task remains non-NULL if lick_touch_init() fails,
preventing future starts; set s_lick_task to NULL before deleting the task. In
lick_trigger_task, when lick_touch_init() returns not ESP_OK, assign s_lick_task
= NULL (or call a helper to clear the handle), then log the error and call
vTaskDelete(NULL); ensure any other early-exit paths also clear s_lick_task so
future init attempts can proceed.
---
Nitpick comments:
In `@main/buttons/button_driver.c`:
- Around line 18-23: s_event_cb is written without synchronization in
button_set_event_callback and read concurrently by the timer callback chain,
risking torn reads on dual-core ESP32-S3; fix by adding a portMUX critical
section around all accesses: declare a static portMUX_TYPE (e.g. s_button_mux =
portMUX_INITIALIZER_UNLOCKED), wrap the pointer store in
button_set_event_callback with
portENTER_CRITICAL(&s_button_mux)/portEXIT_CRITICAL(&s_button_mux), and wrap
every bare read of s_event_cb inside the event callback functions (including the
handlers for DOUBLE_CLICK and LONG_PRESS_START) with the same critical
enter/exit to ensure atomic load/store ordering across tasks.
In `@main/buttons/button_driver.h`:
- Around line 14-16: Move the typedef and its setter above the initialization
declaration so related API pieces are grouped: place the typedef
button_event_cb_t and the function declaration void
button_set_event_callback(button_event_cb_t cb) before void button_Init(void) in
button_driver.h to improve readability and logical ordering.
In `@main/sensors/env_sensor.c`:
- Around line 86-104: The polling loop that reads the AHT20 (the do { ... }
while (--retries > 0) block that calls i2c_master_read_from_device and checks
(data[0] & AHT20_STATUS_BUSY_MASK) burns several 20 ms polls even though
measurement time is ~75–80 ms; add an initial delay of roughly pdMS_TO_TICKS(80)
before entering that polling loop to wait out the measurement, then perform the
existing shorter pdMS_TO_TICKS(20) read/retry cycle for the remaining retries
(adjust retries accordingly) so you avoid unnecessary I2C transactions while
keeping the same timeout/ESP_ERR_TIMEOUT behavior.
In `@main/trigger/lick_trigger.c`:
- Around line 20-27: Replace the hard-coded macros LICK_TOUCH_ENTER_RATIO_PCT,
LICK_TOUCH_RELEASE_RATIO_PCT, and LICK_TOUCH_COOLDOWN_US with configurable
parameters by adding fields for touch_enter_ratio_pct, touch_release_ratio_pct,
touch_cooldown_us (and optionally touch_channel) to your runtime config (e.g.,
mimi_config) or expose them via Kconfig; initialize module-level variables from
that config during the lick trigger init path (e.g., in the lick/task init
function) with the current macro values as defaults, and update code that
references the macros (and s_touch_channel if made configurable) to use these
variables so thresholds/cooldown can be tuned without changing source.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
main/CMakeLists.txtmain/bus/message_bus.cmain/bus/message_bus.hmain/buttons/button_driver.cmain/buttons/button_driver.hmain/imu/QMI8658.cmain/imu/QMI8658.hmain/mimi.cmain/sensors/env_sensor.cmain/sensors/env_sensor.hmain/trigger/lick_trigger.cmain/trigger/lick_trigger.h
| struct Button *user_button = (struct Button *)btn; | ||
| if(user_button == &BUTTON1){ | ||
| BOOT_KEY_State = SINGLE_CLICK; | ||
| if (s_event_cb) s_event_cb(SINGLE_CLICK); |
There was a problem hiding this comment.
s_event_cb fires inside the esp_timer task — blocking I2C reads here will stall button debouncing.
To maintain predictable and timely execution of tasks, callbacks should never attempt block (waiting for resources) or yield (give up control) operations, because such operations disrupt the serialized execution of callbacks. The chain is: esp_timer ISR → Timer_Callback → button_ticks() → Button_*_Callback → s_event_cb. Timer callbacks are dispatched from a high-priority esp_timer task; it is recommended to only do the minimal possible amount of work from the callback itself, posting an event to a lower priority task using a queue instead.
Per the PR description, the registered callback is intended to perform AHT20 I2C reads, which are blocking. Blocking inside this chain will delay subsequent 5 ms tick callbacks, corrupting debounce state and causing missed/phantom button events.
Recommended pattern: signal a FreeRTOS semaphore or queue in s_event_cb, then perform the I2C read in a dedicated lower-priority task that waits on that signal.
💡 Sketch of the non-blocking pattern (in the lick_trigger consumer)
// In lick_trigger.c
static SemaphoreHandle_t s_trigger_sem;
static void on_button_event(PressEvent event) {
// Non-blocking: just signal the sensor task
if (event == SINGLE_CLICK) {
xSemaphoreGiveFromISR(s_trigger_sem, NULL);
// portYIELD_FROM_ISR only if dispatching from true ISR context;
// for ESP_TIMER_TASK dispatch a plain xSemaphoreGive suffices.
}
}
static void sensor_task(void *arg) {
for (;;) {
xSemaphoreTake(s_trigger_sem, portMAX_DELAY);
// Safe to do blocking I2C here
aht20_read(...);
}
}Also applies to: 42-42, 49-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/buttons/button_driver.c` at line 35, The callback invocation inside the
esp_timer context (s_event_cb called from button_ticks/Timer_Callback) must not
perform blocking operations like I2C reads; change the design so s_event_cb only
signals a lower-priority consumer (e.g. give a FreeRTOS semaphore or push an
event to a queue) and move the blocking AHT20 I2C read into a dedicated task
that waits on that semaphore/queue; update the button registration and any
Button_*_Callback implementations to call xSemaphoreGive/xQueueSend (or FromISR
variant if used in true ISR) and implement a sensor_task that does the
aht20_read when signalled.
Comment on lines
+288
to
+303
| esp_err_t QMI8658_Read_Temperature(float *temp_c) | ||
| { | ||
| if (!temp_c) { | ||
| return ESP_ERR_INVALID_ARG; | ||
| } | ||
|
|
||
| uint8_t buf[2] = {0}; | ||
| esp_err_t err = I2C_Read(Device_addr, QMI8658_TEMP_L, buf, 2); | ||
| if (err != ESP_OK) { | ||
| return err; | ||
| } | ||
|
|
||
| int16_t raw = (int16_t)(((uint16_t)buf[1] << 8) | buf[0]); | ||
| *temp_c = ((float)raw / 256.0f) + 25.0f; | ||
| return ESP_OK; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
QMI8658 temperature register conversion formula datasheet
💡 Result:
For the QMI8658 temperature registers:
-
Read TEMPERATURE_L (0x33) and TEMPERATURE_H (0x34), combine as a signed 16-bit little-endian value:
raw = (int16_t)((TEMP_H << 8) | TEMP_L).[1] -
Datasheet scale: 1 LSB = 1/256 °C (i.e., 256 LSB/°C).[1][2]
Conversion formula:
[
T_{^\circ C} = \frac{\text{raw}}{256}
]
Sources: NuttX QMI8658 driver documentation (registers + scale) [1] and a QMI8658 driver doc noting the same datasheet scale [2].
[1] (mail-archive.com)
[2] (docs.rs)
Citations:
- 1: https://www.mail-archive.com/commits%40nuttx.apache.org/msg148117.html?utm_source=openai
- 2: https://docs.rs/ph-qmi8658/latest/ph_qmi8658/fn.temperature_celsius.html
Remove the + 25.0f offset from the temperature conversion formula.
The QMI8658 datasheet specifies the temperature conversion as T = raw / 256 with no offset. The current implementation's raw / 256.0f + 25.0f will bias all temperature readings by 25°C above the actual value. Update line 302 to:
*temp_c = (float)raw / 256.0f;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/imu/QMI8658.c` around lines 288 - 303, The temperature conversion in
QMI8658_Read_Temperature incorrectly adds a 25°C offset; update the assignment
to temp_c to use the datasheet formula by removing the +25.0f bias so that
*temp_c is computed as raw / 256.0f (use the existing raw variable and temp_c
pointer in QMI8658_Read_Temperature).
Comment on lines
+148
to
+156
| static void lick_trigger_task(void *arg) | ||
| { | ||
| (void)arg; | ||
|
|
||
| if (lick_touch_init() != ESP_OK) { | ||
| ESP_LOGE(TAG, "Touch init failed, lick trigger task stopped"); | ||
| vTaskDelete(NULL); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Clear the task handle if init fails.
If lick_touch_init() fails, the task deletes itself but s_lick_task remains non-NULL, so future init attempts are skipped and the trigger never recovers.
🔧 Suggested fix
if (lick_touch_init() != ESP_OK) {
ESP_LOGE(TAG, "Touch init failed, lick trigger task stopped");
+ s_lick_task = NULL;
vTaskDelete(NULL);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static void lick_trigger_task(void *arg) | |
| { | |
| (void)arg; | |
| if (lick_touch_init() != ESP_OK) { | |
| ESP_LOGE(TAG, "Touch init failed, lick trigger task stopped"); | |
| vTaskDelete(NULL); | |
| return; | |
| } | |
| static void lick_trigger_task(void *arg) | |
| { | |
| (void)arg; | |
| if (lick_touch_init() != ESP_OK) { | |
| ESP_LOGE(TAG, "Touch init failed, lick trigger task stopped"); | |
| s_lick_task = NULL; | |
| vTaskDelete(NULL); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/trigger/lick_trigger.c` around lines 148 - 156, s_lick_task remains
non-NULL if lick_touch_init() fails, preventing future starts; set s_lick_task
to NULL before deleting the task. In lick_trigger_task, when lick_touch_init()
returns not ESP_OK, assign s_lick_task = NULL (or call a helper to clear the
handle), then log the error and call vTaskDelete(NULL); ensure any other
early-exit paths also clear s_lick_task so future init attempts can proceed.
Signed-off-by: Bo <boironic@gmail.com>
Signed-off-by: Bo <boironic@gmail.com>
…logging Signed-off-by: Asklv <boironic@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
main/trigger/lick_trigger.c (1)
262-265:⚠️ Potential issue | 🟠 MajorClear
s_lick_taskwhen touch init fails before deleting the task.If init fails here,
s_lick_taskremains non-NULL and laterlick_trigger_init()calls will incorrectly no-op.🔧 Suggested fix
if (lick_touch_init() != ESP_OK) { + s_lick_task = NULL; ESP_LOGE(TAG, "Touch init failed, lick trigger task stopped"); vTaskDelete(NULL); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/trigger/lick_trigger.c` around lines 262 - 265, When lick_touch_init() fails inside the task startup, clear the global task handle s_lick_task before calling vTaskDelete(NULL) and returning so subsequent calls to lick_trigger_init() won't incorrectly think the task is running; locate the failure branch in the function that calls lick_touch_init() (where ESP_LOGE is emitted) and set s_lick_task = NULL immediately prior to vTaskDelete(NULL) / return.
🧹 Nitpick comments (2)
main/mimi.c (1)
92-113: Preserve text/image ordering intelegram_send_rich.Current flow sends photos during parsing but delays all text until the end, which can reorder mixed content.
♻️ Suggested refactor
- if (parse_markdown_image_line(trim, image_url, sizeof(image_url), caption, sizeof(caption))) { + if (parse_markdown_image_line(trim, image_url, sizeof(image_url), caption, sizeof(caption))) { + if (text_off > 0) { + esp_err_t flush_err = telegram_send_message(chat_id, text_buf); + if (flush_err != ESP_OK) any_fail = true; + text_buf[0] = '\0'; + text_off = 0; + } esp_err_t e = telegram_send_photo(chat_id, image_url, caption[0] ? caption : NULL); if (e != ESP_OK) { any_fail = true; ESP_LOGW(TAG, "sendPhoto failed, fallback to text URL"); text_off += snprintf(text_buf + text_off, n + 1 - text_off, "%s\n", image_url); } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/mimi.c` around lines 92 - 113, The current parser sends photos immediately but defers all text until after the loop, breaking original ordering; modify the loop that uses strtok_r and parse_markdown_image_line so that whenever you encounter an image you first flush any accumulated text in text_buf by calling telegram_send_message(chat_id, text_buf) (only if text_buf is non-empty), reset text_off to 0/clear text_buf, then call telegram_send_photo(chat_id, image_url, caption[0] ? caption : NULL), handling errors (update any_fail if photo send fails and, on photo failure, append the image_url back into the now-empty text_buf or directly send it as text as current logic does); after the loop flush remaining text_buf once more into telegram_send_message; keep existing variables and function names (text_buf, text_off, any_fail, parse_markdown_image_line, telegram_send_photo, telegram_send_message) to locate and implement the change.main/trigger/lick_trigger.c (1)
94-101: Consider demoting periodic touch diagnostics to DEBUG level.These logs run continuously (1 Hz) and can drown higher-signal runtime logs.
Also applies to: 272-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/trigger/lick_trigger.c` around lines 94 - 101, The periodic touch diagnostic logs using ESP_LOGI that print touch metrics (the call with TAG and the format "Touch debug GPIO%d: smooth=%lu benchmark=%lu enter=%lu release=%lu active=%d" which references s_touch_channel, smooth, benchmark, enter_threshold, release_threshold, s_touch_last_active) should be demoted to debug level; replace that ESP_LOGI with ESP_LOGD and do the same for the other similar periodic touch diagnostic ESP_LOGI occurrences in this file (the other block that logs touch diagnostics around the same variables) so these continuous 1Hz messages don’t clutter INFO-level logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/skills/skill_loader.c`:
- Around line 103-107: The instruction block in the string literal contains
duplicate "4." entries which makes the step ordering ambiguous; update the
numbering in the multi-line string (the literal containing the lines starting
"4. If the user asks for pictures..." and "4. After each hardware touch event
message...") so the sequence is unique—change the second "4." to "5." and
increment the existing "5." to "6." to preserve order; locate this literal in
skill_loader.c (the string passed where the "## How to use" steps are assembled)
and adjust the numeric prefixes only.
---
Duplicate comments:
In `@main/trigger/lick_trigger.c`:
- Around line 262-265: When lick_touch_init() fails inside the task startup,
clear the global task handle s_lick_task before calling vTaskDelete(NULL) and
returning so subsequent calls to lick_trigger_init() won't incorrectly think the
task is running; locate the failure branch in the function that calls
lick_touch_init() (where ESP_LOGE is emitted) and set s_lick_task = NULL
immediately prior to vTaskDelete(NULL) / return.
---
Nitpick comments:
In `@main/mimi.c`:
- Around line 92-113: The current parser sends photos immediately but defers all
text until after the loop, breaking original ordering; modify the loop that uses
strtok_r and parse_markdown_image_line so that whenever you encounter an image
you first flush any accumulated text in text_buf by calling
telegram_send_message(chat_id, text_buf) (only if text_buf is non-empty), reset
text_off to 0/clear text_buf, then call telegram_send_photo(chat_id, image_url,
caption[0] ? caption : NULL), handling errors (update any_fail if photo send
fails and, on photo failure, append the image_url back into the now-empty
text_buf or directly send it as text as current logic does); after the loop
flush remaining text_buf once more into telegram_send_message; keep existing
variables and function names (text_buf, text_off, any_fail,
parse_markdown_image_line, telegram_send_photo, telegram_send_message) to locate
and implement the change.
In `@main/trigger/lick_trigger.c`:
- Around line 94-101: The periodic touch diagnostic logs using ESP_LOGI that
print touch metrics (the call with TAG and the format "Touch debug GPIO%d:
smooth=%lu benchmark=%lu enter=%lu release=%lu active=%d" which references
s_touch_channel, smooth, benchmark, enter_threshold, release_threshold,
s_touch_last_active) should be demoted to debug level; replace that ESP_LOGI
with ESP_LOGD and do the same for the other similar periodic touch diagnostic
ESP_LOGI occurrences in this file (the other block that logs touch diagnostics
around the same variables) so these continuous 1Hz messages don’t clutter
INFO-level logs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
main/CMakeLists.txtmain/agent/context_builder.cmain/cli/serial_cli.cmain/mimi.cmain/sensors/env_sensor.cmain/skills/skill_loader.cmain/telegram/telegram_bot.cmain/telegram/telegram_bot.hmain/tools/tool_pixiviz.cmain/tools/tool_pixiviz.hmain/tools/tool_registry.cmain/trigger/lick_trigger.csdkconfig.defaults.esp32s3
🚧 Files skipped from review as they are similar to previous changes (1)
- main/CMakeLists.txt
Comment on lines
+103
to
+107
| "4. If the user asks for pictures, call someacg_list first, then send images with someacg_send_image or someacg_send_random.\n" \ | ||
| " If you still need markdown image lines, use: \n" \ | ||
| "4. After each hardware touch event message, interpret it as positive feedback and adapt the next batch.\n" \ | ||
| "5. If temperature/humidity data is present, add one playful English line that references the sensor reading,\n" \ | ||
| " then continue recommendations.\n" \ |
There was a problem hiding this comment.
Fix duplicated step numbering in skill instructions.
## How to use has two step 4 entries, which makes the flow ambiguous for the agent.
🛠️ Suggested text fix
- "4. If the user asks for pictures, call someacg_list first, then send images with someacg_send_image or someacg_send_random.\n" \
+ "4. If the user asks for pictures, call someacg_list first, then send images with someacg_send_image or someacg_send_random.\n" \
" If you still need markdown image lines, use: \n" \
- "4. After each hardware touch event message, interpret it as positive feedback and adapt the next batch.\n" \
- "5. If temperature/humidity data is present, add one playful English line that references the sensor reading,\n" \
+ "5. After each hardware touch event message, interpret it as positive feedback and adapt the next batch.\n" \
+ "6. If temperature/humidity data is present, add one playful English line that references the sensor reading,\n" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/skills/skill_loader.c` around lines 103 - 107, The instruction block in
the string literal contains duplicate "4." entries which makes the step ordering
ambiguous; update the numbering in the multi-line string (the literal containing
the lines starting "4. If the user asks for pictures..." and "4. After each
hardware touch event message...") so the sequence is unique—change the second
"4." to "5." and increment the existing "5." to "6." to preserve order; locate
this literal in skill_loader.c (the string passed where the "## How to use"
steps are assembled) and adjust the numeric prefixes only.
nakhonthai
approved these changes
Mar 5, 2026
nakhonthai
approved these changes
Mar 5, 2026
nakhonthai
approved these changes
Mar 6, 2026
nakhonthai
approved these changes
Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds an end-to-end hardware trigger flow: native ESP32-S3 touch electrode -> AHT20 temperature/humidity read -> send prompt to the currently active agent
session.
It introduces
env_sensor(AHT20 over I2C),lick_trigger(touch-based trigger), and message-bus context routing so hardware events go to the latest activeTelegram/WebSocket session when available. If AHT20 read fails, IMU temperature is used as fallback.
Build was verified with:
source ~/esp/esp-idf/export.shidf.py set-target esp32s3idf.py buildSummary by CodeRabbit