Repository navigation
fix(hanchu): address code review findings from #5672 - #5674
Closed
springfall2008 wants to merge 1 commit into
Closed
springfall2008 wants to merge 1 commit into
springfall2008 wants to merge 1 commit into
Conversation
- Power ceiling: raise the menu's 0-5000 W range to the highest value the device has ever been seen holding (persisted with the config cache), not the value held now - Predbat's own write overwrote it, so the 6 kW raise undid itself after the first write (finding 1). - Drift: compare each control read-back with the applied payload and forget it on a mismatch, so a change made in the Hanchu app, a device reset or a lost write is re-sent (finding 2). - HTTP 401: re-authenticate on the status before the body's shape is checked, so an empty or HTML 401 body no longer skips re-auth (finding 3). - Units: the below-10-means-kW heuristic applies only to fields with no unit sibling (loadPwr), not to grid and battery power (finding 7). - Hold: decide the zero charge rate after the hold slot is placed, so a hold with no room leaves the plan's charge rate alone (finding 8). - Midnight: clamp a 24:00:00 end to 23:59 so TCT_END is never 86400 (finding 9). - Logging: the API trace is now hanchu_api_debug (default off), and "settings unchanged" is only logged at debug level with the trace on (finding 10). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
springfall2008
commented
Oct 21, 2026
springfall2008
left a comment
Owner
Author
There was a problem hiding this comment.
Automated comment from the triage bot.
Reviewed head 48254ca. No issues found — this review has no inline comments.
What I checked, all by reading the checked-out head (no code executed):
- Finding 1 (
ranges_forraise): the ceiling is nowmax(currently held, highest ever held)per serial and key, noted by_note_power_ceilingon every successful iotGet, persisted in the config cache and seeded on restore fromdevice_settingswhen the cache predates it. Traced the round-trip (save_configJSON-safe,restore_staterebuild, seeding loop is a no-op on a fresh cache) and the_clamp_loggedlog-once key.battery_rate_max/inverter_limit/reserve_maxall read the raised bound through the sameranges_forpath. GitNexus impact onranges_foris confined tohanchu.pyand its tests, matching the PR body's grep claim. - Finding 2 (drift detection): traced
_note_external_change's gate — comparisons start only whenlast_write_timeis at leastHANCHU_READBACK_SETTLE(60 s) before the read began, or absent; a write that started after the read began also skips, so a slow cloud round-trip cannot sample across an in-flight write. Keys the read-back does not carry are excluded beforesettings_mismatchesruns (matching the PR body), and the numeric compare handles iotGet's string/float forms.applied_payloadis popped only here and reassigned only on an accepted write; the next reconcile re-sends the plan. The reconcile-after-drift and drift-vs-pacing interplay both check out. - Finding 3 (401 handling): the HTTP-401 branch now precedes the non-object check and swallows a
ValueErrorfrom parsing (content_type=Nonemeans only decode errors are possible there). Verified both an empty and an HTML 401 body reachreauthenticate(), the retried call carries the new token, and 200s with un-parseable bodies still fall through to the transport-retry path as before. - Finding 7 (watt heuristic):
threshold=0is applied only to the five fields that have a unit sibling inHANCHU_TELEMETRY_UNIT_FIELD;loadPwrkeeps the original heuristic. Consistent with the finding's own fix scope (grid, battery and every other sibling-tagged field), including theac_pv_power/generator_powerleaves. - Finding 8 (hold with no room): the slot-3 decision now runs before the power block, and clearing
signalthere leavesCHG_PWR_LMTat the plan's rate. Traced_extra_slot's return-None paths and confirmed the in-window target-met hold (("hold", None)) still zeroes the charge rate as before. - Finding 9 (first half): the
<=→<clamp sends a24:00:00end as 86340 with slot 2 disabled; the only othersplit_window_secondscaller (_slot_pairs) is unaffected for sub-midnight ends and the existing 26:00 test still passes as written. Re-checked that nothing else pins the old 86400 value. - Finding 10 (trace opt-in):
hanchu_api_debugis in the components registry,APPS_SCHEMAand the template, consumed throughinitializeby arg name like its siblings; the CLI harness override (not args.no_trace) matches the body. The two tests that exercised trace-on logs now enable it explicitly, the "unchanged" line readsDebug:(a prefix deye.py/fox.py already use) and no other test relied on the trace being on. - Every new test traced red against the pre-fix behaviour by reading it (the fix reverted → the assertion it asserts fails), per the PR body's claim; both new suites sit inside
TEST_REGISTRY-registered modules; per-test instances mean no shared-fixture state leaks.
Two observations, deliberately not posted as findings:
apps/predbat/hanchu.py:830onwards: the persistedpower_ceilingonly ever rises — a junk read would pin a too-large ceiling until the cache is cleared. That is exactly the design #5672's suggested fix asked for and the device-side rejection path (write failure counts,HANCHU_WRITE_FAILURE_NOTIFY) does surface it, so I am recording it as an accepted tradeoff rather than a defect.hanchu_api_debugis documented only intemplates/hanchu.yaml. That follows this component's existing convention (it has no section indocs/components.md, unlike the other cloud components — pre-existing from #5305, not created here).
This was referenced Oct 21, 2026
Closed
Owner
Author
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
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.
Fixes the accepted findings from the review of the Hanchu ESS cloud component in #5672. Findings 1, 2, 3 and 10 needed fixing before a release ships
hanchu.py.Fixed
ranges_for()now raises the menu's 0–5000 W ceiling to the highest value ever seen per device and key (power_ceiling). That value is saved with the config cache. An older cache without it is seeded from its cacheddevice_settings.refresh_config()compares each iotGet read-back withapplied_payload, using the same numeric comparison as the CLI'ssettings_mismatches. On a mismatch it logs a warning naming each key and forgets the applied payload, so the next reconcile sends it again. This follows thealphaess.pynote_external_changepattern. Keys missing from the read-back are not counted as drift. A read that started less thanHANCHU_READBACK_SETTLE(60 s) after a write, or before it, is not checked.Noneand is not retried as a transport failure.loadPwr). A grid or battery reading whose unit is missing is read as watts.CHG_PWR_LMT=0is still written when no hold slot can be placed24:00:00end givesTCT_END_1=86400split_window_seconds()now clamps an end of exactly 24:00 to 23:59, ashhmmss_to_seconds()does.api_debug = Trueis hard-codedhanchu_api_debugoption (defaultFalse), added to the registry,APPS_SCHEMAandtemplates/hanchu.yaml. "settings unchanged" now logs atDebug:and only while the trace is on. The CLI harness still traces by default (--no-traceturns it off).Not in this PR
alphaess.py,fox.py,deye.pyandsunsynk.pyread their schedule entities the same way. If deployments without retained entity state confirm it, it should be fixed in all of them together, not only in Hanchu.can_span_midnightis False, but it works, and removing it is outside a minimal fix.Tests
New tests for findings 1, 2, 3, 7, 8 and 10 each failed before the fix, for the reason the finding gives. Finding 9 updates an existing assertion that pinned the 86400 value.
test_hanchu_password_is_never_loggednow turns the trace on explicitly, because tracing is opt-in../run_all --quickpasses (4 slow tests skipped) and pre-commit is clean on the changed files.GitNexus impact analysis couldn't run: the local index predates
hanchu.py. Grep shows every changed symbol is used only byhanchu.py,hanchu_const.pyand their tests.🤖 Generated with Claude Code