Skip to content

feat(drivers): let a driver declare the commands an operator may send - #738

Merged
frahlg merged 2 commits into
masterfrom
520-declare-driver-controls
Aug 4, 2026
Merged

feat(drivers): let a driver declare the commands an operator may send#738
frahlg merged 2 commits into
masterfrom
520-declare-driver-controls

Conversation

@frahlg

@frahlg frahlg commented Jul 31, 2026

Copy link
Copy Markdown
Member

Answers the "does the driver need to declare something extra?" half of #520. Today the answer is no — there is nothing to declare. This adds it.

The gap

Settings → Devices renders hand-written branches per driver family (devices.js:23 says so: the choices stay there "until the signed driver catalog grows a general config-schema field"). So a driver with a real control surface cannot describe it. The Heishamon heat pump has had a hardware-verified set_heat_curve_offset since June, and its author was told to drive it from a Home Assistant automation instead.

Signed packages already carry this shape in RuntimeCommand — typed inputs, lease policy, evidence — but only for drivers on the signed channel. rd.policy is nil for every bundled and local driver, so the DRIVER block is where their declaration has to live.

What this adds

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

Surfaced on /api/drivers/{name} and /api/drivers/catalog.

Nothing sends these commands. This is the description; the command path is separate work and is where the interesting problems are (lease expiry runs driver_default_mode, which for heishamon writes the offset back to safe — so a manual setting needs a Core-side hold with heartbeat renewal, since Lease.MaxDuration caps at 300 s).

Notes for review

  • The parser is the real change. pickKVBlock matches [^}]* and stops at the first closing brace; pickString anchors to the start of a line. Both are right for flat fields and both break on a nested list written inline. This reads controls with its own brace matching rather than changing how every other field is parsed. TestBundledCatalogStillParses covers the regression that would matter — a controls reader that quietly eats an unrelated field.
  • Unrenderable declarations are dropped, not surfaced. No id, a type outside number/boolean/string, or a number missing either bound. A slider with one end is a guess, not a control.
  • Bounds are pointers because zero is a real bound — a 0..100 percentage and an undeclared range must not read the same.
  • The JSON key stays absent for drivers that declare nothing, so a client can tell "reports only" from "declares an empty list".
  • Path matching follows IsEVOrVehicleDriver/IsReadOnlyDriver: portable path first, then filename. Operators write drivers/x.lua and /data/x.lua both.

No bundled driver declares controls yet — drivers/ is a generated snapshot and cannot be edited here. Adding the block to heishamon is a device-drivers change, after srcfl/device-drivers#60.

make verify clean.

🤖 Generated with 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: fa3148ce30

ℹ️ 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 go/internal/drivers/catalog_controls.go Outdated
Comment thread go/internal/drivers/catalog_controls.go
Comment thread go/internal/drivers/catalog_controls.go Outdated

@miravoss26 miravoss26 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.

Read the whole diff — a driver can now declare operator-facing controls in its DRIVER block, surfaced read-only via /api/drivers/{name} and the catalog. Nothing dispatches them yet; this is the description, not the path. Clean and unusually well-tested.

Correctness

  • The hand-rolled brace matcher (nestedBlock/topLevelTables) is the right call given the existing pick helpers stop at the first }, and it's string-aware — a { inside a label doesn't truncate the table (TestControlsSurviveBraceInsideString). It doesn't handle escaped quotes or Lua long-brackets, but the comments scope that out for metadata, which is fair.
  • fieldString/fieldNumber's (?:^|[\s,{])name\s*= boundary correctly stops min from matching inside minimum — the \s*= suffix does the real work there.
  • Pointer bounds (*float64) so 0 stays distinguishable from "unset" is exactly right, and tested. Drop-if-unrenderable (no id / bad type / single or inverted bound / bad evidence) keeps a half-formed control off the operator's screen.

One note, not a blocker: driverControls calls LoadCatalogMulti (a full catalog parse from disk) on every /api/drivers/{name} request. Fine for a settings-UI call; worth a cache only if it ever lands on a hot path.

Security screen: no secrets, no new deps, no network, no authz change — and nothing commands the device yet, so the blast radius is a read surface. regexp.QuoteMeta guards the field-name interpolation (all literals today anyway); the regexes are linear, no ReDoS.

CI green, coverage thorough. Safe to merge from my read — leaving the merge to a human (ftw isn't on my auto-merge allowlist).

frahlg and others added 2 commits August 4, 2026 09:24
Settings renders a hand-written branch per driver family, so a driver with a
real control surface has no way to describe it and no way to reach a person.
The Heishamon heat pump has had a verified curve-offset command since June and
its author was told to write a Home Assistant automation instead.

A `controls` list in the DRIVER block names the command, labels it, describes
its single input — type, bounds, step, unit — and states what counts as proof
the device took it. /api/drivers/{name} and /api/drivers/catalog surface it.

Nothing sends these commands. This is the description; the path is separate
work. Signed packages already carry the shape in RuntimeCommand, but only for
drivers on the signed channel — a bundled or local driver has no policy, so
the DRIVER block is where its declaration has to live.

The existing pick helpers stop at the first closing brace and anchor to the
start of a line, both right for flat fields and both wrong for a nested list
written inline. This reads controls with its own brace matching rather than
changing how every other field is parsed for the sake of one that did not
exist yet.

A declaration the UI could not render is dropped rather than surfaced half
formed: no id, a type outside number/boolean/string, or a number missing
either bound. Bounds are pointers because zero is a real bound — an
undeclared minimum and a minimum of zero must not read the same.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@frahlg
frahlg force-pushed the 520-declare-driver-controls branch from fa3148c to 47f7e81 Compare August 4, 2026 07:28
@frahlg
frahlg merged commit 603a0d4 into master Aug 4, 2026
21 of 23 checks passed
Leitet added a commit to srcfl/device-drivers that referenced this pull request Aug 6, 2026
Implements the contract from srcfl/ftw#738/#741, merged today.
driver_command_v2 and driver_default_mode_v2 return structured
results the host can hold to account: applied requires a write ack
plus a setpoint readback the host itself observed, and default mode
always writes the remote-control disable and reads it back. The v1
entrypoints stay for local operator builds; their default mode keeps
the skip-if-not-ours courtesy toward FoxESS-app schedule periods,
which v2 cannot prove and therefore does not offer.

The migration surfaced a latent v1 bug: the host write bindings
return an error string rather than raising, so pcall alone reported
failed writes as success. checked_write/checked_write_multi now test
both layers on every write path.

Package recipe moves to the v2 runtimes (gopher-lua-source-v2,
sourceful.host/*/v2) with driver_default_mode_v2 as the default-mode
entrypoint; both targets build as unsigned candidates locally.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Leitet <johan@sourceful-labs.com>
@frahlg
frahlg deleted the 520-declare-driver-controls branch August 7, 2026 08:14
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.

2 participants