fix: stop fabricating meter zeros, declare read-only, stand Easee down - #112
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_fab9f159-ec1a-471c-8488-d139abe0f769) |
![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: 032b41088e
ℹ️ 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".
| connectivity: cloud | ||
| setup: [vendor_portal] | ||
| ders: [ev] | ||
| control: true |
There was a problem hiding this comment.
This new 0.1.0 driver is explicitly marked experimental and says it has not been exercised against live hardware, yet the catalog advertises control and the Lua implementation performs charger current, phase, pause, and resume writes. New drivers must start read-only and control requires HIL acceptance, so publish this with control: false and read_only = true until those gates are satisfied.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
| if action == "ev_set_current" then | ||
| local amps = watts_to_amps(power_w) | ||
| if cmd and type(cmd) == "table" then | ||
| local req_phases = as_number(cmd.phases) | ||
| if req_phases == 1 or req_phases == 3 then | ||
| phases = req_phases |
There was a problem hiding this comment.
When cmd.phases changes the charger between one and three phases, amperage is calculated using the previous phase count. For example, a 3.68 kW 3→1 transition computes about 5 A and then pauses because it is below min_a, while a 1→3 transition computes 16 A and can offer roughly three times the requested power. Update/validate phases before calling watts_to_amps.
Useful? React with 👍 / 👎.
| if paused_state then | ||
| if send_command(CMD_RESUME) then | ||
| paused_state = false | ||
| end | ||
| end | ||
| return true |
There was a problem hiding this comment.
After an ev_pause, a nonzero ev_set_current returns true even when send_command(CMD_RESUME) fails. In that case the charger remains paused while the host records the setpoint as successfully applied and may not retry it; return failure when the required resume does not succeed.
Useful? React with 👍 / 👎.
| if not latest then | ||
| return 1000 | ||
| end | ||
| host.emit_metric("p1_crc_errors", crc_errors) |
There was a problem hiding this comment.
A CRC-valid telegram that omits both active-power OBIS fields still emits p1_crc_errors before returning without meter telemetry. If such malformed telegrams continue arriving, this metric refreshes driver health every second and hides the missing meter stream—the exact metric-only condition this change is intended to prevent. Move the metric emission after the missing-power check.
Useful? React with 👍 / 👎.
| firmware_versions: "" | ||
| notes: "Zaptec Go / Go 2 / Pro via Zaptec Cloud REST API. Email + password; optional charger serial." | ||
| min_driver_version: "0.1.0" | ||
| min_host_version: "2.0.0" |
There was a problem hiding this comment.
The new driver hardcodes Zaptec observation IDs and control endpoints, but its manifest has no upstream_docs entry. Consequently the weekly watcher cannot detect when the API documentation changes or disappears, leaving these telemetry and hardware-control mappings liable to drift unnoticed; add the durable vendor API reference used to decode them.
AGENTS.md reference: AGENTS.md:L60-L66
Useful? React with 👍 / 👎.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Driver audit against the host contract tightened in srcfl/ftw#1199 (companion PR). The individual fixes read as genuine bugs, not just compliance paperwork:
fronius_smart_meter:read_f32was returning0on a failed/given-up Modbus register, so a comms hiccup could publish a fabricated0 Wsite-meter reading — exactly the kind of value a planner would trust. Now returnsnil, the poll skips the meter emit entirely when total power (40098) is missing, and optional fields are omitted rather than coerced to 0. Good fix.pixii: serial probe was gated onok, a local that was structurally alwaysfalse—host.set_snnever ran. Dead-code bug, correctly spotted and removed.zuidwijk_p1: CRC metric no longer emitted on silence (was keeping driver health "alive" on a metric alone with no trusted reading behind it); meter emit now skipped when both import/export OBIS fields are missing instead of defaulting to 0.easee_cloud:driver_default_modenow actually stands the charger down (pause + clampdynamicChargerCurrentto 0) instead of a no-op — matters if FTW loses steer mid-session.config_secrets = { "password" }added, which is correct given the diff also adds the offline stand-down that keeps the raw credential flow the same.- Read-only declarations added to the telemetry-only hybrids/meters (goodwe, growatt, sofar, kostal, sma, victron, fronius, pixii_pv, solis_string, tibber) — consistent with the new host contract from #1199.
- Two new drivers (
zaptec_cloud,tesla_wall_connector) promoted from FTW testdata, manifests added.
Tests: make test-driver run for every changed id per the PR body, full suite 4404 passed / 1069 skipped. All CI checks green except one:
dco check is failing — commit 032b410 is missing a Signed-off-by: trailer. That's a real blocker, not a flake: Sign off with 'git commit -s'... or 'git rebase --signoff f18ceef...' and force-push. Needs a human to fix before this merges — I can't rewrite commit history.
Everything else: safe to merge from my read, once the DCO trailer is added.
|
Automated repo maintenance pass (device-driver PR review). Not merging. Reviewed the diff directly (fronius_smart_meter's nil-vs-fabricated-zero fix, pixii's dead 1. 2. Codex's two P1 review comments are correct, not noise — I traced both in the diff myself:
Given both, Worth a look too, lower severity (Codex P2, both look right on a read but not blocking): the Generated by Claude Code |
Fronius Smart Meter no longer emits 0 W after a given-up power register. Tesla vehicle cache replay marks soc_fresh=false. Pixii serial probe no longer gates on an unset ok. Read-only hybrids and meters declare read_only. Easee default_mode pauses and writes 0 A. Promote Zaptec cloud and Tesla Wall Connector from FTW testdata. Heishamon.lua is left for #66. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Pre-merge review of #112 found two problems with zaptec_cloud's control path: it shipped control-capable (manifest control: true, no read_only) with verification_status still "experimental" and no live charger ever exercised end-to-end, which AGENTS.md's "new drivers start read-only" / HIL-acceptance bar for control does not allow. It also had a real bug: driver_command computed the requested current from the phase count before applying a same-call phase change, so a 1<->3 phase switch would have offered roughly 3x/3x-under the intended power for that command. Make the driver observation-only like tesla_wall_connector: DRIVER.read_only = true, manifest control: false, driver_command accepts ev_pause/resume/ start/set_current as a no-op success (so the planner doesn't mark it failed) instead of writing to the charger, and driver_default_mode has nothing to release. Removes the now-dead send_command/update_charger/ watts_to_amps helpers along with the config knobs (min_a/max_a) that only fed the removed current calculation. Control can come back once the phase- ordering bug is fixed and a real Zaptec unit has verified pause/resume/ current end-to-end. make test-driver ID=zaptec_cloud: 35 passed, 16 skipped. make check: 4404 passed, 1069 skipped. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018cHiT4V4NeH5qyac8RSj4e
032b410 to
603b8e0
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c67feb3f-32cf-4d5d-ad5f-0989072842a6) |
2 participants
Why
Audit of every FTW-bundled driver against the host contract. Companion host PR: srcfl/ftw#1199.
What
read_onlysoc_fresh = falseokconfig_secrets = { "password" };driver_default_modepauses and writes 0 Ahost.set_sn(config.serial)when set (MQTT still has no hub serial)read_only = true(goodwe, growatt, sofar, kostal, sma, victron, fronius, pixii_pv, solis_string, tibber, zuidwijk_p1)Not in this PR
heishamon.luais owned by #66 (controls declaration, stacked on #60/#59). Cleanup-write on Heishamon can follow that stack.Tests
make test-driverfor every changed id. Fulldrivers/tests+tests: 4404 passed, 1069 skipped.Note
Medium Risk
Changes affect site metering and EV stand-down (
easee_clouddefault_mode) and could alter planner inputs where fabricated zeros or stale SoC were previously accepted; new cloud credentials and read-only EV no-ops add operational but not auth-critical surface area.Overview
This release tightens telemetry honesty and host contract alignment across many bundled Lua drivers, plus two new read-only EV observers.
Meter / poll behavior:
fronius_smart_meterstops turning failed Modbus reads into 0 W—read_f32returns nil, total power (40098) must be present or the poll emits nothing, and optional fields/metrics are omitted when nil.zuidwijk_p1only reportsp1_crc_errorsafter a valid frame (so silence cannot look healthy), skips the meter emit when both import/export power OBIS are missing, and catalog control is corrected to false.Identity & safety:
pixiifixes serial discovery (host.set_snwas never reached).ferroampcallshost.set_snwhenconfig.serialis set.tesla_vehiclemarks cached SoC replays withsoc_fresh = falseand wraps HTTP/JSON in safepcallhelpers.easee_cloudaddsconfig_secretsfor the password anddriver_default_modepauses charging and setsdynamicChargerCurrent = 0when FTW loses steer.Read-only declaration: A batch of hybrid/meter drivers now set
read_only = truein the DRIVER block (e.g. Fronius, GoodWe, Growatt, SMA, Victron, sonnen with no-op command/default_mode, Tibber withhttp_hosts, etc.).New drivers:
tesla_wall_connector(Gen 3 local HTTP) andzaptec_cloud(cloud REST, password secret)—both observation-only; EV control commands return success as no-ops so the planner does not fail. Catalog, manifests,devices.yaml, and tests are bumped to match versions and protocol/emit contracts.Reviewed by Cursor Bugbot for commit 603b8e0. Bugbot is set up for automated code reviews on this repo. Configure here.