Repository navigation
feat(discovery): Fox Cloud reports to the discovery catalogue - #5184
Merged
Merged
Conversation
…ithhold a report Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…orter Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ter it Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FoxAPI.run() called refresh_discovery() after automatic_config(). With fox_automatic on and no device automatic_config() can drive - a battery with no scheduler - automatic_config() raises out of run() on every retry, so the catalogue never described the very installs whose dump most needs to say why. The call now sits just above the auto-config step, still after the device poll and publish_data(), matching GivTCP. Only the call's position changes: refresh_discovery() never raises and automatic_config() reads nothing it writes, so run()'s control flow, return value and exception are as before. The new wiring test drives the real automatic_config() through two first=True retries and checks both that the ValueError still escapes and that a report was filed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
battery_entries was len(batteryList), which cannot tell the GH#4919 AIO from a healthy system: a real list mixes bcu/ivu control units carrying no capacity with bmu entries carrying one in Wh, so one KH pack reads 3, and the AIO (a bcu plus four bmu, each claiming the whole pack under the inverter's own serial) reads 5 - the same as a genuine four-module stack. It becomes two ratings named for what the API returned: battery_capacity_entries (the entries publish_data() sums) and battery_capacity_serials (distinct batterySN among them). Four against one is the bug's signature; a healthy four-module stack reads four/four. inverter_type "FoxCloud" was gated on hasBattery alone, so a battery with no scheduler was reported as an inverter while automatic_config() raised for it. It now uses automatic_config()'s own predicate - hasBattery and function.scheduler and capacity > 0 - duplicated rather than shared so the control path is untouched, and pinned by a test that runs the real automatic_config() on each case. The fixture is rebuilt from Fox's own samples (get_device_list() and get_device_detail() docstrings, Wh capacities, type and batterySN), with the AIO case taken from GH#4919's reported API response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The capability token "scheduler" becomes the spec's "schedule"; third_party_gen moves from capabilities to flags, since a third-party generator the inverter meters is topology, not a capability. - info["firmware"] carries Fox's per-board versions (manager, master, slave) flattened into one string, as GE Cloud flattens its firmware_version dict. - A round-trip test pushes a realistic report (thirdPartyGen set, the GH#4919 AIO alongside) through validate_report() and requires every record back unchanged, with nothing logged as dropped. - The docstring no longer claims inverter_w matches the _inverter_capacity sensor for PV-only devices (publish_data() sets it to 0 there), and says why stationName, stationID and moduleSN are left out; a test asserts none of them reaches the report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ities The programmes note called Fox a "v1" reporter, but v1 was the spec's original five. And the inverters row said every record carries its entities, when only a reporter with a Predbat-control-to-entity table - GivTCP today - builds that map; GE Cloud and Fox records carry none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Most of the Fox review's findings traced back to this plan's text, so: - Global Constraints gain the reporter rules it surfaced: call refresh_discovery() before automatic_config(); set inverter_type with automatic_config()'s own predicate; fixtures from real samples, units included; evidence numbers must distinguish the cases they claim to; a validate_report() round trip per reporter; site names never in info; the spec's capability tokens; firmware in info; comments cite symbols, not line numbers. - A dated Amendments entry records each change and why, including the corrected reasoning for Fox and GE Cloud carrying no entity map. - Task 4's Interfaces line no longer claims it consumes discovery_entities(), and its Background, Step 5 placement, mutation checks, code block and test block are regenerated from the code as it stands. - Subsequent plans record that entity maps are a decision needed before Plan 2, and the vocabulary note uses Fox's current tokens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the two unresolved moderate findings in apps/predbat/fox.py.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds Fox Cloud hardware reporting to the discovery catalogue and improves mixed-type set sorting.
Changes:
- Adds Fox discovery records, capabilities, firmware, and battery diagnostics.
- Wires discovery reporting into Fox runtime cycles.
- Adds tests and updates documentation and rollout plans.
- Safely sorts mixed-type sets in
inverter_record().
| File | Summary |
|---|---|
docs/superpowers/plans/2026-09-20-discovery-reporter-rollout.md |
Updated rollout plan and reporter guidance. |
docs/discovery-catalogue.md |
Documented discovery reporting and builder behavior. |
apps/predbat/tests/test_fox_api.py |
Added Fox discovery and lifecycle coverage. |
apps/predbat/tests/test_coordinator.py |
Added mixed-type sorting regression coverage. |
apps/predbat/fox.py |
Added Fox reporter and runtime wiring. Moderate finding (1 vote): match ExportLimit case-insensitively. Moderate finding (3 votes): validate numeric capacity before calling capacity_watts(). |
apps/predbat/coordinator.py |
Improved safe normalization of mixed-type sets. |
.cspell/custom-dictionary-workspace.txt |
Added fixture vocabulary. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…as automatic_config() does Addresses the Copilot review on #5184: - Only a positive int or float capacity reaches capacity_watts(), the same test drives_it already applied. A string capacity used to raise inside build_discovery() and lose Fox's whole report; now it drops the rating only. - export_limit is matched case-insensitively, as automatic_config() does when it sets hasExportLimit, so the copied test stays identical to its source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
What this does
Adds Fox Cloud as a reporter to the discovery catalogue. It is the first of the Phase 1 vendor reporters and the first built from the start on the shared
inverter_record()builder (#5179). Previously the catalogue could describe GivEnergy hardware only.It also fixes the builder: sets are now sorted with
key=str, so a set mixing types can no longer raise and lose a reporter's whole report.Observe-only
Fox's behaviour is unchanged.
build_discovery()only reads the dataautomatic_config()already collects —device_list,device_detailanddevice_settings— so there are no new API calls.The one change to
FoxAPI.run()is a single synchronousself.refresh_discovery()call, which catches and logs its own failures.automatic_config()still runs exactly when it did and still raises exactly as it did.Why the reporter looks the way it does
Review found three problems in the first version. All three came from the plan, not the implementation, and each explains a design choice here:
automatic_config(). Fox'sautomatic_config()raises when no battery has a scheduler, so calling the reporter after it meant Fox never reported on exactly the installs that most need a dump. GivTCP already reports before its auto-config.batteryListmixes control units that have no capacity (bcu,ivu) withbmuentries that report capacity in Wh, so a raw entry count reads 3 for a single pack. The GH#4919 AIO fault (fourbmuentries sharing the inverter's serial) and a healthy four-module stack both have five entries, so no single count can tell them apart. The reporter therefore givesbattery_capacity_entries(the entriespublish_data()sums) andbattery_capacity_serials(the distinct serials among them). Four entries against one serial is the GH#4919 signature. It deliberately does not report a capacity figure, because the summed one is known to be wrong.inverter_type: FoxCloudonly whereautomatic_config()would drive the device. That means the same test it uses:hasBatteryand a scheduler andcapacity > 0. The test is copied into the reporter rather than shared, so the control path isn't touched.The capability token is the spec's
schedule.third_party_genis a flag, not a capability. Firmware is ininfo.stationNameis deliberately left out, since it is free text that can hold a street address.No entity map, deliberately
Only GivTCP reports an entity map today. Fox's
automatic_config()already maps Predbat's controls to Fox entities, so adding a map means moving that table into a shared constant used by bothautomatic_config()and the reporter. That would touch a control path, and this rollout is observe-only. Entity maps will be their own piece of work.Testing
fox.py, with real units. The GH#4919 case is included, and a healthy multi-pack case is derived from it, since the repo has no real multi-pack capture.FoxAPI.run()across cycles, including one whereautomatic_config()raises and a report must still be filed.validate_report()and checks nothing is dropped.Review fixes
capacitythat isn't a positive number now drops only theinverter_wrating. Before, a string capacity madecapacity_watts()raise and lost Fox's whole report.export_limitis matched case-insensitively, the same wayautomatic_config()setshasExportLimit.Next
The rollout plan, amended in this PR, now lists nine rules for the remaining reporters, all learnt from this review: when to file, how to derive
inverter_type, using real fixtures, making counts that can tell cases apart, round-trip tests, keeping site names out, using spec vocabulary, firmware placement, and referring to symbols rather than line numbers. AlphaESS, Solis and Deye/Sunsynk follow in the next plan.🤖 Generated with Claude Code