fix(ferroamp): give the float32 encoder a domain, and stop emitting inf#26
Merged
Conversation
![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: fb22cc88a1
ℹ️ 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 on lines
+50
to
+53
| local function finite(value) | ||
| if value ~= value then return 0 end | ||
| if value == math.huge or value == -math.huge then return 0 end | ||
| return value |
There was a problem hiding this comment.
When firmware returns the stated 0x7F7FFFFF sentinel, both declared production runtimes—GopherLua and LuaJIT—represent numbers as float64, so 3.4028235e38 * 1000 remains the finite value 3.4028235e41; it is not equal to math.huge and this helper emits the bogus reading into site totals. The overflow only occurs in the repository's LUA_32BITS test interpreter, so reject the decoded float32 sentinel/domain before scaling rather than relying on host-number overflow.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
The setpoint encoder accepted any number. Four ways that went wrong, all on
a path that writes to a battery:
inf the normalising loop never terminates; inf / 2 is inf
1e39 encodes as hi=0x803C, which decodes to -5.5e-39 — a large charge
command becomes a small discharge
NaN returns {nan, nan} as register words
<1.2e-38 yields a negative hi, which is not a 16-bit number at all
None of these come from a sane setpoint. All of them come from the control
plane, which is where setpoints originate, so a unit slip or a NaN from a
division upstream reaches them directly. The encoder now refuses what float32
cannot hold and driver_command returns false rather than writing something
arbitrary. curtail encodes before enabling the limit, so a refused command
cannot leave the site curtailed at whatever was in the register already.
Verified against Python struct as ground truth: decode_f32_be is exact over
22686 bit patterns covering the full exponent range, both signs, subnormals
and 20000 random words — 0 mismatches on the LUA_32BITS build where the old
helper failed. The encoder's working range is unchanged byte for byte; worst
relative error over realistic setpoints is 4.75e-08, which is float32
rounding.
Separately, the driver emitted inf. decode_f32_be refuses infinity, but the
driver then scales kW to W, and an inverter answering with float32's largest
value — which some firmware does for a register it cannot serve — decodes to
3.4e38 and overflows back into inf on the multiply. That is the exact thing
decode_f32_be's comment sets out to prevent. All eleven scaled readings are
now checked before they reach a site total.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
…its sign
Found by an external review of the same code. On a 32-bit integer build,
negating the smallest integer wraps to itself:
-math.mininteger == math.mininteger -- true
math.abs(math.mininteger) < 0 -- true
So `-power_w / 1000` leaves the sign alone for power_w = -2147483648, and
the driver asks the battery to charge when the caller said discharge.
Ferroamp reads negative kW as charge, so the command inverts rather than
merely being wrong in magnitude. `math.abs(power_w)` on the curtail path
returns a negative number for the same input, which would encode a negative
export limit.
Verified on ./lua55:
-power_w / 1000 -> -2147483.75 (wrong sign)
-(power_w / 1000) -> 2147483.75 (right)
Dividing first, and multiplying by 1.0 before math.abs, makes the value a
float, where negation behaves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
frahlg
force-pushed
the
26-ferroamp-float-domain
branch
from
July 26, 2026 07:31
179f651 to
59bd70b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while getting an external review of the decode maths from #21. The decode side came back clean; the encode side did not.
The encoder had no domain
encode_f32_wswrites a battery setpoint. It accepted any number, and four kinds went wrong:infinf / 2isinf. The driver hangs.1e39hi=0x803C, which decodes to-5.5e-39. A large charge becomes a small discharge.NaN{nan, nan}as register words.< 1.2e-38hi, which is not a 16-bit number at all.None of these come from a sane setpoint. All of them come from the control plane, which is where setpoints originate — a unit slip, or a NaN from a division upstream, reaches them directly.
The encoder now refuses what float32 cannot hold, and
driver_commandreturnsfalseinstead of writing something arbitrary.curtailencodes before enabling the limit, so a refused command cannot leave the site curtailed at whatever figure was in the register already.Sign inversion on a control path is the same class of bug this function was rewritten to fix in #21 — it was still there, just moved to the far end of the range.
It also emitted
infdecode_f32_berefuses infinity: "Infinity and NaN would poison every downstream sum; report nothing." But the driver then scales kW to W, and an inverter answering with float32's largest value — which some firmware does for a register it cannot serve — decodes to3.4e38and overflows straight back intoinfon the multiply.All eleven scaled readings are now checked before they reach a site total.
Evidence
Verified against Python
structas ground truth, run on./lua55(which turned out to be a 32-bit float build —0.1 + 0.2prints0.3):decode_f32_beis exact. 22 686 bit patterns — the full exponent range0x00–0xFF, both signs, subnormals including the smallest, 20 000 random words. 0 mismatches. The 94 inf/NaN patterns all return 0 as intended. The fix: use the canonical decode helpers, which 35 drivers were missing #21 rewrite holds on precisely the build where the old helper failed.4.75e-08— float32 rounding. Round numbers round-trip exactly.0xFFFF 0xFFFF, NaN and ±inf all decode to 0. Only0x7F7F 0xFFFFgot through, via the scaling.make check: 2782 passed, 577 skipped — unchanged from main.Not addressed here
Whether a finite but implausible reading should be emitted at all is a broader question — it affects all 62 drivers and belongs in the host-API contract, not in one driver. This PR only stops values that are not numbers.
🤖 Generated with Claude Code