apc_modbus: Updates, command, writable variables and outlet group support #2184

EchterAgo · 2023-11-14T17:44:09Z

This adds support for commands, writable variables and outlet groups.

For commands there is a new table called apc_modbus_command_map which defines the supported commands as a tuple of command name, register offset and value to write. This also adds a new instcmd status called STAT_INSTCMD_CONVERSION_FAILED for when conversion of values fails. On startup all the commands are registered using dstate_addcmd. This also adds support for the upsdrv_shutdown function.
For writable variables we added a new flag called APC_VF_RW to the existing variables that indicates a writable variable. We added code to convert from a string to UINT/INT/STRING variables with output in APCs register format. There is a new _apc_modbus_setvar function that handles setting variables and rereading them from the device. This also adds a new setvar status called STAT_SET_CONVERSION_FAILED for when conversion of values fails. Variables are now correctly set as ST_FLAG_STRING and ST_FLAG_RW and we call dstate_setaux to give a maximum length for strings.
For outlet groups, we now have names, configurable delays and commands per outlet group. Devices have an outlet group called MOG (Main outlet group) that switches all the outputs of the UPS and 1-3 SOGs (Switched outlet groups) that can be controlled independently. Note that MOG is outlet.group.0 and the SOGs start at outlet.group.1. The outlet groups should have markings with the same index at the back of the unit.
This also reduces the length of some of the defines to make the variable maps more readable.
It also adds a comment to when the reopen matcher is created that clarifies why we create it.

fixes #2119

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.

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 · 2023-11-14T17:45:22Z

Please give this some more testing. I did test a bit already but was unable to test controlling the load on the UPS.

@PrplHaz4

EchterAgo · 2023-11-14T17:47:00Z

@jimklimov I'd still like to know what to do regarding outlet group commands, see #2119 (comment)

EchterAgo · 2023-11-14T17:48:27Z

What I also need to know if the load.* and shutdown.* commands work or if we need to specify the MOG as a target.

EchterAgo · 2023-11-16T08:06:19Z

@jimklimov thanks for the header change, I forgot that.

AppVeyorBot · 2023-11-16T08:59:23Z

✅ Build nut 2.8.1.1174-master completed (commit be8000c807 by @jimklimov)

jimklimov · 2023-11-16T12:28:29Z

@EchterAgo : sorry, I'm a lot on the road and in meetings over this week, could not give much thought to your questions :\

CC @aquette : any ideas regarding outlets (individual/groups) management here?

EchterAgo · 2023-11-16T22:14:42Z

I noticed eaton-pdu-marlin-mib.c already uses outlet.group.* for commands.

jimklimov · 2023-11-21T13:01:52Z

@EchterAgo : I think your force-push removed apc_modbus.h from the Makefile.am again?..

EchterAgo · 2023-11-21T13:05:06Z

@jimklimov readded

jimklimov · 2023-11-22T14:25:35Z

@EchterAgo : got a detailed testing report in the mailing list : https://alioth-lists.debian.net/pipermail/nut-upsuser/2023-November/013486.html

Seems there's some work still needed here and there... :)

EchterAgo · 2023-11-22T17:27:32Z

@EchterAgo : got a detailed testing report in the mailing list : https://alioth-lists.debian.net/pipermail/nut-upsuser/2023-November/013486.html

Seems there's some work still needed here and there... :)

That is good to hear.

I'll look into the power switching commands, already have an idea what might be wrong. Sadly I can not test this here at the moment because I'm currently running a 200TB data migration and don't want to interrupt that.

As for the battery.date issue, I thought I tested that, will look again.

I did test the calibrate.start and calibrate.stop commands.

I'll implement ups.test.result.

EchterAgo · 2023-11-22T17:59:57Z

Rebased and added ups.test.result

@jimklimov DCO is not happy because of a mismatch between your emails:

Commit sha: 6d08119, Author: Jim Klimov, Committer: Axel Gembe; Expected "Jim Klimov jimklimov@gmail.com", but got "Jim Klimov jimklimov+nut@gmail.com".

EchterAgo · 2023-11-22T18:53:59Z

I think I found the battery.date issue, strptime produces a struct tm with tm_isdst, tm_gmtoff and tm_zone based on the current time zone.

_apc_modbus_date_from_nut works with local time while _apc_modbus_date_to_nut works with UTC. I'm going to make everything work with UTC.

EchterAgo · 2023-11-22T19:07:28Z

Pushed new commits, setvar issue is fixed and the battery.date issue too.

EchterAgo · 2023-11-22T19:14:07Z

I also added the target outlet group to the load.* and shutdown.* commands. I think this might be what is causing those commands not to work, not sure about the outlet.* commands, I don't know if those were specifically tested.

EchterAgo · 2023-11-22T19:15:51Z

@jimklimov do you want me to make the author and signed-off-by in your commit use the same email address? If so, with or without the +nut suffix?

AppVeyorBot · 2023-11-22T19:19:11Z

❌ Build nut 2.8.1.1214-master failed (commit 3dea26c6b1 by @EchterAgo)

EchterAgo · 2023-11-22T19:27:50Z

mingw build:

apc_modbus.c: In function '_apc_modbus_date_from_nut':
apc_modbus.c:784:27: error: implicit declaration of function 'timegm'; did you mean 'time'? [-Werror=implicit-function-declaration]
  784 |         if ((epoch_time = timegm(&tm_struct)) == -1) {
      |                           ^~~~~~
      |                           time

looks like we need another way to fix this or implement a timegm alternative for Windows.

EchterAgo · 2023-11-22T19:28:33Z

Or we work with local time, but then we have to also calculate apc_date_start_offset based on the current time zone.

…port This adds support for commands, writable variables and outlet groups. - For commands there is a new table called `apc_modbus_command_map` which defines the supported commands as a tuple of command name, register offset and value to write. This also adds a new instcmd status called `STAT_INSTCMD_CONVERSION_FAILED` for when conversion of values fails. On startup all the commands are registered using `dstate_addcmd`. This also adds support for the `upsdrv_shutdown` function. - For writable variables we added a new flag called `APC_VF_RW` to the existing variables that indicates a writable variable. We added code to convert from a string to UINT/INT/STRING variables with output in APCs register format. There is a new `_apc_modbus_setvar` function that handles setting variables and rereading them from the device. This also adds a new setvar status called `STAT_SET_CONVERSION_FAILED` for when conversion of values fails. Variables are now correctly set as `ST_FLAG_STRING` and `ST_FLAG_RW` and we call `dstate_setaux` to give a maximum length for strings. - For outlet groups, we now have names, configurable delays and commands per outlet group. Devices have an outlet group called MOG (Main outlet group) that switches all the outputs of the UPS and 1-3 SOGs (Switched outlet groups) that can be controlled independently. Note that MOG is `outlet.group.0` and the SOGs start at `outlet.group.1`. The outlet groups should have markings with the same index at the back of the unit. - This also reduces the length of some of the defines to make the variable maps more readable. - It also adds a comment to when the reopen matcher is created that clarifies why we create it. Signed-off-by: Axel Gembe <axel@gembe.net>

Signed-off-by: Jim Klimov <jimklimov@gmail.com>

This outputs the test result, the source of the start of the test and a result modifier. Signed-off-by: Axel Gembe <axel@gembe.net>

The for loop searching for the correct register map kept on looping after finding the value which caused the `apc_map` mariable to potentially point to the wrong register map. This can cause the re-read of the set value at the end of the function to fail. Signed-off-by: Axel Gembe <axel@gembe.net>

MINGW headers do not provide a `timegm` implementation but there is a `_mkgmtime` which does the same: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/mkgmtime-mkgmtime32-mkgmtime64 Signed-off-by: Axel Gembe <axel@gembe.net>

This changes `_apc_modbus_date_from_nut` to use `timegm` instead of `mktime` which does not add the current time zone information. This fixes dates that are off-by-one compared to what was set. Signed-off-by: Axel Gembe <axel@gembe.net>

Maybe this is needed, needs to be confirmed. Signed-off-by: Axel Gembe <axel@gembe.net>

EchterAgo · 2023-11-24T07:28:49Z

FYI, if we want an actual fallback implementation for timegm, there are some here:

https://stackoverflow.com/questions/16647819/timegm-cross-platform

The one from boost should be license compatible and easy to port.

AppVeyorBot · 2023-11-24T07:35:38Z

✅ Build nut 2.8.1.1221-master completed (commit 796037c4d5 by @EchterAgo)

jimklimov · 2023-11-27T14:05:47Z

@EchterAgo : more testing results coming in the mail thread, many things proven working :)

One point that got me concerned was about possibly (mis-)treating battery calibration as a "OB TEST" state. There are some places where we treat the "CAL" state specially (e.g. for requesting a shutdown or not when the battery charge goes low in upsmon). Now that I looked for it, I saw a few more cases of status_set("TEST") in older drivers, but did not right now revise what they were about (battery or some other tests?) - can you please revise what happens in apc_modbus, from search results I saw it also using CAL but did not read into the context...

FWIW, docs/new-drivers.txt does mention CAL but not a TEST...

jimklimov · 2023-11-28T09:24:01Z

Further discussion and practical testing reports in Mailing list:

jimklimov · 2023-12-15T09:10:45Z

Cheers, how is it going here? Do you plan to post some updates shortly, or is (from your viewpoint) the change OK to merge as is and follow up on specific problems and improvements later?

EchterAgo · 2024-01-12T17:28:45Z

Cheers, how is it going here? Do you plan to post some updates shortly, or is (from your viewpoint) the change OK to merge as is and follow up on specific problems and improvements later?

Sorry, it has been difficult for me to find time to work on this lately. I just was able to test the outlet commands and it seems that the outlet specific commands for outlet group 1, which is the only one I have, all work for me. What does not work are the outlet commands that specify the MOG (main outlet group). I'm not sure if that group is actually supposed to be switchable. I'll check the documentation some more.

AppVeyorBot · 2024-02-17T01:59:52Z

✅ Build nut 2.8.1.1436-master completed (commit bc974c3abc by @jimklimov)

AppVeyorBot · 2024-02-27T12:25:46Z

✅ Build nut 2.8.1.1490-master completed (commit 50a07921cd by @jimklimov)

jimklimov · 2024-03-02T10:52:40Z

Cheers, I'm going to merge this PR as it stands today, posted an issue for follow-ups according to comments above.

I believe this change set would already benefit some users, and would expose the code for more practical experience and feedback for further steps. Thank you very much for bringing it this far, and I hope you would have time to get even more done :)

EchterAgo mentioned this pull request Nov 14, 2023

apc_modbus: Add support for commands #2119

Closed

EchterAgo force-pushed the apc_modbus_updates branch 3 times, most recently from 0ab8aad to 8105c73 Compare November 15, 2023 21:26

jimklimov added enhancement APC modbus labels Nov 16, 2023

jimklimov added this to the 2.8.2 milestone Nov 16, 2023

EchterAgo force-pushed the apc_modbus_updates branch from 69d5bab to 0ba1288 Compare November 16, 2023 22:12

EchterAgo force-pushed the apc_modbus_updates branch from 4bde4f6 to c5593d3 Compare November 21, 2023 13:04

EchterAgo force-pushed the apc_modbus_updates branch from c5593d3 to f1c0bf5 Compare November 22, 2023 17:57

EchterAgo and others added 4 commits November 24, 2023 14:23

drivers/Makefile.am: dist the apc_modbus.h file

16ce370

Signed-off-by: Jim Klimov <jimklimov@gmail.com>

apc_modbus: Add ups.test.result

8ee9cbc

This outputs the test result, the source of the start of the test and a result modifier. Signed-off-by: Axel Gembe <axel@gembe.net>

EchterAgo force-pushed the apc_modbus_updates branch from dc0a1ea to a514218 Compare November 24, 2023 07:25

EchterAgo added 3 commits November 24, 2023 14:26

apc_modbus: Always store times in UTC

1594898

This changes `_apc_modbus_date_from_nut` to use `timegm` instead of `mktime` which does not add the current time zone information. This fixes dates that are off-by-one compared to what was set. Signed-off-by: Axel Gembe <axel@gembe.net>

apc_modbus: Add target outlet group to load.* and shutdown.* commands

e52658c

Maybe this is needed, needs to be confirmed. Signed-off-by: Axel Gembe <axel@gembe.net>

EchterAgo force-pushed the apc_modbus_updates branch from a514218 to e52658c Compare November 24, 2023 07:26

Merge branch 'master' into apc_modbus_updates

a6341f2

jimklimov added 6 commits January 13, 2024 19:22

Merge branch 'master' into apc_modbus_updates

489fe45

Merge branch 'master' into apc_modbus_updates

9999a09

Merge branch 'master' into apc_modbus_updates

384d77f

Merge branch 'master' into apc_modbus_updates

c1a755d

Merge branch 'master' into apc_modbus_updates

aba14dc

Merge branch 'master' into apc_modbus_updates

3f818c5

Merge branch 'master' into apc_modbus_updates

fda7f3e

This was referenced Mar 2, 2024

If we want an actual fallback implementation for timegm, there are some... #2337

Open

apc_modbus: follow-up wanted regarding Main Outlet Group operations #2338

Open

jimklimov merged commit 39b8b56 into networkupstools:master Mar 2, 2024
10 of 14 checks passed

HarrisonWilson mentioned this pull request Aug 18, 2024

apc_modbus over TCP not supported hassio-addons/addon-nut#375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apc_modbus: Updates, command, writable variables and outlet group support #2184

apc_modbus: Updates, command, writable variables and outlet group support #2184

EchterAgo commented Nov 14, 2023 •

edited

Loading

EchterAgo commented Nov 14, 2023

EchterAgo commented Nov 14, 2023

EchterAgo commented Nov 14, 2023

EchterAgo commented Nov 16, 2023

AppVeyorBot commented Nov 16, 2023

jimklimov commented Nov 16, 2023

EchterAgo commented Nov 16, 2023

jimklimov commented Nov 21, 2023

EchterAgo commented Nov 21, 2023

jimklimov commented Nov 22, 2023

EchterAgo commented Nov 22, 2023 •

edited

Loading

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

AppVeyorBot commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 24, 2023

AppVeyorBot commented Nov 24, 2023

jimklimov commented Nov 27, 2023 •

edited

Loading

jimklimov commented Nov 28, 2023

jimklimov commented Dec 15, 2023

EchterAgo commented Jan 12, 2024 •

edited

Loading

AppVeyorBot commented Feb 17, 2024

AppVeyorBot commented Feb 27, 2024

jimklimov commented Mar 2, 2024

apc_modbus: Updates, command, writable variables and outlet group support #2184

apc_modbus: Updates, command, writable variables and outlet group support #2184

Conversation

EchterAgo commented Nov 14, 2023 • 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

EchterAgo commented Nov 14, 2023

EchterAgo commented Nov 14, 2023

EchterAgo commented Nov 14, 2023

EchterAgo commented Nov 16, 2023

AppVeyorBot commented Nov 16, 2023

jimklimov commented Nov 16, 2023

EchterAgo commented Nov 16, 2023

jimklimov commented Nov 21, 2023

EchterAgo commented Nov 21, 2023

jimklimov commented Nov 22, 2023

EchterAgo commented Nov 22, 2023 • edited Loading

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

AppVeyorBot commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 22, 2023

EchterAgo commented Nov 24, 2023

AppVeyorBot commented Nov 24, 2023

jimklimov commented Nov 27, 2023 • edited Loading

jimklimov commented Nov 28, 2023

jimklimov commented Dec 15, 2023

EchterAgo commented Jan 12, 2024 • edited Loading

AppVeyorBot commented Feb 17, 2024

AppVeyorBot commented Feb 27, 2024

jimklimov commented Mar 2, 2024

EchterAgo commented Nov 14, 2023 •

edited

Loading

EchterAgo commented Nov 22, 2023 •

edited

Loading

jimklimov commented Nov 27, 2023 •

edited

Loading

EchterAgo commented Jan 12, 2024 •

edited

Loading