Skip to content

feat(heishamon): declare the heat curve offset as a control a host can render - #66

Open
frahlg wants to merge 1 commit into
heishamon-power-and-outdoor-metricfrom
heishamon-declare-offset-control
Open

feat(heishamon): declare the heat curve offset as a control a host can render#66
frahlg wants to merge 1 commit into
heishamon-power-and-outdoor-metricfrom
heishamon-declare-offset-control

Conversation

@frahlg

@frahlg frahlg commented Aug 1, 2026

Copy link
Copy Markdown
Member

Stacked on #60, which is stacked on #59. Completes the driver side of srcfl/ftw#520.

A controls entry in the DRIVER block states what an operator may command, in terms a host UI can render without knowing this driver:

  controls = {
    {
      id       = "set_heat_curve_offset",
      label    = "Heat curve offset",
      evidence = "write_ack",
      input    = { type = "number", min = -3, max = 3, step = 1, unit = "°C" },
    },
  },

FTW reads this (srcfl/ftw#738) and draws the control on the Heating card (srcfl/ftw#749). Before it, a control verified against hardware in June had no way to reach a person.

The evidence claim is write_ack, deliberately

apply_offset returns as soon as host.mqtt_publish succeeds. The pump does echo the value on main/Z1_Heat_Request_Temp, but it arrives on a later poll and nothing waits for it or compares it.

Declaring readback would tell a host the setting was confirmed when all we know is that a message left — and it would suppress the "the pump does not confirm this setting" line the host renders for exactly this case. Making it a real readback means completing the command asynchronously against that echo, which is a change to the control path rather than a metadata edit.

Bounds vs config

The declared −3…+3 are the defaults. min_offset/max_offset can narrow them in config, and driver_command clamps to whichever is tighter, so a narrowed site stays safe — its UI just offers a step or two that land on the configured limit. Worth a look if you would rather the declaration not state a range the site can change.

Verified

  • make check green: 3148 passed, 1323 skipped
  • FTW's catalog parser reads it correctly — checked against the real ParseCatalogFile, since its DRIVER block regex stops at the first closing brace in column 0 and a nested list is exactly what could break it. Result: id=heishamon version=0.7.0 caps=[heatpump], one control with all bounds intact.

🤖 Generated with Claude Code

…n render

A controls entry in the DRIVER block names set_heat_curve_offset, labels it,
and describes its one input: number, -3..+3, step 1, °C. FTW reads this and
draws the row (srcfl/ftw#738). Before it, a control verified against hardware
in June had no way to reach a person, and its author was told to write a Home
Assistant automation instead.

The bounds are the defaults. min_offset and max_offset narrow them in config
and driver_command clamps to whichever is tighter, so a narrowed site stays
safe — its UI just offers a step or two that land on the configured limit.

The evidence is write_ack, not readback, because that is what the driver does:
apply_offset returns as soon as the publish succeeds. The pump does echo the
value on main/Z1_Heat_Request_Temp, but it arrives on a later poll and nothing
waits for it or compares it. Declaring readback would tell a host the setting
was confirmed when all we know is that a message left, and would suppress the
line the host shows for exactly this case.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>

frahlg commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Automated maintenance pass: not merging this one — flagging for a maintainer.

The change itself is sound: it only declares the heat-curve-offset control in the DRIVER table's new controls block (id, label, evidence, bounds) so a host can render it — no behavior changes, and the underlying write path (Z1_Heat_Request_Temp) is the same one already hardware-verified per the driver's own verified_by/verified_at/verification_notes fields (Rolf, 2026-06-21). The evidence = "write_ack" choice is correctly conservative — it doesn't claim a readback the driver doesn't perform. Version, sha256, size and manifest/index/support-status are all consistently bumped to 0.7.0. CI is green across all 5 checks.

But this is stacked on #60#59, and:

  1. feat(heishamon): report electrical power, and the outdoor temperature under its canonical name #60 (this stack's middle PR) is still waiting on a hardware confirmation — its power_topic default (Heat_Power_Consumption) is an educated guess, not confirmed against a running pump, per the author's own PR description and the prior review comment on feat(heishamon): report electrical power, and the outdoor temperature under its canonical name #60.
  2. The merge API refuses to merge the bottom of this stack (#59): 403 Merging stacked PRs via this endpoint is not supported. Use the asynchronous merge endpoint instead. — confirmed again on this pass. That needs a maintainer merging from the GitHub web UI, which also handles the stack cleanly (dependent PRs re-target automatically once their base branch merges).

So the order to land this stack is: get hardware confirmation on #60's power_topic, then a maintainer merges #59#60#66 in the web UI (this API can't do it). Nothing further to do here until then.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee0137e6af

ℹ️ 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".

Comment thread drivers/lua/heishamon.lua
-- main/Z1_Heat_Request_Temp, but that arrives on a later poll and nothing
-- here waits for it or compares it. Claiming readback would tell a host
-- the setting was confirmed when all we know is that a message left.
controls = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve the control metadata in the signed artifact

When this driver is installed from the signed FTW channel, .github/workflows/ftw-drivers-release.yml builds it through _ftw_artifact, but tools/ftw_repository.py:251-273 reconstructs __sourceful_ftw_metadata without controls, and line 327 reassigns DRIVER to that reduced table after loading the source. Consequently the published artifact's final driver metadata has no declared control, so the host cannot render the new row even though the catalog source has it; propagate this field into the generated metadata and cover the signed artifact in a test.

AGENTS.md reference: AGENTS.md:L25-L26

Useful? React with 👍 / 👎.

frahlg commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Automated maintenance pass: reviewed, looks sound, but not merging.

  • DRIVER.version (0.6.0 → 0.7.0), manifests/heishamon.yaml, index.yaml sha256/size, devices.yaml, SUPPORT_STATUS.md and support-status.json all move together and agree with each other.
  • The controls declaration doesn't introduce a new hardware claim — it's metadata for a control the driver's own verified_by/verified_at/verification_notes already record as hardware-verified on 2026-06-21 ("Offset control verified via Z1_Heat_Request_Temp"). The evidence = "write_ack" choice is correctly conservative: the driver doesn't wait for or compare the pump's echo, so claiming readback would overstate what's confirmed.
  • 5 CI checks green (dco, packages, drivers, history-secret-scan, public-boundary). signed channel accepts this tree doesn't run here because this PR's base isn't main yet — expected for a stacked branch, not a gap.

Not merging because this is stacked on #60, which is stacked on #59, and neither has landed:

Once #59 and #60 are in, this one should be quick to land on top.


Generated by Claude Code

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