feat(api): send a declared driver command, and hold it for a bounded time - #741
feat(api): send a declared driver command, and hold it for a bounded time#741frahlg wants to merge 1 commit into
Conversation
…time
POST /api/drivers/{name}/control sends one command the driver declared and
holds it; DELETE ends the hold early; /api/drivers/{name} shows what is set
and until when.
Deliberately outside control v2. Synthesising a RuntimePolicy for an unsigned
driver is worse than doing nothing: HostEnv.permissionAllowed grants
everything only while the policy is nil, so a policy without permissions
silently blocks the driver's own MQTT, and LuaDriver.Command refuses a control
v2 driver on the legacy path — v2 wants driver_command_v2 entrypoints no
community driver has. Signed packages keep CommandV2 unchanged.
What that costs: no host-enforced write scope, no host-verified evidence.
What it keeps is the part that protects hardware. Core clamps to the declared
bounds rather than trusting the Lua, and the declaration is the whole
allowlist — an undeclared control is a 400, not a 200 for a command the Lua
ignored, which is what the registry cannot otherwise tell apart.
Every hold ends by itself, into the driver's own driver_default_mode rather
than a value Core invented: only the driver knows what neutral is. Default
4 h, maximum 24 h, nothing survives a restart. Because the policy is nil the
registry's lease machinery never arms, so the 300 s Lease.MaxDuration ceiling
does not apply and the hold does not need to fight it.
Tests drive a real registry and a real Lua driver, so they distinguish "Core
sent it" from "Core said it sent it", and cover the case that bites: replacing
a hold must stop the old timer from defaulting the device out from under the
new setting.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.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: d8118ce57e
ℹ️ 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".
| if err := s.deps.Registry.Send(r.Context(), name, body); err != nil { | ||
| writeJSON(w, 500, map[string]string{"error": err.Error()}) | ||
| return | ||
| } |
There was a problem hiding this comment.
If the HTTP client disconnects while Send is in flight, or Lua changes the device and then returns an error, Registry.Send can report an error even though the command reached hardware. This branch returns without arming a timer or restoring default mode, so a healthy, polling device can retain the setting indefinitely—the exact browser-close failure the bounded hold is meant to prevent. Reserve/serialize the hold before dispatch and restore the driver default whenever command completion is ambiguous.
AGENTS.md reference: AGENTS.md:L34-L36
Useful? React with 👍 / 👎.
| type controlRequest struct { | ||
| Control string `json:"control"` | ||
| Value *float64 `json:"value"` | ||
| DurationS int `json:"duration_s"` |
There was a problem hiding this comment.
For a catalog control with input.type equal to boolean or string, decoding the documented value into *float64 rejects a normal payload such as {"value":true} or {"value":"eco"} with 400. Omitting the value instead sends only the action, so both nonnumeric input types accepted by pickControls are unusable through this endpoint; preserve the JSON value according to the declared input type.
Useful? React with 👍 / 👎.
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
| if err := s.deps.Registry.SendDefault(ctx, name); err != nil { |
There was a problem hiding this comment.
When a new POST arrives just after the expiry callback deletes and unlocks the old hold, the new command can be applied and armed before this SendDefault executes; the old expiry then immediately resets the newly requested setting. The earlier pointer identity check cannot detect that interleaving because the map entry was removed before dispatch, so command dispatch and hold transitions need per-driver serialization.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| resp.Controls = s.driverControls(name) | ||
| resp.Hold = s.activeControlHold(name) |
There was a problem hiding this comment.
The control-loop watchdog and site-freshness paths call Registry.SendDefault directly, but nothing removes the corresponding API hold. After a telemetry failure successfully returns the device to autonomous mode, this endpoint therefore continues reporting the old control as active until its original expiry, and recovery never reapplies it. Clear the hold whenever another path defaults/restarts the driver, or centralize hold state with default dispatch so the response describes the actual setting in force.
AGENTS.md reference: AGENTS.md:L34-L36
Useful? React with 👍 / 👎.
1 participant
Stacked on #738 — merge that first. Stage C of the path in #520.
POST /api/drivers/{name}/controlwith{control, value, duration_s}sends one command the driver declared and holds it.DELETEon the same path ends it early./api/drivers/{name}shows the active hold, so a UI can render what is set and until when.Why this sits outside control v2
The obvious approach — synthesise a
RuntimePolicyfor unsigned drivers and reuseCommandV2— does not survive contact:HostEnv.permissionAllowed(host.go:215) grants everything only while the policy is nil. A synthesised policy without permissions silently blocks heishamon's own MQTT. Adding a knob would break the driver.LuaDriver.Command(lua.go:261) refuses a control v2 driver on the legacy path, and v2 requiresdriver_command_v2+driver_default_mode_v2entrypoints. No community driver has them.So this leaves the policy layer completely untouched. Signed packages keep
CommandV2with its write scope, lease and evidence, unchanged. Unsigned drivers get a simpler path validated against the catalog declaration from #738.The cost, stated plainly: no host-enforced write scope, no host-verified evidence. That belongs in the UI copy, not buried here.
What it keeps: Core clamps every value to the declared bounds rather than trusting the Lua — a driver that forgets to clamp is exactly the driver this protects. And the declaration is the whole allowlist, so an undeclared control is a 400 rather than a 200 for a command the Lua silently ignored, which the registry cannot otherwise distinguish.
The hold
Every hold ends by itself, and ending calls the driver's own
driver_default_moderather than a value Core invented — only the driver knows what neutral is (heishamon's is its configuredsafe_offset, which an operator may have moved). Default 4 h, max 24 h, nothing survives a restart.This also disposes of the 300 s
Lease.MaxDurationceiling I flagged earlier: the registry's lease machinery only arms forIsControlV2(), so with a nil policy there is no ceiling to fight.Tests
They drive a real
Registryand a real Lua driver, so they tell "Core sent it" from "Core said it sent it" — the earlier version of these tests passed against a driver that had never polled. Covered: clamping reaches the device, undeclared control is refused, hold is visible and releasable, hold expires into default, and replacing a hold stops the old timer from defaulting the device out from under the new setting.Verified to rebase cleanly onto #728 (mDNS) — no overlap; that branch lives in a fork so it cannot be the PR base.
make verifyclean. UI in the Heating view is next.🤖 Generated with Claude Code