Skip to content

fix(hanchu): address code review findings from #5672 - #5674

Closed
springfall2008 wants to merge 1 commit into
mainfrom
fix/hanchu-review-5672
Closed

springfall2008 wants to merge 1 commit into
mainfrom
fix/hanchu-review-5672

Conversation

@springfall2008

Copy link
Copy Markdown
Owner

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

# Finding Change
1 The 6 kW power-ceiling raise undoes itself 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 cached device_settings.
2 Changes made on the device are never corrected refresh_config() compares each iotGet read-back with applied_payload, using the same numeric comparison as the CLI's settings_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 the alphaess.py note_external_change pattern. Keys missing from the read-back are not counted as drift. A read that started less than HANCHU_READBACK_SETTLE (60 s) after a write, or before it, is not checked.
3 An HTTP 401 with an empty or non-JSON body never re-authenticates The 401 check now runs before the non-object check. A 401 body that fails to parse becomes None and is not retried as a transport failure.
7 The "magnitude < 10 means kW" rule applies to every power field The heuristic now applies only to fields with no unit sibling (loadPwr). A grid or battery reading whose unit is missing is read as watts.
8 CHG_PWR_LMT=0 is still written when no hold slot can be placed The charge power is now computed after the extra-slot decision.
9 A 24:00:00 end gives TCT_END_1=86400 split_window_seconds() now clamps an end of exactly 24:00 to 23:59, as hhmmss_to_seconds() does.
10 api_debug = True is hard-coded New hanchu_api_debug option (default False), added to the registry, APPS_SCHEMA and templates/hanchu.yaml. "settings unchanged" now logs at Debug: and only while the trace is on. The CLI harness still traces by default (--no-trace turns it off).

Not in this PR

  • 4, the hold slot may block PV charging. Not fixed: it depends on the sunshine hardware test running now, and the review proposes no fix yet.
  • 5, the export floor is applied only once the window is active. Not fixed: it is also waiting on hardware. The suggested fix (write the export target as the floor whenever an export window is enabled) also changes behaviour before the window. A 16:00 export with a 40% target would stop the battery covering the house below 40% from the moment the window was written. Predbat's plan expects self-use down to the reserve until 16:00. This needs a decision, not just a code change.
  • 6, a restart can write an all-disabled schedule. Not fixed: it is still unconfirmed, and alphaess.py, fox.py, deye.py and sunsynk.py read 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.
  • 9 (second half). I kept the slot-2 midnight split rather than removing it. It can't be reached while can_span_midnight is 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_logged now turns the trace on explicitly, because tracing is opt-in. ./run_all --quick passes (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 by hanchu.py, hanchu_const.py and their tests.

🤖 Generated with Claude Code

- 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>
@github-actions github-actions Bot added the BOT_REVIEW Trigger an autotriage label Oct 21, 2026

@springfall2008 springfall2008 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_for raise): the ceiling is now max(currently held, highest ever held) per serial and key, noted by _note_power_ceiling on every successful iotGet, persisted in the config cache and seeded on restore from device_settings when the cache predates it. Traced the round-trip (save_config JSON-safe, restore_state rebuild, seeding loop is a no-op on a fresh cache) and the _clamp_logged log-once key. battery_rate_max/inverter_limit/reserve_max all read the raised bound through the same ranges_for path. GitNexus impact on ranges_for is confined to hanchu.py and its tests, matching the PR body's grep claim.
  • Finding 2 (drift detection): traced _note_external_change's gate — comparisons start only when last_write_time is at least HANCHU_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 before settings_mismatches runs (matching the PR body), and the numeric compare handles iotGet's string/float forms. applied_payload is 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 ValueError from parsing (content_type=None means only decode errors are possible there). Verified both an empty and an HTML 401 body reach reauthenticate(), 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=0 is applied only to the five fields that have a unit sibling in HANCHU_TELEMETRY_UNIT_FIELD; loadPwr keeps the original heuristic. Consistent with the finding's own fix scope (grid, battery and every other sibling-tagged field), including the ac_pv_power/generator_power leaves.
  • Finding 8 (hold with no room): the slot-3 decision now runs before the power block, and clearing signal there leaves CHG_PWR_LMT at 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 a 24:00:00 end as 86340 with slot 2 disabled; the only other split_window_seconds caller (_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_debug is in the components registry, APPS_SCHEMA and the template, consumed through initialize by 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 reads Debug: (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:830 onwards: the persisted power_ceiling only 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_debug is documented only in templates/hanchu.yaml. That follows this component's existing convention (it has no section in docs/components.md, unlike the other cloud components — pre-existing from #5305, not created here).

@springfall2008

Copy link
Copy Markdown
Owner Author

Superseded by #5684, which combines #5682, #5674 and #5680 into one PR for review. This PR's commits are in it unchanged, in merge order. The branch is kept, so this can be reopened if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant