Second-capacitor + write_capacitance + review-notes polish#3
Conversation
- P0: example.py --sp-p now float + write_value(); '50' means 50%, not raw 50 (0.50%). - P1: Register.from_name() rejects ambiguous suffixes (SP_P, ERROR, CONFIG, ...) with a helpful candidate list instead of silently picking the input-reg variant. - P1: Extracted FakeSerialClient + fixtures from tests/test_transport.py to tests/conftest.py; test_capacitance.py and test_units.py no longer cross-import. - P2: CI hardening — least-privilege permissions, PR-only concurrency cancel, workflow_dispatch, Python 3.13 in matrix, new build job runs `python -m build` and `twine check --strict`. - P2: README clarifies retry_writes default + adds Developer checks section mirroring CI; doc/SDR-... path references replaced with 'Spec rev A7'. - P3: Documented low-level write_coil/write_coils caller-responsibility coercion. - P3: read_many() batches contiguous-address runs by kind into single FC01/02/03/04 reads (capped at 2000 bits / 125 words), dedupes, validates model up front. - P3: Split test_transport.py into test_transport_errors.py, test_device_info.py, test_client_lifecycle.py — happy-path + batching stays in test_transport.py. All CI gates pass locally (ruff, mypy, pytest --cov, py_compile, build, twine). 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
SmartpowerModbus/smartpower_modbus/client.py
Line 625 in e054686
Preserve capacitance type validation
When a caller passes True/False (or a numeric string) to write_capacitance()/write_second_capacitance(), this float(value) coercion runs before _encode_capacitance() and bypasses the encoder's explicit non-real/bool rejection, so write_capacitance(True) silently writes a 1.0 F setting instead of raising InvalidValueError and leaving the bus untouched. Pass the original value into _encode_capacitance() (or validate before coercion) so the public write helpers enforce the same checks that the encoder tests document.
ℹ️ 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".
1 participant
Summary
read_second_capacitance/write_capacitance/write_second_capacitanceAPI for the secondHOLD_REG_SECOND_CAP_*pair, with atomic 2-register read/write and uint16-precision-maximising exponent picker.--sp-psemantic mismatch inexample.py, rejects ambiguous suffix lookups inRegister.from_name(), batchesread_many()into contiguous-range reads, and cleans up test layout (shared fakes moved totests/conftest.py,test_transport.pysplit into 4 focused modules).permissions, PR-onlyconcurrencycancel,workflow_dispatch, Python 3.13 added to the matrix, newbuildjob runspython -m buildandtwine check --strict.Test plan
python -m pytest -q --cov=smartpower_modbus— 222 tests pass, coverage 89%.python -m ruff check .— clean.python -m mypy smartpower_modbus— clean.python -m py_compile example.py— clean.python -m build+python -m twine check --strict dist/*— both pass.python example.py --port COMx --slave 1 --model SmartPowerGen_2.0 --sp-p 50should now write 50% (not 0.50%).smartpower-cli ... write SP_P 50should now fail with an ambiguity error pointing atINPUT_REG_SP_PvsHOLD_REG_SP_Pinstead ofReadOnlyRegisterError.🤖 Generated with Claude Code