Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches📝 Generate docstrings
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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main/HMI/src/input.c`:
- Around line 95-99: Update input_init around button_task_handle creation and
gpio_isr_handler_add so a failed ISR registration deletes the created task,
clears button_task_handle, and restores the initialization state before
returning the error; preserve the existing success path.
In `@main/self_test/self_test.c`:
- Around line 309-311: Update the input initialization branch in self-test setup
around input_init so an ESP_OK failure enters a recovery path that does not rely
on reset_self_test or button input, rather than merely logging and continuing.
Ensure BootSemaphore can still be released and the self-test cannot block
indefinitely before proceeding.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e073896-405a-4980-aaa1-8bb17f97d875
📒 Files selected for processing (15)
main/CMakeLists.txtmain/HMI/inc/display.hmain/HMI/inc/input.hmain/HMI/inc/screen.hmain/HMI/src/display.cmain/HMI/src/input.cmain/HMI/src/screen.cmain/global_state.hmain/http_server/openapi.yamlmain/idf_component.ymlmain/lv_conf.hmain/lv_font_portfolio-6x8.cmain/self_test/self_test.cmain/system.csdkconfig.defaults
💤 Files with no reviewable changes (12)
- main/HMI/inc/display.h
- main/idf_component.yml
- main/lv_font_portfolio-6x8.c
- main/system.c
- main/CMakeLists.txt
- sdkconfig.defaults
- main/HMI/src/screen.c
- main/lv_conf.h
- main/HMI/inc/screen.h
- main/HMI/src/display.c
- main/http_server/openapi.yaml
- main/global_state.h
| // Task has to exist before the ISR can notify it | ||
| ESP_RETURN_ON_FALSE(xTaskCreate(button_task, "button", 3072, NULL, 5, &button_task_handle) == pdPASS, | ||
| ESP_ERR_NO_MEM, TAG, "Failed to create button task"); | ||
|
|
||
| ESP_RETURN_ON_ERROR(gpio_isr_handler_add(GPIO_BUTTON_BOOT, button_isr_handler, NULL), TAG, "Error adding ISR handler"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Roll back the task when ISR handler registration fails.
xTaskCreate succeeds before gpio_isr_handler_add. If handler registration fails, input_init returns an error but leaves button_task blocked forever with its allocated stack. Delete the task and restore initialization state before returning the error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/HMI/src/input.c` around lines 95 - 99, Update input_init around
button_task_handle creation and gpio_isr_handler_add so a failed ISR
registration deletes the created task, clears button_task_handle, and restores
the initialization state before returning the error; preserve the existing
success path.
| if (input_init(NULL, reset_self_test) != ESP_OK) { | ||
| ESP_LOGE(TAG, "Input init failed!"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not continue after input_init fails.
This branch only logs the error and continues. A later failed test calls tests_done, which waits for reset_self_test to release BootSemaphore. input_init failed, so that callback path is unavailable and the device can remain blocked indefinitely. Use a recovery path that does not depend on button input before continuing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/self_test/self_test.c` around lines 309 - 311, Update the input
initialization branch in self-test setup around input_init so an ESP_OK failure
enters a recovery path that does not rely on reset_self_test or button input,
rather than merely logging and continuing. Ensure BootSemaphore can still be
released and the self-test cannot block indefinitely before proceeding.
1 participant
Summary by CodeRabbit
New Features
Removed Features
Bug Fixes