Skip to content

Conversation

EchterAgo
Copy link
Contributor

@EchterAgo EchterAgo commented Jul 13, 2025

apc_modbus: Add CHRG and DISCHRG status

We use the acceptable input flag and the current battery level to
determine if the device status is charging or discharging.

Please merge #3013 first.

Signed-off-by: Axel Gembe [email protected]

General points

  • Described the changes in the PR submission or a separate issue, e.g.
    known published or discovered protocols, applicable hardware (expected
    compatible and actually tested/developed against), limitations, etc.

  • There may be multiple commits in the PR, aligned and commented with
    a functional change. Notably, coding style changes better belong in a
    separate PR, but certainly in a dedicated commit to simplify reviews
    of "real" changes in the other commits. Similarly for typo fixes in
    comments or text documents.

  • Please star NUT on GitHub, this helps with sponsorships! ;)

Frequent "underwater rocks" for driver addition/update PRs

  • Revised existing driver families and added a sub-driver if applicable
    (nutdrv_qx, usbhid-ups...) or added a brand new driver in the other
    case.

  • Did not extend obsoleted drivers with new hardware support features
    (notably blazer and other single-device family drivers for Qx protocols,
    except the new nutdrv_qx which should cover them all).

  • For updated existing device drivers, bumped the DRIVER_VERSION macro
    or its equivalent.

  • For USB devices (HID or not), revised that the driver uses unique
    VID/PID combinations, or raised discussions when this is not the case
    (several vendors do use same interface chips for unrelated protocols).

  • For new USB devices, built and committed the changes for the
    scripts/upower/95-upower-hid.hwdb file

  • Proposed NUT data mapping is aligned with existing docs/nut-names.txt
    file. If the device exposes useful data points not listed in the file, the
    experimental.* namespace can be used as documented there, and discussion
    should be raised on the NUT Developers mailing list to standardize the new
    concept.

  • Updated data/driver.list.in if applicable (new tested device info)

Frequent "underwater rocks" for general C code PRs

  • Did not "blindly assume" default integer type sizes and value ranges,
    structure layout and alignment in memory, endianness (layout of bytes and
    bits in memory for multi-byte numeric types), or use of generic int where
    language or libraries dictate the use of size_t (or ssize_t sometimes).
  • Progress and errors are handled with upsdebugx(), upslogx(),
    fatalx() and related methods, not with direct printf() or exit().
    Similarly, NUT helpers are used for error-checked memory allocation and
    string operations (except where customized error handling is needed,
    such as unlocking device ports, etc.)

  • Coding style (including whitespace for indentations) follows precedent
    in the code of the file, and examples/guide in docs/developers.txt file.

  • For newly added files, the Makefile.am recipes were updated and the
    make distcheck target passes.

General documentation updates

  • Updated docs/acknowledgements.txt (for vendor-backed device support)

  • Added or updated manual page information in docs/man/*.txt files
    and corresponding recipe lists in docs/man/Makefile.am for new pages

  • Passed make spellcheck, updated spell-checking dictionary in the
    docs/nut.dict file if needed (did not remove any words -- the make
    rule printout in case of changes suggests how to maintain it).

Additional work may be needed after posting this PR

  • Propose a PR for NUT DDL with detailed device data dumps from tests
    against real hardware (the more models, the better).

  • Address NUT CI farm build failures for the PR: testing on numerous
    platforms and toolkits can expose issues not seen on just one system.

  • Revise suggestions from LGTM.COM analysis about "new issues" with
    the changed codebase.

@EchterAgo EchterAgo changed the title Add apc modbus chrg apc_modbus: Add CHRG and DISCHRG status Jul 13, 2025
@EchterAgo EchterAgo marked this pull request as draft July 13, 2025 11:19
@EchterAgo
Copy link
Contributor Author

Converted to draft because it needs to check some more conditions. I'll need to test.

@jimklimov jimklimov added enhancement APC modbus Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) labels Jul 14, 2025
@jimklimov jimklimov added this to the 2.8.4 milestone Jul 14, 2025
@jimklimov jimklimov added impacts-release-2.8.2 Issues reported against NUT release 2.8.2 (maybe vanilla or with minor packaging tweaks) impacts-release-2.8.3 Issues reported against NUT release 2.8.3 (maybe vanilla or with minor packaging tweaks) need testing Code looks reasonable, but the feature would better be tested against hardware or OSes labels Jul 14, 2025
@jimklimov
Copy link
Member

@EchterAgo : offtopic question - did you ever hit issues like those discussed in #2609? Any idea if they are rooted in the device, NUT driver, libmodbus core (at least, other people did have issues with exceeded timeouts in its unit tests similar to what I saw, possibly linked to SMP systems), or (y)our libmodbus rtu_usb fork specifically?..

Also, do you develop/test against your variant of rtu_usb branch (probably same as https://github.com/networkupstools/libmodbus/tree/v3.1.10%2Brtu_usb-NUTv2.8.1 when you handed it off), or the current https://github.com/networkupstools/libmodbus/tree/rtu_usb we suggest NUT users use?

We use the acceptable input flag and the current battery level to
determine if the device status is charging or discharging.

Signed-off-by: Axel Gembe <[email protected]>
@EchterAgo EchterAgo force-pushed the add_apc_modbus_chrg branch from 737bef0 to 21a5af3 Compare July 14, 2025 16:32
@jimklimov
Copy link
Member

@EchterAgo : FYI, I've updated the PR with a merge from master and another DRIVER_VERSION bump. If you have some development commits added locally since posting the PR, feel free to rebase them over this github version before pushing up :)

@jimklimov jimklimov marked this pull request as ready for review July 17, 2025 11:29
@jimklimov jimklimov marked this pull request as draft July 17, 2025 11:30
@AppVeyorBot
Copy link

@jimklimov jimklimov marked this pull request as ready for review July 18, 2025 07:21
@jimklimov
Copy link
Member

Un-drafting temporarily to let CI see the updated code base.

@jimklimov jimklimov marked this pull request as draft July 18, 2025 15:06
@jimklimov
Copy link
Member

jimklimov commented Jul 24, 2025

Hm, CI did not update the status back for a draft PR.

@jimklimov jimklimov modified the milestones: 2.8.4, 2.8.5 Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APC enhancement impacts-release-2.8.2 Issues reported against NUT release 2.8.2 (maybe vanilla or with minor packaging tweaks) impacts-release-2.8.3 Issues reported against NUT release 2.8.3 (maybe vanilla or with minor packaging tweaks) Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) modbus need testing Code looks reasonable, but the feature would better be tested against hardware or OSes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants