Add new driver for UPS supporting microlink protocol #2663

tom1422 · 2024-10-17T12:59:41Z

An experimental driver for APC microlink protocol devices that I would like to contribute to the project. Currently this only supports serial, but adding USB support should be straightforward.

Will need more work to ensure it follows NUT convention more closely (e.g. Variables being defined not at start of functions).

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.

AppVeyorBot · 2024-10-17T13:20:08Z

❌ Build nut 2.8.2.2262-master failed (commit 787c78847a by @tom1422)

desertwitch · 2024-10-17T14:13:06Z

Wow, thanks for the effort, I'm sure many people will love to see this protocol supported. 💯 Out of personal interest, what was the UPS device that you built this around and which variables were you able to extract that worked for your device? Can you post a upsc data dump of the UPS variables (of your device) when running your driver?

tom1422 · 2024-10-17T15:02:16Z

Wow, thanks for the effort, I'm sure many people will love to see this protocol supported. 💯 Out of general interest, what was the UPS device that you built this around and which variables were you able to extract that worked for your device? Can you post a data dump of the UPS variables (of your device) when running your driver?

Thank you, here's some of the info as requested:

The device i used was an SMT750i (that is too old to support modbus).

I also tried it on a friend's UPS (some larger and or newer units that do support modbus) and they seem to all expose more or less the same variables (there's probably still more configurations that need supporting).

Here is the dump from upsc for my device: (removed serial number)

Init SSL without certificate database
battery.charge: 100.000000
battery.date: 2019-02-01
battery.date.maintenance: 2023-07-31
battery.runtime: 16250.000000
battery.voltage: 27.125000
device.id: APCUPS
device.mfr.date: 2010-02-05
device.model: Smart-UPS750
device.serial: xxxxxxxxxxxx
device.type: ups
driver.debug: 2
driver.flag.allow_killpower: 0
driver.name: apc_ul
driver.parameter.pollinterval: 1
driver.parameter.port: /dev/ttyUSB0
driver.parameter.synchronous: auto
driver.state: updateinfo
driver.version: 2.8.2.1095.1-1096-g07169142d
driver.version.internal: 0.01
experimental.output.energy: 1434365.000000
input.frequency: 50.078125
input.quality: Good
input.sensitivity: High
input.transfer.high: 264.000000
input.transfer.low: 216.000000
input.transfer.reason: AcceptableInput
input.voltage: 233.187500
outlet.group.0.delay.reboot: 8.000000
outlet.group.0.delay.shutdown: 180.000000
outlet.group.0.delay.start: 0.000000
outlet.group.0.name: UPS Outlets
outlet.group.0.status: StateOn
outlet.group.0.timer.reboot: Inactive
outlet.group.0.timer.shutdown: Inactive
outlet.group.0.timer.start: Inactive
outlet.group.count: 1
output.current: 0.000000
output.frequency: 50.078125
output.voltage: 233.187500
output.voltage.nominal: VAC240
ups.delay.reboot: 8.000000
ups.delay.shutdown: 180.000000
ups.delay.start: 0.000000
ups.efficiency: unavailable
ups.firmware: UPS 09.4
ups.load: 0.000000
ups.power: 0.000000
ups.power.nominal: 750.000000
ups.realpower: 0.000000
ups.realpower.nominal: 500.000000
ups.status: OL
ups.temperature: 27.000000
ups.test.interval: Startup_14_After_last
ups.test.result:
ups.timer.reboot: Inactive
ups.timer.shutdown: Inactive
ups.timer.start: Inactive

(I have also updated the pull request with some extra details)

drivers/apc_ul_util.c

+			return;
+		}
+
+		for (uint8_t i = 1; i < exited.array_length; i++)


jimklimov · 2024-10-17T18:53:14Z

Sounds great, and CI (or local ./ci_build.sh) should help weed out most warnings quickly :)

Did you find a protocol definition, or was this "lock-picked" somehow?

jimklimov · 2024-10-17T19:13:48Z

As for USB support, if you mean something greater than /dev/ttyUSB0 which is serial as far as code is concerned, perhaps nutdrv_qx can be a sort of model for a driver that supports both types of media; all others are for some one media.

jimklimov

Taking a look at this PR about why it failed CI (and to read a bit more into code, although still cursorily), got some early comments and simple fixes here.

As you noted before, I suppose the next big issue to pass NUT CI requirements across the board would be a bit of dumbing down the C language syntax (e.g. requiring the declarations to be in first lines of scope, not further in code), so it builds on old systems too.

jimklimov · 2024-10-20T21:24:03Z

drivers/apc_ul_util.h

+#define FQID(id_i, type_i) {.id = (id_i), .type = (type_i), .array_index = -1, .array_length = -1}
+#define FQID_INDEX(id_i, type_i, index_i, length_i) {.id = (id_i), .type = (type_i), .array_index = (index_i), .array_length = (length_i)}
+
+#define FQUN(...) \
+	{__VA_ARGS__}


Looking forward to seeing whether the different compiler generations (and their C99+ settings) in the NUT CI farm would accept this syntax or find it too revolutionary :)

jimklimov · 2024-10-20T21:24:38Z

drivers/apc_ul_util.h

+	int array_index;
+	int array_length;


Why not e.g. ssize_t if we want signed (and to fit into compiler-guaranteed array sizing)?

jimklimov · 2024-10-20T21:26:35Z

drivers/apc_ul_util.h

+	char *command_directive;
+
+	uint8_t *command;
+	int command_length;


Can this "length" be negative? Would a (s)size_t be better here?

jimklimov · 2024-10-20T21:26:43Z

drivers/apc_ul_util.h

+typedef struct
+{
+	uint8_t *array;
+	int length;


Can this "length" be negative? Would a (s)size_t be better here?

jimklimov · 2024-10-20T21:28:24Z

drivers/apc_ul_util.h

+int fqun_to_string(const fqid_t *fqun, char *dest);
+
+int compareFQUN(const fqid_t *source, const fqid_t *target, int depth);
+
+int validate_row(microlink_row_t row);
+
+int addChecksumToMessage(uint8_t *packet, int data_length, int array_size);
+
+int parseDatastore(uint8_t *data_block, uint8_t number_of_rows,
+				   uint8_t row_size, usage_t *usage_array);
+
+void create_slave_password(uint8_t *master_password, int master_password_length,
+						   uint8_t *microlink_header, int header_length,
+						   uint8_t *serial_number, int serial_num_length,
+						   uint8_t *slave_password);
+
+void create_master_password_verify(uint8_t *slave_password, int slave_password_length,
+								   uint8_t *microlink_header, int header_length,
+								   uint8_t *serial_number, int serial_num_length,
+								   uint8_t *master_password_verify);


Can "length"'s in these methods arguments be negative? Would a (s)size_t be better here?

Also. for methods that populate a caller-provided string (e/g/ fqun_to_string) I'd prefer seeing an argument to track the allowed buffer length (decremented and consulted in the logic).

jimklimov · 2024-10-20T21:37:05Z

@tom1422 : hm, I got too used to having maintainer write access to forks' branches that PRs come from.
Can't push my fixes for some compiler complaints here :-}

jimklimov · 2024-10-20T21:43:38Z

drivers/apc_ul_util.c

+		case fqid_collection:
+			if (current_fqid.array_length > 0)
+			{
+				snprintf(dest + ptr_string, 12, "[%-3i, %-3i]", current_fqid.array_index % 256, current_fqid.array_length % 256);


13 not 12 here, to allow for \0 always.

Also note the other comment, about adding an argument to check for not overflowing a caller-provided buffer size.

jimklimov · 2024-10-20T21:45:13Z

drivers/apc_ul_util.c

+			return;
+		}
+
+		for (uint8_t i = 1; i < exited.array_length; i++)


int i (beside moving declarations to start of scope)?

jimklimov · 2024-10-20T22:29:43Z

Per offline discussion, completion of this contribution will likely be delayed, so marking Draft for now.

desertwitch · 2024-10-24T13:19:47Z

I just saw the driver file is empty with the latest force push, what's going on? Refactoring?

tom1422 · 2024-10-24T13:28:10Z

I just saw the driver file is empty with the latest force push, what's going on? Refactoring?

I'm talking to jimklimov about this, but I have decided to remove it while I sort out / determine what's best (legally speaking). I hope you can understand and please feel free to close the pull request (if you think that would be better, I dont mind opening one in future or contributing on this one directly).

desertwitch · 2024-10-24T13:29:49Z

I just saw the driver file is empty with the latest force push, what's going on? Refactoring?

I'm talking to jimklimov about this, but I have decided to remove it while I sort out / determine what's best (legally speaking). I hope you can understand and please feel free to close the pull request (if you think that would be better, I dont mind opening one in future or contributing on this one directly).

Ah I see, I figured something was going on behind the scenes... 😉
All good, was just curious and still appreciate all the effort you've put into this.

Signed-off-by: Thomas Eldon <[email protected]>

jimklimov · 2024-12-01T13:22:36Z

Although currently suspended, cross-linking to issue #1426

github-advanced-security bot found potential problems Oct 17, 2024

View reviewed changes

drivers/apc_ul_util.c Outdated

return;

}

for (uint8_t i = 1; i < exited.array_length; i++)

Check failure

Code scanning / CodeQL

Comparison of narrow type with wide type in loop condition

Comparison between [i](1) of type uint8_t and [array_length](2) of wider type int.

jimklimov added enhancement APC USB serial port HCL documentation DDL labels Oct 17, 2024

jimklimov added this to the 2.8.3 milestone Oct 17, 2024

jimklimov requested changes Oct 20, 2024

View reviewed changes

jimklimov reviewed Oct 20, 2024

View reviewed changes

jimklimov marked this pull request as draft October 20, 2024 22:28

jimklimov modified the milestones: 2.8.3, 2.8.4 Oct 20, 2024

tom1422 force-pushed the nut-microlink branch 3 times, most recently from 818fc36 to e05577f Compare October 24, 2024 13:15

removed duplicated authored commits

dd28f6c

Signed-off-by: Thomas Eldon <[email protected]>

tom1422 force-pushed the nut-microlink branch from e05577f to dd28f6c Compare October 25, 2024 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new driver for UPS supporting microlink protocol #2663

Add new driver for UPS supporting microlink protocol #2663

tom1422 commented Oct 17, 2024 •

edited

Loading

AppVeyorBot commented Oct 17, 2024

desertwitch commented Oct 17, 2024 •

edited

Loading

tom1422 commented Oct 17, 2024

jimklimov commented Oct 17, 2024

jimklimov commented Oct 17, 2024

jimklimov left a comment •

edited

Loading

jimklimov Oct 20, 2024

jimklimov Oct 20, 2024

jimklimov Oct 20, 2024

jimklimov Oct 20, 2024

jimklimov Oct 20, 2024

jimklimov commented Oct 20, 2024

jimklimov Oct 20, 2024

jimklimov Oct 20, 2024

jimklimov commented Oct 20, 2024

desertwitch commented Oct 24, 2024

tom1422 commented Oct 24, 2024

desertwitch commented Oct 24, 2024

jimklimov commented Dec 1, 2024

Add new driver for UPS supporting microlink protocol #2663

Are you sure you want to change the base?

Add new driver for UPS supporting microlink protocol #2663

Conversation

tom1422 commented Oct 17, 2024 • edited Loading

General points

Frequent "underwater rocks" for driver addition/update PRs

Frequent "underwater rocks" for general C code PRs

General documentation updates

Additional work may be needed after posting this PR

AppVeyorBot commented Oct 17, 2024

desertwitch commented Oct 17, 2024 • edited Loading

tom1422 commented Oct 17, 2024

jimklimov commented Oct 17, 2024

jimklimov commented Oct 17, 2024

jimklimov left a comment • edited Loading

Choose a reason for hiding this comment

jimklimov Oct 20, 2024

Choose a reason for hiding this comment

jimklimov Oct 20, 2024

Choose a reason for hiding this comment

jimklimov Oct 20, 2024

Choose a reason for hiding this comment

jimklimov Oct 20, 2024

Choose a reason for hiding this comment

jimklimov Oct 20, 2024

Choose a reason for hiding this comment

jimklimov commented Oct 20, 2024

jimklimov Oct 20, 2024

Choose a reason for hiding this comment

jimklimov Oct 20, 2024

Choose a reason for hiding this comment

jimklimov commented Oct 20, 2024

desertwitch commented Oct 24, 2024

tom1422 commented Oct 24, 2024

desertwitch commented Oct 24, 2024

jimklimov commented Dec 1, 2024

tom1422 commented Oct 17, 2024 •

edited

Loading

desertwitch commented Oct 17, 2024 •

edited

Loading

jimklimov left a comment •

edited

Loading