drivers: add experimental APC Microlink serial driver#3406
drivers: add experimental APC Microlink serial driver#3406libschmudie-so wants to merge 20 commits into
Conversation
|
A ZIP file with standard source tarball and another tarball with pre-built docs for commit 1fdc538 is temporarily available: NUT-tarballs-PR-3406.zip. |
|
❌ Build nut 2.8.5.4533-master failed (commit 25f28b85db by @libschmudie-so) |
|
This looks very solid, thanks! CI disliked something about dist recipes though; while at it - the man page HTML rendering is not listed in the Makefile.am. I'm on a phone commuting now, so can't look deeper. |
…etworkupstools#3406] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…#3406] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
|
@libschmudie-so : Cheers, your PR is made from your master branch and not with a toggle that I can edit it as a maintainer, so I posted a couple of recipe fixes as PRs into your fork. Hopefully they should get the CI builds passing. Is this a personal or corporate-backed contribution? Feel free to add a note in |
Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
docs/man/Makefile.am: provide apsmicrolink.html also [networkupstools#3406]
drivers/Makefile.am: EXTRA_DIST apcmicrolink-maps.h apcmicrolink.h [networkupstools#3406]
NEWS.adoc: introduce apcmicrolink driver [networkupstools#3406]
|
❌ Build nut 2.8.5.4537-master failed (commit a65d9792c4 by @libschmudie-so) |
|
Yeah, now it is sad about spelling checks. Do you have |
|
There are also compiler complaints from static analysis, like: For the switch, they have two conflicting requirements ("must have a default", and "do not need a default when all possible enum values were listed"), so we use a copy-paste block of pragmas to quiesce that. So list all enum values where needed, and add a For the formatting string, see |
Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
|
✅ Build nut 2.8.5.4713-master completed (commit 140fc68627 by @libschmudie-so)
|
|
✅ Build nut 2.8.5.4713-master completed (commit 140fc68627 by @libschmudie-so) |
|
@jimklimov Stupid question, but can you tell me what I am missing regarding the issue in "GHA-01: Tarballs / Make Dist and Docs Tarballs, see workflow page for links". It works locally and with ci_build.sh at this point without errors... |
|
❌ Build nut 2.8.5.4714-master failed (commit 93b97962b5 by @libschmudie-so) |
Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
|
✅ Build nut 2.8.5.4718-master completed (commit f9204fadbd by @libschmudie-so)
|
|
✅ Build nut 2.8.5.4718-master completed (commit f9204fadbd by @libschmudie-so) |
Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
Nevermind I think i figured it out... |
|
✅ Build nut 2.8.5.4723-master completed (commit 6c64df88b0 by @libschmudie-so)
|
Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
|
@jimklimov I think I have every fixed up and am also using some stuff shared with apc_modbus now as it is indeed similar. Is there anything left from your side? |
|
✅ Build nut 2.8.5.4727-master completed (commit 5a3d7ef674 by @libschmudie-so)
|
…d clang [networkupstools#3406] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
There was a problem hiding this comment.
This is quite a significant body of work, now with refactoring/simplification that seems generally beneficial, but I would welcome a review from @EchterAgo as this change impacts the driver maintained by him, just to make sure I did not miss something critical.
As for the review, it seems there is one line that should be dropped to pass CI across the board (PR against source branch posted), and highlighted several spots where I think it would be more maintainable to write the code differently, but I am open to being convinced I misunderstood a greater scheme :) Otherwise LGTM upon at least a cursory reading.
| #include "apc_common.h" | ||
|
|
||
| const apc_value_map_t apc_input_quality_map[] = { | ||
| { (1U << 0), "Acceptable" }, |
There was a problem hiding this comment.
Here and below, is there a particular reason to NOT use textual names defined in the header for same bit-shifted values (avoid possible discrepancies if only one of the explicit bit-shifts somehow gets edited later without concern for the other)?
There was a problem hiding this comment.
As far as I know not all have a matching enum, but for those that do I can of course use the enum values. They pretty much just came straight out of apcmicrolink-maps without much changing, thats why it doesn't have the enums in there.
| return 0; | ||
| } | ||
|
|
||
| time_stamp = ((time_t)value * 86400) + ((time_t)10957 * 86400); |
There was a problem hiding this comment.
Rather a question of style and maintainability than a requirement to change: why was 10957 hard-coded here and not defined as a macro or static number? e.g. as dropped from apc_modbus.c:
static const time_t apc_date_start_offset = 946684800; /* 2000-01-01 00:00 */
Wondering if the specific numbers would differ as/when/if this code migrates to 64-bit time sooner or later.
There was a problem hiding this comment.
I honestly haven't thought about it that much. The value from the UPS was in days so the 10957 in days made sense to me. I guess we could pull it out into a static or macro tho.
| #include "apcmicrolink-maps.h" | ||
|
|
||
| static const microlink_value_map_t microlink_outlet_status_map[] = { | ||
| { (1U << 0), "StateOn" }, |
There was a problem hiding this comment.
Same question as elsewhere about use of bit-shift values explicitly vs. enum value textual names, to avoid future surprises.
| @@ -0,0 +1,357 @@ | |||
| /* apc_common.c - Shared APC driver helpers | |||
| * | |||
| * Copyright (C) 2026 Lukas Schmid <lukas.schmid@netcube.li> | |||
There was a problem hiding this comment.
Just in case: is there some code (transplanted map contents) that would warrant adding Copyright © 2023 Axel Gembe <axel@gembe.net> here?
CC @EchterAgo
There was a problem hiding this comment.
From what I recall the functions in here are what I had written, but they probably are inspired from the apc_modbus so I'll just add the copyright in there too.
| return 0; | ||
| } | ||
|
|
||
| static int microlink_parse_bool(const char *text, int *value) |
drivers/apcmicrolink.c: drop pragma to ignore a warning unknown to old clang
…st inspired by the original code from apc-modbus Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
|
✅ Build nut 2.8.5.4741-master completed (commit a0e8bd7409 by @libschmudie-so)
|
|
✅ Build nut 2.8.5.4742-master completed (commit b1ae14653a by @libschmudie-so)
|
|
✅ Build nut 2.8.5.4743-master completed (commit fb8908dfda by @libschmudie-so)
|
Implement a new apcmicrolink driver for APC Smart-UPS units that expose the Microlink serial protocol over the RJ45 serial interface.
The driver establishes a Microlink session over the serial link, reads and parses the device descriptor blob at runtime, and uses that descriptor map to publish standard NUT variables, writable settings, alarms, and supported instant commands. It also adds labeled Microlink command masks for battery tests, runtime calibration, UPS control, and outlet-group power control, including APC-style load., shutdown., and outlet.group.N.* commands.
The driver now uses the Microlink UPS status word for ups.status, exposes input.transfer.reason from the UPS status change cause enum, and avoids folding simple-signaling bits into the synthesized NUT status. The patch also adds the driver to the build and hardware lists, documents configuration and current limitations, and cleans up initial upstream review issues such as missing file headers, whitespace, and format-string warnings.
Tested against:
APC Smart-UPS 750 (SMT750RMI2UC)
Coding helper disclosure: Codex
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.
Use of coding helper tools and AI should be disclosed in the commit
or PR comments (it is interesting to know which ones do a decent job).
As with other contributions, a human is responsible and thanked for the
quality and content of the change, and is presumed to have the right to
post that code to be published further under the project's license terms.
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 othercase.
Did not extend obsoleted drivers with new hardware support features
(notably
blazerand other single-device family drivers for Qx protocols,except the new
nutdrv_qxwhich should cover them all).For updated existing device drivers, bumped the
DRIVER_VERSIONmacroor 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.hwdbfileProposed NUT data mapping is aligned with existing
docs/nut-names.txtfile. If the device exposes useful data points not listed in the file, the
experimental.*namespace can be used as documented there, and discussionshould be raised on the NUT Developers mailing list to standardize the new
concept.
Updated
data/driver.list.inif 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
intwherelanguage or libraries dictate the use of
size_t(orssize_tsometimes).Progress and errors are handled with
upsdebugx(),upslogx(),fatalx()and related methods, not with directprintf()orexit().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.txtfile.For newly added files, the
Makefile.amrecipes were updated and themake distchecktarget passes.General documentation updates
Added a bullet point into
NEWS.adoc, possibly alsoUPGRADING.adocif there is something packagers or custom-build users should take into
account (new driver categories, configuration options, dependencies...)
Updated
docs/acknowledgements.txt(for vendor-backed device support)Added or updated manual page information in
docs/man/*.txtfilesand corresponding recipe lists in
docs/man/Makefile.amfor new pagesPassed
make spellcheck, updated spell-checking dictionary in thedocs/nut.dictfile if needed (did not remove any words -- themakerule 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.