feat(heishamon): report electrical power, and the outdoor temperature under its canonical name - #60
feat(heishamon): report electrical power, and the outdoor temperature under its canonical name#60frahlg wants to merge 2 commits into
Conversation
… under its canonical name FTW's heating view discovers a heat pump by the hp_power_w metric and shows nothing at all without it — no chart, no reading, no entry. So a driver verified against real hardware in June had no page in the product it was written for. hp_power_w now comes from main/Heat_Power_Consumption, which config power_topic overrides for a Heishamon build that names the topic differently. It is emitted only once that topic has arrived, so a wrong name costs the power reading and leaves every other metric alone. hp_outside_temp_c becomes hp_outdoor_temp_c, the name nibe_local and myuplink report and the one the view charts. Nothing read the old name. History under it stays where it is and stops growing. The DRIVER version said 0.4.0 while the manifest said 0.5.0; make bump-driver takes both to 0.6.0. The example config's MQTT password looked real enough to be one, and is now a placeholder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
There was a problem hiding this comment.
Automated maintenance pass: not merging this one.
The driver-side work looks sound — power_topic is a config-overridable, file-local-concatenated topic (compliant with #59's rewritten test_subscription_topics_are_static), the metric only emits once the topic has actually arrived so a wrong name costs just that one reading, the version drift between DRIVER (0.4.0) and the manifest (0.5.0) is a legitimate catch, and hp_outside_temp_c → hp_outdoor_temp_c lines up with what nibe_local/myuplink already report. drivers/packages/public-boundary/history-secret-scan/dco checks are green; drivers was still in progress at review time.
Not merging because the PR body already says it itself: "Needs a check against hardware before merge", and asks for confirmation that Heat_Power_Consumption is the right topic name on a real build. That's exactly the kind of hardware-correctness question this pass is supposed to defer to a human on rather than resolve by assumption — seconding it here rather than merging past it.
Also blocked mechanically until then: this PR is based on #59's branch, not main, and #59 itself is approved but couldn't be merged via API (stacked-PR restriction — needs the web UI). Once a maintainer merges #59 and the topic name is confirmed against hardware, this should be quick to land.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c151414fd
ℹ️ 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".
|
Automated maintenance pass: not merging this one. The change itself looks correct — But the author's own PR description says it plainly: "Needs a check against hardware before merge." Also note: this PR is stacked on #59 (also reviewed this pass, CI green, ready but blocked from auto-merge by GitHub's stacked-PR API restriction). #59 needs to land first; the GitHub API wouldn't even let me retarget this PR's base to Generated by Claude Code |
|
смотри внимательней кого отмечаешь в пулреквестах |
|
Sorry — I tagged the wrong person. I’ve removed the mentions from the PR and earlier comments. |
miravoss26
left a comment
There was a problem hiding this comment.
Reviewed the diff. Adds hp_power_w (from main/Heat_Power_Consumption, overridable via power_topic) and renames hp_outside_temp_c → hp_outdoor_temp_c to match nibe_local/myuplink and what FTW's heating view reads. Version drift 0.4.0/0.5.0 → 0.6.0.
- The one behaviour change worth a second look is the metric rename. The old
hp_outside_temp_cappears read by nobody and existing history stops growing under the old key;hp_outdoor_temp_cis what the view charts, so this is what makes the pump show up. I found no remaining consumer of the old name, worth a glance that no dashboard/export still keys on it. hp_power_wis emitted only oncepower_topichas arrived, so a wrong topic name costs the power reading and nothing else. Safe default.- Nice catch swapping the real-looking MQTT password in the example config for a placeholder.
No authz/network changes, driver checks green. Safe to merge from my read once #59 lands and this rebases (currently CONFLICTING on the stack). Human merges, not in my allowlist.
|
Automated repo maintenance pass (device-driver PR review). Flagging for the record before this merges: the PR description itself notes Everything else here looks solid: version bump 0.5.0 → 0.6.0 is correctly scoped as a minor (additive metric + safe rename with no breaking change), the Generated by Claude Code |
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
|
Updated the PR head to fd4dddd.
I resolved the review thread. I am still not merging this PR: the default Heat_Power_Consumption topic needs confirmation from a live Heishamon build. That is now the remaining product gate. |
miravoss26
left a comment
There was a problem hiding this comment.
Adds a hp_power_w metric to the heishamon driver (from main/Heat_Power_Consumption, overridable via new power_topic config) and renames hp_outside_temp_c → hp_outdoor_temp_c to match nibe_local/myuplink's naming — both needed because FTW's heating view discovers a heat pump by hp_power_w and charts the outdoor-temp metric under the new name; without this the driver had no page at all despite being hardware-verified.
Read through the Lua: power staleness is tracked separately from the other metrics' last_msg_ts (a fresh unrelated topic doesn't resurrect a stale power reading), and the new freshness test (test_heishamon_power_freshness.lua) exercises exactly that. Old example-config MQTT password replaced with a placeholder — good catch, that looked like a real credential. index.yaml/manifest sha256 + size bump match the driver's actual diff size. Dependency PR #59 is already merged, so no blocker there.
Two things for a human before merge, not code issues:
mergeable: CONFLICTING— needs a rebase against current base before it can land.- The author flags this as unverified against real Heishamon hardware —
Heat_Power_Consumptionis TOP21 on the builds they could find named, but they have no pump to confirm the topic name against. If it's wrong, the fix is just apower_topicconfig override (logged at init), not a driver rewrite — low blast radius, but still needs someone with hardware to confirm.
Needs a human decision on the hardware check + a rebase; not safe to auto-merge as-is.
Based on #59 — merge that first. Answers the driver-visibility half of srcfl/ftw#520.
FTW's heating view discovers a heat pump by the
hp_power_wmetric (web/heating.js:172) and shows nothing at all without it: no chart, no reading, no entry. So a driver verified against real hardware in June has had no page in the product it was written for.hp_power_wnow comes frommain/Heat_Power_Consumption, which the newpower_topicconfig key overrides for a Heishamon build that names the topic differently. It is emitted only once that topic has arrived, so a wrong name costs the power reading and leaves every other metric alone.hp_outside_temp_cbecomeshp_outdoor_temp_c— the name nibe_local and myuplink report, and the one the view charts. Nothing read the old name. Existing history under it stays where it is and stops growing.Also: the
DRIVERversion said 0.4.0 while the manifest said 0.5.0, and nothing outsidemake bump-drivercompares them; both now say 0.6.0. The example config carried what reads like a real MQTT password, now a placeholder.make checkgreen.make test-driver ID=heishamon: 36 passed, 14 skipped.Needs a check against hardware before merge.
Heat_Power_Consumptionis TOP21 on the Heishamon builds I could find named, but I have no pump to confirm it against. If the hardware names it differently, the driver takespower_topicand the default should change to whatever it reports. Thepower_topic=value is logged at init.This does not give the pump a node in Live Energy Balance, or a control in the UI. Both need FTW Core changes and stay in srcfl/ftw#520.
🤖 Generated with Claude Code