Repository navigation
generic_modbus, adelsystem_cbi: use typed buffers for register reads - #3645
user01010111 wants to merge 4 commits into
Conversation
Read Modbus bits and registers into byte- and word-sized buffers, then convert the result explicitly into the caller's integer value. This removes uninitialised-value use while retaining the existing masks and error handling. Fixes networkupstools#3642 AI assistance: OpenAI Codex with gpt-6-astra and gpt-daybreak-blue-latest. Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>
|
A ZIP file with standard source tarball and another tarball with pre-built docs for commit 9c03c8a is temporarily available: NUT-tarballs-PR-3645.zip. |
jimklimov
added
modbus
portability
|
|
||
| /* Read a modbus register */ | ||
| int register_read(modbus_t *mb, int addr, regtype_t type, void *data) | ||
| int register_read(modbus_t *mb, int addr, regtype_t type, unsigned int *data) |
There was a problem hiding this comment.
Having some second thoughts here and below: should data be a CPU/ARCH dependent uint * or a specifically sized uint16_t * (as probably dictated/expected by the UPS hardware)?
There was a problem hiding this comment.
data points to an unsigned int reg_val in get_dev_state(). The libmodbus calls already receive correctly sized uint16_t register buffers or uint8_t bit buffers; we then assign the numeric value into the caller’s integer. This preserves the full register range independently of integer width and byte order, matching the bulk-read path. I’d retain unsigned int * here.
There was a problem hiding this comment.
I was concerned about potential loss of bits on systems where int would be less than 16 bits wide, although Google tells me that with current standards and platforms that is the minimum guaranteed size (16/32/64 bits being common variability).
But there is also an aspect of wasting memory on systems with bit-width too large. If we know we get 8/16 bits from the device and libraries, there is little point of storing and passing around arrays of 32/64-bit words, right?
Would there be a point in fixing the get_dev_state() to use the hardware-dictated/wire-protocol word sizes instead?
There was a problem hiding this comment.
Both Adelsystem read modes pass the GCC/Clang boundary and consumer checks and sanitizer runs. With the dependent alarm-count and recovery fixes applied, full-driver polling and disconnect/reconnect tests also pass against real libmodbus in both modes on custom and default ports. Each recovery test survives three refused reconnects, restores fresh readings and retains the configured port.
I changed register_read() and its get_dev_state() caller together to use uint16_t, with explicit casts at the formatting sites. That makes the register-value contract clearer. The array and stored fields already use uint16_t, so this narrows the temporary scalar rather than saving array memory. The typed libmodbus buffers, masks and failure behaviour are unchanged.
The wider review also found that both existing register_write() helpers reinterpret integer storage through smaller pointer types. Source review indicates that on affected big-endian hosts, a value of 1 can become 0 while the write reports success; this has not been runtime-tested on a big-endian host. This predates the read changes. I suggest a separate fix for both drivers; would you prefer it separately or within this series?
Use uint16_t for register_read() output and its get_dev_state() caller. Keep the typed libmodbus buffers, masks and failure behaviour, and cast formatting arguments explicitly for portable integer promotion. Validated both read modes with GCC and Clang, boundary and consumer checks, sanitizers, and real-libmodbus polling with the dependent fixes. Both modes also pass custom/default-port disconnect and recovery tests, surviving three refused reconnects and restoring changed readings. GCC project tests and distribution/install/uninstall checks pass. AI assistance: OpenAI Codex (gpt-6-astra). Adversarial review by Claude Code (claude-opus-5-5, high effort) covered the complete PR, amendment, callers and validation evidence, with a follow-up on compiler diagnostics and polling state dumps. No blocking read-path finding remained. Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>
…us series branch Carry the reviewed Adelsystem uint16_t read-value amendment forward without rewriting the existing contribution or maintainer history. The amendment was developed with OpenAI Codex (gpt-6-astra) and reviewed with Claude Code (claude-opus-5-5, high effort); the review scope was networkupstools#3645 and its type amendment. Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>
|
✅ Build nut 2.8.5.5905-master completed (commit 454d5cb415 by @jimklimov)
|
Merge order: #3645, then #3646, then #3647. The later PRs include the preceding commits until those changes merge.
Read Modbus bits and registers into byte- and word-sized buffers, then assign their numeric values to the caller. This removes uninitialised-value use in Generic polling and Adelsystem's individual-register mode while retaining the existing masks and error handling. Adelsystem's helper and caller now both use
uint16_t, with explicit formatting casts.The original read-storage defect was reproduced before the fix and checked afterwards with Valgrind. The
uint16_tamendment passes GCC/Clang checks covering all four read types, boundary values, masks, failure clearing, formatting and scaled values in both Adelsystem read modes. ASan/UBSan/LSan checks pass. With #3646 and #3647 applied, both modes also pass full-driver polling and disconnect/reconnect tests against real libmodbus on disposable loopback endpoints. Recovery passes on custom and default ports after three refused reconnects, restoring fresh readings and retaining the configured port.GCC project tests and distribution/install/uninstall checks pass; Clang builds of both affected drivers pass. The broader Clang build encounters diagnostics in unchanged C++ code. Valgrind could not start for the new amendment because matching loader debug symbols were unavailable. No hardware testing is claimed.
Fixes #3642.
AI assistance: the original series used OpenAI Codex with
gpt-6-astraandgpt-daybreak-blue-latest. The Adelsystem type amendment was implemented and validated with OpenAI Codex (gpt-6-astra). Claude Code (claude-opus-5-5, high effort) performed an adversarial review of the full #3645 diff, the amendment, connected callers and consumers, and the validation evidence. A follow-up checked the compiler diagnostics and polling state dumps; no blocking finding remained in the read changes. The separate pre-existing write-helper concern is outside this amendment.Reproduction files and instructions for the original series: #3644 (comment)