fix: harden SI doors, HA discovery slugs, and charging identities - #970
Conversation
Stop NaN/overflow SoC from leaking as percent or Inf at the units, app, and HA doors. Slug MQTT discovery object ids so driver names with spaces or commas are not silently dropped. Store synthetic history SoC as 0–1. Lock leftover-PV vs battery-grid-buy math in a tested sitepower identity without rewriting files owned by open planner/dispatch drafts. Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
The brand-cleanup gate treats new "forty_two_watts" copy as unclassified. The illegal-topic repro is the driver name, not the box id. Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 454e8a1b45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, r := range strings.ToLower(name) { | ||
| ok := (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' || r == '_' |
There was a problem hiding this comment.
When two valid configured driver names normalize identically, such as Garage and garage or Laddare, Garage and Laddare Garage, this lossy slug assigns both drivers the same discovery IDs and state topics. Their Home Assistant entities overwrite one another and subsequent state publications become indistinguishable; include a stable collision-resistant component instead of using the slug as the identity.
Useful? React with 👍 / 👎.
| func (b *Bridge) driverUniqueID(driver, suffix string) string { | ||
| return b.deviceID + "_" + mqttObjectID(driver) + suffix | ||
| } |
There was a problem hiding this comment.
For an existing driver whose name is already legal but contains uppercase characters, such as the test's Ev-Charger_1, this changes its unique_id and discovery topic on upgrade even though the declared allowed set includes uppercase. Because discovery configs are retained and the old topic is never removed, Home Assistant keeps the old entity while registering a new lowercase one, leaving users with duplicate and stale entities; preserve the original identifiers for already-legal names or explicitly migrate/remove the retained topics.
Useful? React with 👍 / 👎.
Leave already-legal MQTT object ids, including mixed case, unchanged so Home Assistant does not keep a retained Ev-Charger_1 entity next to a new lowercase copy. Illegal names keep a readable stem and append an FNV-1a tag of the original string so "Laddare, Garage" and "Laddare Garage" do not share a topic. Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
2 participants
Accepted text proposal
Issue or Discussion: maintainer request to harden charging/units math against edge cases after
v2.2.1-beta.1. Also covers #960 (HA MQTT illegal discovery topics).Maintainer comment that accepted this scope: start a broad hardening pass on tests, edge cases, units, charging logic, and strange math errors.
What changed
go/internal/units):PercentFromFraction/ permille fold NaN/Inf/overflow onto[0,1]/[0,100]/[0,1000].CanonicalPowerEnergyis case-insensitive and zeros non-finite values.FractionFromLegacyPercenttreats(1, 2)as 0–1 overflow (BMS 1.02), not as 1.02%. Values≥2still divide by 100;150stays1.5soValidFractioncan reject it.[A-Za-z0-9_-]+. Illegal names keep a readable stem and append an FNV-1a tag of the original string (Laddare, Garage→Laddare_Garage_57d42421) so comma vs space do not collide. Already-legal names, including mixed case (Ev-Charger_1), are returned unchanged so Home Assistant does not keep a retained entity next to a new lowercase copy. Friendlynameis unchanged. SoC publishes throughPercentFromFraction.PermilleFromFraction(NaN → 0 permille). Boost lease args clamp after the legacy-percent door.bat_socis a 0–1 fraction (was 0–100, so the dashboard showed ~5000% after-backfill).go/internal/sitepower): tested site-boundary math:grid = load + pv + battery + ev + v2xmax(0, −(load+pv))gridW > 50is not the surplus-only import test (that is the fix(mpc): surplus-only EV can take leftover PV while the home battery grid-charges #957 live-formula leak)ValidateReading(percent SoC, NaN, +Inf, 1.02), energy ledger later export does not shrink import, battery-boost JSON round-trip.Why
Post-SI-core, several doors still treated overflow as percent (
1.02→1.02%), published NaN, stored backfill SoC as 0–100, and built HA discovery topics from raw YAML names. Surplus-only EV vs leftover PV is easy to get wrong if you look at the meter sign.Codex P2s on
mqttObjectID(commit454e8a1b): lowercase slugging changed already-legal mixed-case unique_ids, and comma vs space collapsed onto one slug.121570cakeeps legal ids stable and tags illegal names with FNV-1a of the original string.Boundaries and safety
Out of scope on purpose — open PRs own those files:
main.go,config.go, planner prefs / UImpc.go, loadpoint controller /surplus_reserve.godispatch.gositepoweris the identity those drafts should adopt. Production DP still usesevW > 0 && gridW > 50. Idle-hybrid deadband (#822), driver-YAMLmax_charge_w: 0→ 5 kW (main.go), and the<1000kWp-vs-watts heuristic are documented, not “fixed under” those PRs.Stale telemetry, planner output, and driver default-mode paths are unchanged. No UI files.
Verification
gofmton every edited Go filego test ./internal/units ./internal/sitepower ./internal/devtools ./internal/ha ./internal/appproto ./internal/loadpoint ./internal/telemetry ./internal/state ./internal/api— passgo test ./internal/control ./internal/mpc ./internal/calendar ./internal/config— passgo test ./internal/haafter121570ca— pass (legal mixed-case ids unchanged; comma vs space distinct)5a3c5a48: core, full stack,go test + vetgreen;no new legacy product copyfailed because the HA fixture copiedforty_two_watts. Fixture now usessite_box.drivers/*.luasnapshot (pre-existing, not this diff)Checklist