[FIX] Inject watchdog keepalive during anti-wrinkle silence - #340
[FIX] Inject watchdog keepalive during anti-wrinkle silence#340sharkyy wants to merge 2 commits into
Conversation
|
Hi @sharkyy! This PR references #339, which has not yet received the @3dg1luk43 has been notified and will review the linked issue. Once the Nothing more is needed from you for now — thanks for your patience! |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe watchdog now injects synthetic 0W readings when anti-wrinkle receives no sensor updates beyond ChangesAnti-wrinkle watchdog fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PowerSensor
participant WashDataManager
participant Detector
participant Entities
PowerSensor->>WashDataManager: Sends anti-wrinkle pulse
WashDataManager->>Detector: Processes real power reading
WashDataManager->>WashDataManager: Detects silence beyond off_delay
WashDataManager->>Detector: Injects synthetic 0W reading
Detector-->>WashDataManager: Transitions to OFF
WashDataManager->>Entities: Notifies state update
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🤖 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 `@custom_components/ha_washdata/manager.py`:
- Around line 3252-3255: The anti-wrinkle keepalive path must not update the
sampling-throttle clock used by the reading-processing logic. Replace the
`_last_reading_time` assignment after `self.detector.process_reading(0.0, now)`
with a separate keepalive timestamp, and update the keepalive scheduling logic
to use it while preserving `_last_reading_time` for real readings so high-power
tumble pulses still reach the detector.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6d460e7-8292-4f9c-838a-a98e0d17f7db
⛔ Files ignored due to path filters (1)
tests/test_anti_wrinkle_watchdog_keepalive.pyis excluded by!tests/**
📒 Files selected for processing (3)
CHANGELOG.mdcustom_components/ha_washdata/manager.pycustom_components/ha_washdata/manifest.json
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Validate Pull Request / 1_Check PR Description.txt: [FIX] Inject watchdog keepalive during anti-wrinkle silence
Conclusion: failure
##[group]Run actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b
with:
github-***REDACTED***
script: const pr = context.payload.pull_request;
const owner = context.repo.owner;
const repo = context.repo.repo;
const prNumber = pr.number;
const prAuthor = pr.user.login;
const prBody = pr.body || '';
const GITLOCALIZE_BOT = 'gitlocalize-app[bot]';
if (prAuthor === owner || prAuthor === GITLOCALIZE_BOT) {
console.log(`Author is repo owner or GitLocalize bot (${prAuthor}), skipping completeness check.`);
return;
}
if (pr.state === 'closed') {
console.log('PR is closed — skipping completeness check.');
return;
}
const BOT_MARKER = '<!-- pr-validator-completeness-bot -->';
const LABEL = 'needs description';
// Check 1: description section has real content (not just HTML comments / placeholders)
const descSection = (prBody.match(/## Description\s*([\s\S]*?)(?=\n##|$)/i) || [])[1] || '';
const descClean = descSection
.replace(/<!--[\s\S]*?-->/g, '')
.replace(/\bCloses?\s+#\s*<!--.*?-->/gi, '')
.trim();
const descriptionOk = descClean.length > 10;
// Check 2: at least one checkbox checked in Type of Change
const typeSection = (prBody.match(/## Type of Change\s*([\s\S]*?)(?=\n##|$)/i) || [])[1] || '';
const typeOk = /- \[x\]/i.test(typeSection);
const isComplete = descriptionOk && typeOk;
// Find existing bot comment (paginate so the marker is not missed on busy PRs)
const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: prNumber, per_page: 100 });
const existing = comments.find(c => c.body && c.body.includes(BOT_MARKER));
if (isComplete) {
// Remove warning label if present
try {
await github.rest.issues.removeLabel({ owner, repo, issue_number: prNumber, name: LABEL });
} catch (_) {}
// Remove warning comment if present
if (existing) {
await github.rest.issues.deleteComment({ owner, repo, comment_id: existing.id });
}
console.log('PR description is complete.');
return;
}
c...
GitHub Actions: Validate Pull Request / Check PR Description: [FIX] Inject watchdog keepalive during anti-wrinkle silence
Conclusion: failure
##[group]Run actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b
with:
github-***REDACTED***
script: const pr = context.payload.pull_request;
const owner = context.repo.owner;
const repo = context.repo.repo;
const prNumber = pr.number;
const prAuthor = pr.user.login;
const prBody = pr.body || '';
const GITLOCALIZE_BOT = 'gitlocalize-app[bot]';
if (prAuthor === owner || prAuthor === GITLOCALIZE_BOT) {
console.log(`Author is repo owner or GitLocalize bot (${prAuthor}), skipping completeness check.`);
return;
}
if (pr.state === 'closed') {
console.log('PR is closed — skipping completeness check.');
return;
}
const BOT_MARKER = '<!-- pr-validator-completeness-bot -->';
const LABEL = 'needs description';
// Check 1: description section has real content (not just HTML comments / placeholders)
const descSection = (prBody.match(/## Description\s*([\s\S]*?)(?=\n##|$)/i) || [])[1] || '';
const descClean = descSection
.replace(/<!--[\s\S]*?-->/g, '')
.replace(/\bCloses?\s+#\s*<!--.*?-->/gi, '')
.trim();
const descriptionOk = descClean.length > 10;
// Check 2: at least one checkbox checked in Type of Change
const typeSection = (prBody.match(/## Type of Change\s*([\s\S]*?)(?=\n##|$)/i) || [])[1] || '';
const typeOk = /- \[x\]/i.test(typeSection);
const isComplete = descriptionOk && typeOk;
// Find existing bot comment (paginate so the marker is not missed on busy PRs)
const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: prNumber, per_page: 100 });
const existing = comments.find(c => c.body && c.body.includes(BOT_MARKER));
if (isComplete) {
// Remove warning label if present
try {
await github.rest.issues.removeLabel({ owner, repo, issue_number: prNumber, name: LABEL });
} catch (_) {}
// Remove warning comment if present
if (existing) {
await github.rest.issues.deleteComment({ owner, repo, comment_id: existing.id });
}
console.log('PR description is complete.');
return;
}
c...
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: ONLY NumPy allowed for numerical operations - No SciPy, scikit-learn, or other ML libraries
No external API calls - All processing must be local
ALWAYS usedt_util.now()for timezone-aware datetimes instead of standard datetime functions
All time/energy calculations MUST be datetime-aware using timestamps, not sample counts, with explicit gap handling
NO inline strings in Python code for UI text - All labels/descriptions must be in strings.json and translations/en.json
Useasync_update_entryfor config entry modifications in Home Assistant
Store tunables inentry.options, identity keys inentry.data
Gate debug entities behindexpose_debug_entitiesoption - only expose debug information when explicitly enabled
Always excludepower_data,debug_data, andpower_tracefrom fired Home Assistant events to comply with 32KB event data limit
NEVER drop user data during migrations - always preserve cycles, labels, and corrections from previous versions
Files:
custom_components/ha_washdata/manager.py
🔇 Additional comments (2)
custom_components/ha_washdata/manifest.json (1)
24-24: LGTM!CHANGELOG.md (1)
8-12: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@custom_components/ha_washdata/manager.py`:
- Around line 2870-2872: Update the anti-wrinkle throttle-bypass condition in
the detector logic around is_anti_wrinkle_pulse to compare power against the
same effective exit threshold used by the idle-timer reset:
max(anti_wrinkle_exit_power, stop_threshold_w), rather than min_power. Add a
regression case covering an anti-wrinkle pulse below min_power that still meets
this effective threshold and must bypass throttling.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2bcd822b-a05b-43a5-a840-56cb1ca1b555
⛔ Files ignored due to path filters (1)
tests/test_anti_wrinkle_watchdog_keepalive.pyis excluded by!tests/**
📒 Files selected for processing (2)
CHANGELOG.mdcustom_components/ha_washdata/manager.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: validate-hacs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: ONLY NumPy allowed for numerical operations - No SciPy, scikit-learn, or other ML libraries
No external API calls - All processing must be local
ALWAYS usedt_util.now()for timezone-aware datetimes instead of standard datetime functions
All time/energy calculations MUST be datetime-aware using timestamps, not sample counts, with explicit gap handling
NO inline strings in Python code for UI text - All labels/descriptions must be in strings.json and translations/en.json
Useasync_update_entryfor config entry modifications in Home Assistant
Store tunables inentry.options, identity keys inentry.data
Gate debug entities behindexpose_debug_entitiesoption - only expose debug information when explicitly enabled
Always excludepower_data,debug_data, andpower_tracefrom fired Home Assistant events to comply with 32KB event data limit
NEVER drop user data during migrations - always preserve cycles, labels, and corrections from previous versions
Files:
custom_components/ha_washdata/manager.py
🔇 Additional comments (2)
custom_components/ha_washdata/manager.py (1)
3239-3272: LGTM!CHANGELOG.md (1)
8-12: LGTM!
| is_anti_wrinkle_pulse = ( | ||
| self.detector.state == STATE_ANTI_WRINKLE and power >= min_p | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the throttle bypass to the detector’s anti-wrinkle exit threshold.
The detector resets its anti-wrinkle idle timer for readings at or above max(anti_wrinkle_exit_power, stop_threshold_w), but this bypass requires power >= min_power. When that effective threshold is below min_power, a real tumble pulse in between can arrive after a synthetic keepalive, be throttled, and fail to reset the idle timer.
Use the same effective-exit threshold here and add a regression case for a pulse below min_power.
🔧 Proposed fix
+ anti_wrinkle_effective_exit = max(
+ float(self.detector.config.anti_wrinkle_exit_power),
+ float(self.detector.config.stop_threshold_w),
+ )
is_anti_wrinkle_pulse = (
self.detector.state == STATE_ANTI_WRINKLE
- and power >= min_p
+ and power >= anti_wrinkle_effective_exit
)🤖 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 `@custom_components/ha_washdata/manager.py` around lines 2870 - 2872, Update
the anti-wrinkle throttle-bypass condition in the detector logic around
is_anti_wrinkle_pulse to compare power against the same effective exit threshold
used by the idle-timer reset: max(anti_wrinkle_exit_power, stop_threshold_w),
rather than min_power. Add a regression case covering an anti-wrinkle pulse
below min_power that still meets this effective threshold and must bypass
throttling.
fd255ab to
94fa228
Compare
The anti-wrinkle idle-timeout (anti_wrinkle_idle_timeout) and the 2 h safety cap both live in the detector and only advance from within process_reading, so they need incoming readings to fire. A publish-on-change power sensor goes completely silent once power flatlines at standby / 0 W after the last tumble pulse, which freezes the idle timer mid-count and pins the state in anti_wrinkle until the next real reading (typically the next cycle). Nothing was advancing the timer during that silence. The watchdog is stopped for the whole anti-wrinkle tail (anti_wrinkle is entered via _finish_cycle, whose on_cycle_end callback calls _stop_watchdog, and the watchdog is only restarted on the next cycle start), and the state-expiry timer explicitly skipped anti_wrinkle. Drive the keepalive from the state-expiry timer instead, which does keep ticking through the tail: on silence longer than off_delay it injects a 0 W keepalive, letting the detector's own idle-timeout / 2 h safety close the tail into OFF. Gate the keepalive on _last_real_reading_time (only genuine sensor readings bump it), not _last_reading_time which the keepalive itself bumps: with off_delay == the 60 s state-expiry interval, gating on the self-bumped clock made the condition true only every other tick, so the idle timer advanced at half real-time and took ~2x as long to close. Real-silence gating fires a keepalive every tick during genuine silence, so the tail closes right at anti_wrinkle_idle_timeout after the last pulse. The synthetic keepalive bumps _last_reading_time, which is also the sampling-throttle clock in _async_power_changed. A real tumble pulse (>= min_power, therefore not is_low_power) arriving within one sampling interval of a keepalive would be discarded by that throttle before reaching the detector, so it could not reset the idle timer. Exempt high-power readings from the throttle while in anti_wrinkle; sub-min baseline readings stay throttled as before. Tests in tests/test_anti_wrinkle_silent_close.py cover: keepalive injection during anti-wrinkle silence, gating on real-silence (not the self-bumped clock), no injection before off_delay, no-op before first reading, end-to-end close into OFF under injected keepalives, a pulse resets the idle timer, and the throttle bypass for real pulses. Closes 3dg1luk43#339 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
94fa228 to
57f1be9
Compare
The throttle bypass for real tumble pulses required power >= min_power, but the detector resets its anti-wrinkle idle timer for readings at or above effective_exit = max(anti_wrinkle_exit_power, stop_threshold_w). When that threshold is below min_power, a real pulse in [effective_exit, min_power) resets the detector's idle timer yet was throttled here, so a keepalive- adjacent pulse could be dropped and the mode could time out mid-tumble. Gate the bypass on effective_exit instead. Adds regression tests for a pulse below min_power (must bypass) and a reading below effective_exit (must stay throttled). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Good catch — addressed in 626e62b. The throttle bypass now gates on the same threshold the detector uses to reset its anti-wrinkle idle timer: anti_wrinkle_exit_threshold = max(
float(self.detector.config.anti_wrinkle_exit_power),
float(self.detector.config.stop_threshold_w),
)
is_anti_wrinkle_pulse = (
self.detector.state == STATE_ANTI_WRINKLE
and power >= anti_wrinkle_exit_threshold
)So when Added two regression tests:
Full file green (11 passed). |
Carries upstream PR 3dg1luk43#340 by Maximilian Schmidt (@sharkyy) for issue 3dg1luk43#339, cherry-picked ahead of review so the fix can be validated on real hardware while the maintainer is on summer break. Commits 57f1be9 and 626e62b only; the unrelated startup/notify commit on the same branch was left out. Verified before the pick: with the fix reverted, 4 of the 11 tests in tests/test_anti_wrinkle_silent_close.py fail, so they do catch the defect. With it: 11 passed, full suite 1387 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…main (fork build 0.5.3.6)
1 participant
Linked Accepted Issue (required for non-translation PRs)
Closes #339
Description
The detector enters
anti_wrinkleat the end of a dryer cycle and never leaves it when the power sensor is publish-on-change (Shelly/Zigbee/etc.). On a recorded run the state enteredanti_wrinkleat 16:29 and was stillanti_wrinkleat 18:18 — long after the dryer physically finished.The anti-wrinkle idle-timeout (
anti_wrinkle_idle_timeout) and the 2-hour safety cap both live inside the detector and only advance from withinprocess_reading, so they need incoming readings. A publish-on-change sensor goes silent once power flatlines at standby / 0 W after the last tumble pulse, so the idle timer freezes mid-count.Nothing was advancing the timer during that silence. As pointed out in review, the watchdog cannot help: it is stopped for the whole anti-wrinkle tail — anti-wrinkle is entered via
_finish_cycle, whoseon_cycle_endcallback calls_stop_watchdog(), and it is only restarted on the next cycle start. The state-expiry timer, however, keeps ticking through the tail (it is armed by the cycle-end tail via_start_state_expiry_timer, and_cycle_completed_timeis set), but it explicitlyreturned early foranti_wrinkle.This PR drives the keepalive from the state-expiry timer instead. Once the sensor has been silent longer than
off_delayit injects a 0 W keepalive, letting the detector's own idle-timeout (or the 2-hour safety cap) close the tail intooff.Two important details:
_last_real_reading_time(only genuine sensor readings bump it), not_last_reading_timewhich the keepalive itself bumps. Becauseoff_delayequals the 60 s state-expiry interval, gating on the self-bumped clock would make the condition true only every other tick, so the idle timer would advance at half real-time and take ~2× as long to close. Real-silence gating fires a keepalive every tick during genuine silence, so the tail closes right atanti_wrinkle_idle_timeoutafter the last tumble pulse._last_reading_time, which is also the sampling-throttle clock in_async_power_changed. A real tumble pulse (>= min_power, therefore notis_low_power) arriving within one sampling interval of a keepalive would otherwise be discarded by that throttle before reaching the detector and could not reset the idle timer, so high-power readings are exempted from the throttle while inanti_wrinkle. Sub-min_powerbaseline readings stay throttled as before.Validated live on a real dryer with a publish-on-change plug: the tail now closes into
offon schedule instead of hanging.Type of Change
Changes Made
manager.py_handle_state_expiry: inject a 0 W keepalive duringanti_wrinkleon real sensor silence (>off_delay), gated on_last_real_reading_time, so the detector's idle-timeout / 2 h safety closes the tail intooffmanager.py_async_power_changed: real anti-wrinkle tumble pulses (>= min_power) bypass the sampling throttle so a recent keepalive cannot suppress themmanager.py_watchdog_check_stuck_cycle: reverted to its original form (the previous, non-working watchdog branch removed)tests/test_anti_wrinkle_silent_close.py: new regression testsCHANGELOG.md: entry under the existing 0.5.4 sectionTesting
./run_tests.sh(viapytest tests/)New tests:
_handle_state_expiryinjects a keepalive during anti-wrinkle silence; it gates on real-silence (fires even when_last_reading_timewas just self-bumped); no injection beforeoff_delay; no-op before the first reading; the real detector closes anti-wrinkle intooffunder injected keepalives once the idle-timeout elapses; a real tumble pulse resets the idle timer; the throttle bypass admits real pulses while RUNNING high readings and anti-wrinkle baselines stay throttled.Full fast suite green: 1365 passed (only
test_mock_socket_synthesis.pyskipped — missing optionalniceguidevtools dependency, unrelated to this change).Tested on:
Breaking Changes?
Checklist
python3 -m compileall custom_components testspasses./run_tests.shand all tests passstrings.jsonandtranslations/