Remove deep_sleep from external components and use the built in core component #16
Conversation
…component Remove deep_sleep from external components and use the built in core component
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces test orchestration in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Button as Button/Input
participant Boot as startup / on_wake
participant Globals as Globals\n(runTest, testCycleCount)
participant Script as testScript
participant Light as RGB Light
Button->>Boot: multi-click / hold event
Boot->>Globals: read runTest
alt runTest == true
Boot->>Script: invoke testScript()
Script->>Globals: set runTest = false
Script->>Globals: set testCycleCount = 0
Script->>Light: turn green ON
Script->>Script: delay
Script->>Light: turn OFF
Script-->>Boot: return
else runTest == false
Boot->>Boot: continue normal startup/status flows
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Integrations/ESPHome/Core.yaml (1)
300-301: Remove or uncomment the commented test initialization code.Lines 300–301 contain commented code that appears related to the new test logic:
// id(testCycleCount) = 0; // id(runTest) = true;This logic is now implemented a few lines below (lines 315–317). Either remove these comments entirely or, if they represent alternative logic, document why they are needed separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Integrations/ESPHome/Core.yaml(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Building Integrations/ESPHome/BTN-1_Minimal.yaml / ESPHome beta
- GitHub Check: Building Integrations/ESPHome/BTN-1_Minimal.yaml / ESPHome dev
- GitHub Check: Building Integrations/ESPHome/BTN-1_Minimal.yaml / ESPHome stable
- GitHub Check: Building Integrations/ESPHome/BTN-1.yaml / ESPHome beta
- GitHub Check: Building Integrations/ESPHome/BTN-1.yaml / ESPHome dev
- GitHub Check: Building Integrations/ESPHome/BTN-1.yaml / ESPHome stable
🔇 Additional comments (4)
Integrations/ESPHome/Core.yaml (4)
161-168: Clarify the intent ofrunTestpersistence andtestCycleCountusage.Two new globals are introduced:
runTesthasrestore_value: yes(persists across reboots with initial value"true"), meaning the device will execute the test script on every boot.testCycleCountis initialized to0but appears to be set but never incremented or read elsewhere in the script.Is
restore_value: yesforrunTestintentional? If the device is meant to test automatically after every reboot, this is fine, but it should be documented. Additionally, clarify the purpose oftestCycleCount—is it intended to track test iterations, and if so, should it be incremented somewhere (e.g., withintestScript)?
74-80: Verify on_boot hook timing and interaction with other boot priorities.A new
on_boothook at priority-10is added to triggertestScriptifrunTestis true. This priority is very late in the boot sequence (after priority-900wake detection logic at line 37). Confirm that executing the test script at this stage does not interfere with normal device startup or other higher-priority initialization tasks.Review all
on_bootpriorities in the file to ensure-10is the correct timing for test execution and does not conflict with sensor updates, API initialization, or other critical startup operations.
315-317: Verify reset button's new test script invocation.The reset button's short-press behavior (< 3 seconds) now initializes test globals and calls
testScript(lines 315–317). Ensure this change is intentional and tested, as it alters the button's existing behavior. Previously (based on comment at line 312), this was a "StatusCheck" action; now it doubles as a test trigger.Confirm that this behavior is desired and document it. Also verify that calling
statusCheckfollowed bytestScript(both at line 313 and 317) doesn't cause redundant or conflicting operations.
141-143: No action needed—deep_sleep component migration is complete.The removal of the external deep_sleep component from
external_componentsis fully complete. All references throughout the codebase use the built-indeep_sleep:component (defined at lines 192–206 withid: deep_sleep_1), and no remaining references to the external GitHub component exist in any YAML files.
| - id: testScript | ||
| then: | ||
| if: | ||
| condition: | ||
| - lambda: "return id(runTest) == true;" | ||
| then: | ||
| - lambda: "id(runTest) = false;" | ||
| - lambda: "id(testCycleCount) = 0;" | ||
| - lambda: "id(runTest) = false;" | ||
| - light.turn_on: | ||
| id: rgb_light | ||
| red: 0% | ||
| green: 100% | ||
| blue: 0% | ||
| - delay: 5s | ||
| - light.turn_on: | ||
| id: rgb_light | ||
| red: 0% | ||
| green: 0% | ||
| blue: 0% | ||
| - light.turn_off: |
There was a problem hiding this comment.
Fix redundant state assignments and unused variable in testScript.
The testScript contains logic issues:
- Redundant assignment:
runTestis set tofalsetwice (lines 709 and 711). - Unused variable:
testCycleCountis set to0(line 710) but never incremented or checked within the script. - Redundant condition check: The script checks
runTest == trueat line 707, but it's only invoked from callers that already verifyrunTestis true (lines 78, 317), making the condition unnecessary.
Apply this diff to clean up the logic:
- id: testScript
then:
if:
- condition:
- - lambda: "return id(runTest) == true;"
- then:
- - lambda: "id(runTest) = false;"
- - lambda: "id(testCycleCount) = 0;"
- - lambda: "id(runTest) = false;"
+ condition:
+ - lambda: "return id(runTest) == true;"
+ then:
+ - lambda: "id(runTest) = false;"
- light.turn_on:
id: rgb_light
red: 0%
green: 100%
blue: 0%
- delay: 5s
- light.turn_on:
id: rgb_light
red: 0%
green: 0%
blue: 0%
- light.turn_off:
id: rgb_lightAdditionally, document the purpose of testCycleCount. If it should track the number of test iterations, add logic to increment it; otherwise, remove it from the globals to avoid confusion.
📝 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.
| - id: testScript | |
| then: | |
| if: | |
| condition: | |
| - lambda: "return id(runTest) == true;" | |
| then: | |
| - lambda: "id(runTest) = false;" | |
| - lambda: "id(testCycleCount) = 0;" | |
| - lambda: "id(runTest) = false;" | |
| - light.turn_on: | |
| id: rgb_light | |
| red: 0% | |
| green: 100% | |
| blue: 0% | |
| - delay: 5s | |
| - light.turn_on: | |
| id: rgb_light | |
| red: 0% | |
| green: 0% | |
| blue: 0% | |
| - light.turn_off: | |
| - id: testScript | |
| then: | |
| if: | |
| condition: | |
| - lambda: "return id(runTest) == true;" | |
| then: | |
| - lambda: "id(runTest) = false;" | |
| - light.turn_on: | |
| id: rgb_light | |
| red: 0% | |
| green: 100% | |
| blue: 0% | |
| - delay: 5s | |
| - light.turn_on: | |
| id: rgb_light | |
| red: 0% | |
| green: 0% | |
| blue: 0% | |
| - light.turn_off: | |
| id: rgb_light |
🤖 Prompt for AI Agents
In Integrations/ESPHome/Core.yaml around lines 703 to 723, clean up testScript
by removing the redundant assignments and unnecessary condition: delete the
duplicate "id(runTest) = false;" (lines 709 and 711) and remove the surrounding
"if: condition: - lambda: 'return id(runTest) == true;'" block so the actions
run directly (or alternatively keep the callers' pre-checks and remove the local
check entirely). For testCycleCount (line 710) either document and implement
increment logic to track iterations (e.g., increment at each run) or remove it
from globals and this script if it is unused; ensure any remaining state changes
are singular (no duplicate state writes) and keep the light on/off sequence as
the only actions.
|
Fixed. Thanks! |
…component
Remove deep_sleep from external components and use the built in core component
Version:
What does this implement/fix?
Types of changes
Checklist / Checklijst:
If user-visible functionality or configuration variables are added/modified:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.