Add read_second_capacitance + write_capacitance / write_second_capacitance#2
Conversation
…tance The firmware exposes two tank-capacitor pairs as adjacent (value + exponent) holding registers; the client only had a read accessor for the first pair and nothing on the write side. Round out the API: - read_second_capacitance() -> float (Farads, mirrors read_capacitance against HOLD_REG_SECOND_CAP_VAL / HOLD_REG_SECOND_CAP_EXP at 0x3012/13) - write_capacitance(value: float) -> None - write_second_capacitance(value: float) -> None Both writes encode the Farads value into a (val, exp) pair that maximises uint16 precision (mantissa lands in [6554, 65535] when possible) and emit one atomic FC 0x10 (Write Multiple Registers) PDU. Reads of the second pair use FC 0x03 with count=2 — same atomicity guarantee as the first pair. Internal refactor: extract _read_cap_pair and _write_cap_pair so the four public methods share one implementation, plus a module-level _encode_capacitance helper that's independently testable. read_capacitance keeps its existing semantics; the body just moves into _read_cap_pair. Validation: - Negative, nan, inf inputs raise InvalidValueError. - Exponent outside [-30, 6] raises (matches read_capacitance's bound, catches uninitialised firmware memory in either direction). - bool inputs are explicitly rejected (int subclass guard) so write_capacitance(True) can't sneak through as 1.0 F. Tests: 35 new in tests/test_capacitance.py covering the encoder round- trip across pF→F, the single-call atomicity of both pairs in both directions, and every documented rejection path. Verification: pytest 214 passed (179 pre-existing + 35 new), ruff clean, mypy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <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: 1398e35f7f
ℹ️ 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".
| assert exp_reg.addr == val_reg.addr + 1, ( | ||
| f"{val_reg.name} and {exp_reg.name} must be at adjacent addresses" | ||
| ) | ||
| val, exp = _encode_capacitance(float(value)) |
There was a problem hiding this comment.
When the public write_capacitance(True) or write_second_capacitance(False) path is used, this float(value) cast converts the bool to 1.0/0.0 before _encode_capacitance() can run its explicit bool guard, so the new API silently writes a plausible capacitor setting instead of raising InvalidValueError. This contradicts the intended int-subclass protection and can misconfigure hardware when a caller accidentally passes a boolean.
Useful? React with 👍 / 👎.
1 participant
Summary
The firmware exposes two tank-capacitor pairs as adjacent (value + exponent) holding registers, but the client only had a read accessor for the first pair and nothing on the write side. This rounds out the convenience API:
read_second_capacitance() -> float— mirror ofread_capacitanceagainstHOLD_REG_SECOND_CAP_VAL/HOLD_REG_SECOND_CAP_EXP(0x3012 / 0x3013).write_capacitance(value: float) -> None— Farads in, atomic 2-register write out.write_second_capacitance(value: float) -> None— same against the second pair.Both writes encode the Farads value into a
(val, exp)pair that maximises uint16 precision (mantissa lands in[6554, 65535]whenever possible) and emit one FC 0x10 (Write Multiple Registers) PDU. Reads of the second pair use FC 0x03 withcount=2, same atomicity guarantee as the first pair.Internal refactor: extracted
_read_cap_pair/_write_cap_pairso the four public methods share one implementation, plus a module-level_encode_capacitancehelper that's independently testable.read_capacitancekeeps its existing semantics — the body just moves into_read_cap_pair.Validation: negative,
nan,infraiseInvalidValueError; exponent outside[-30, 6]raises (matches the read-side bound);boolinputs are explicitly rejected (int-subclass guard) sowrite_capacitance(True)can't sneak through as 1.0 F.Test plan
testsworkflow passes on Python 3.10, 3.11, and 3.12pytest --cov=smartpower_modbusreports 214 passing (179 pre-existing + 35 new intests/test_capacitance.py)python -c "from smartpower_modbus.client import _encode_capacitance; print(_encode_capacitance(100e-6))"→(10000, -8)rel=1e-4read_capacitance/read_second_capacitanceLocal verification (already run)
pytest -q: 214 passed (179 pre-existing + 35 new)ruff check .: cleanmypy smartpower_modbus: clean🤖 Generated with Claude Code