fix(drivers): resume Modbus drivers that give up every register after a network blip - #982
Conversation
… a blip A network flap makes pixii.lua (and the same probe_read copy in solis, huawei, and others) skip every register after three failed reads. The TCP session can return while the driver has nothing left to ask, so the battery stays offline until process restart. The give-up tables are Lua locals. Reload the file — without driver_cleanup, so a live setpoint is not cleared — when a Modbus driver that used to read stops probing, then poll again. Signed-off-by: Cursor Agent <cursoragent@cursor.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: eb38ddc069
ℹ️ 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".
| old := d.L | ||
| d.L = L | ||
| old.Close() |
There was a problem hiding this comment.
When driver_init errors or its lifecycle context expires during a reprobe, these lines replace and close the previously initialized VM before callInitLocked runs. The registry only receives a poll error, so subsequent default-mode calls use the partially initialized state; if the default hook depends on configuration or locals populated during init, the autonomous fallback can fail precisely while the driver is stale. Initialize the candidate successfully before committing the swap, or restore the old VM on failure.
AGENTS.md reference: AGENTS.md:L36-L36
Useful? React with 👍 / 👎.
| if d.Env.lastPollEvidence.Attempts == 0 { | ||
| d.skipReprobe = true |
There was a problem hiding this comment.
When the link remains down beyond the first give-up threshold, the fresh VM's immediate retry makes Modbus attempts, so this condition is false; once that VM gives up again, shouldReprobe reloads it again. A prolonged outage therefore reruns the file and driver_init every few polls, repeatedly executing initialization side effects and generating avoidable connection/log churn. Latch after the reload attempt rather than only when its immediate retry also makes zero calls.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eb38ddc. Configure here.
| // retry, not a shutdown. | ||
| d.Env.Logger.Info("modbus driver stopped probing registers; reloading to retry") | ||
| if rerr := d.reprobeLocked(ctx); rerr != nil { | ||
| return 0, fmt.Errorf("reprobe: %w", rerr) |
There was a problem hiding this comment.
Missing latch after failed reload
High Severity
When reprobeLocked fails, Poll returns without setting skipReprobe. A give-up quiet poll keeps Attempts at 0, so every later tick retries reload. That contradicts the safety boundary that a failed reload latches until process restart and turns a bad path or missing driver file into a per-poll reload loop with repeated error logs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eb38ddc. Configure here.
| if driverDeclaresReadOnlyBattery(L) { | ||
| d.Env.BatteryTelemetryOnly = true | ||
| } | ||
| return d.callInitLocked(ctx) |
There was a problem hiding this comment.
VM replaced before init succeeds
Medium Severity
reprobeLocked closes the previous VM and assigns the new one before callInitLocked finishes. If driver_init fails, the prior state is already gone and the live driver is left on an uninitialized VM, so recovery from a transient init error is worse than staying on the old give-up state.
Reviewed by Cursor Bugbot for commit eb38ddc. Configure here.
| if d.Env.lastPollEvidence.Attempts > 0 { | ||
| d.sawModbusRead = true | ||
| d.skipReprobe = false | ||
| } |
There was a problem hiding this comment.
Failed attempts arm reprobe
Medium Severity
notePollModbusActivity sets sawModbusRead whenever Attempts is greater than zero, including pure transport failures. The stated boundary is that a driver that never successfully read is not reloaded, but a never-online device still arms reprobe after its first failed polls and then reload-cycles after give-up.
Reviewed by Cursor Bugbot for commit eb38ddc. Configure here.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Reviewed the diff. Fixes a real reliability bug: several catalog Lua drivers (pixii, solis, huawei, ...) permanently give up on a register after 3 failed reads so an unimplemented point doesn't fail every poll — but a transient Modbus transport blip makes every register look absent the same way, and the driver then has nothing left to ask even once the link recovers. Previously that required a process restart to clear.
- Fix reloads the Lua VM (fresh file read, fresh
driver_initwith the stored config) only when a driver that had previously read successfully (sawModbusRead) comes back with zero attempts — and latches (skipReprobe) so a legitimately absent register doesn't trigger a reload loop.TestGiveUpOnAbsentRegisterDoesNotReloadconfirms the two cases are distinguished correctly. - Deliberately skips
driver_cleanupon reprobe (that path writes the hardware default, e.g. setpoint 0 on a battery) — good call, documented in the comment, and this is a probe retry, not a shutdown. Poll()stays fully underd.mu, so the reload-then-repoll happens atomically from the caller's view; the old VM is closed after the swap, no leak.- Well-tested for the Pixii case specifically (recovers SoC telemetry,
IsOnline()true, no device fault after the blip) as well as the generic give-up pattern.
One non-blocking question: reprobeLocked re-reads the driver file from disk (os.ReadFile(d.Path)), so if the file changed between the original load and the blip (a driver update landing mid-session), the reload would pick up the new version rather than the one the process started with. If driver-file updates already trigger a full runtime restart elsewhere, this is moot — otherwise worth a sentence confirming that's intended.
Safe to merge from my read.
PR #982 reloaded a give-up driver, but a missing file retried every poll, failed attempts armed reprobe on a never-online device, and driver_init ran after the previous VM was already closed. Latch on reload failure, arm only after a successful read, and initialize the candidate before swapping so default-mode still has its locals. A long outage still retries: latching after every reload would miss the 9-minute Pixii blip that this path exists to recover. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
3 participants
Accepted text proposal
Issue or Discussion: Slack thread “pixii offline efter nätverksblipp” (HannesB / forty-two-watts).
Maintainer comment that accepted this scope: Fredrik asked to fix the self-locking Pixii offline bug so the battery comes back on its own after a network blip.
What changed
The Lua host now reloads a Modbus driver’s VM when that driver used to read registers and then attempts zero reads in a poll — the give-up signature after a transport blip (
no route to host), not after a single unimplemented register.Reload does not call
driver_cleanup(that would write setpoint 0). The samePoll()then runs again. If the reload still produces 0 reads,skipReprobelatches so a driver that legitimately stops reading is not reloaded every poll.An unimplemented register while others still read does not trigger reload.
Why
On site
forty-two-watts(Pixii at192.168.1.52:502):pixii.lualoggedregister X did not answer 3 times; leaving it alone until restart.The catalog pattern (
GIVE_UP_AFTER = 3) exists so unimplemented registers (Pixii 40288meter_energy_sfon firmware below CPU 2.0.23) do not fail every poll. The host already distinguishes transport vs refusal; give-up still treated a dead link like a missing register.The canonical driver lives in
srcfl/device-drivers. This is a host safety net so a blip does not require a process restart.Boundaries and safety
driver_cleanup, so a stuck battery is not written to 0 W as a side effect of recovery.Out of scope: changing
pixii.luaitself (bundled snapshot; the catalog should only give up on illegal-address / exception 2). That belongs insrcfl/device-drivers.Verification
go test ./internal/drivers/—TestGiveUpDriverRecoversAfterTransportBlip,TestGiveUpOnAbsentRegisterDoesNotReload,TestPixiiRecoversAfterTransportGiveUp(bundledpixii.luabehind a gated Modbus that returnsno route to host).modbus driver stopped probing registers; reloading to retry, then live telemetry.Pixii Modbus reprobe test log
Checklist
To show artifacts inline, enable in settings.