add new driver for UPS INVT HT11 1-3KVA series

[asperg]

INVT HR11 series
INVT HT11 series
SNR-UPS-ONRT (3000-20000) series
HIDEN EXPERT UDC9200S-RT 1-3 кВА
HIDEN EXPERT UDC9200H-RT 6-10 кВА

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.

[lgtm-com]

This pull request introduces 2 alerts when merging 46c0e2d into 9c1e920 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same
• 1 for Implicit function declaration

[jimklimov]

Seems like modbus-ascii files are part of libmodbus? If so, why are they here then?

[asperg]

Added modbus-ascii files because in the libmodbus library, the functions responsible for ascii are moved to a separate branch and are absent in the standard libmodbus library in linux distributions

[jimklimov]

Thanks for clarification. I guess then there should be a configure-time check on whether the system provides needed headers, types and methods, or a fallback implem is needed (like we have for many string methods). After all, distros do differ and there is a better chance that libmodbus code would be kept up to date by its project, than a copy in NUT would. Also makes sense to reference the origin for later resyncs.

[asperg]

I will try to do what you suggest, however I am not a developer, I am a systems engineer. I was faced with the task of implementing UPS in my enterprise, and I solved it as best I could. The libmodbus project consists of a lot of files, I just compiled them into two files. Regarding checks for the presence of ascii functions in the release of the current distribution - a very good idea, I will study how to implement it. I'll put pullrequest in draft mode.

[gdt]

I think libmodbus files and the driver should be in separate commits.

I don't understand by libmodbus-ascii is part of a specific driver binary, vs being a library or in a library that other drivers could use. But maybe that's according to nut doctrine.

I am really unclear on why it's ok to pull in libmodbus code. That should be explained in the commit adding those files. The files are labeled LGPL2.1-or-later, and nut proper is GPL2-or-later. I am fuzzy on this - I think it's just plain ok -- but the rationale should also be explained in the commit message. It should also explain where the bits came from. If they are from libmodbus, as seems likely, they should first be added as is, and then modified.

I don't understand if the autoconf tests used by libmodbus are all done by nut. There don't seem to be any new ones. Maybe that's ok, but it's not explained.

The commit adding the driver should have a first line that tries to mention at least the UPS family, subject to fitting in 50 characters.

[lgtm-com]

This pull request introduces 2 alerts when merging f1f7d95 into 9c1e920 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same
• 1 for Implicit function declaration

[jimklimov]

@asperg : I was looking at issue comments in libmodbus project like stephane/libmodbus#576 (comment) and it seems that "all devices support RTU" (although it may be cumbersome due to stricter requirements including timing - especially via gateways).

Do you know if the devices you added support for are ASCII-only, or if the proposed driver can be changed to work with RTU reliably, and so use the well-tested editions of libmodbus available in OS distributions?

[biergaizi]

Thanks for clarification. I guess then there should be a configure-time check on whether the system provides needed headers, types and methods, or a fallback implem is needed (like we have for many string methods).

ASCII support was never merged into the main branch of the upstream libmodbus, it only exists as an unofficial fork so far, so no such checks are necessary to begin with...

Overall, unfortunately, it's my impression that libmodbus is not actively implementing any new feature at this point, and it has several deficiencies that need to be improved. For example, my device driver, huawei-ups2000, needs the "read device identification" feature, which was an open issue in libmodbus since 2012, it's still unimplemented today. My proposal of creating a stop-gap solution to allow raw device access in libmodbus Issue 231 also received no reply from the developer. So I don't expect it to be implemented in libmodbus any time soon.

For NUT, I had to provide my own "read device identification" code by opening a serial device in NUT, perform this function, close the serial device, and reopen it libmodbus...

The files are labeled LGPL2.1-or-later, and nut proper is GPL2-or-later. I am fuzzy on this - I think it's just plain ok -- but the rationale should also be explained in the commit message.

+1. The license is absolutely not an issue. See the comment from Free Software Foundation - "This is the previous version of the LGPL: a free software license, but not a strong copyleft license, because it permits linking with nonfree modules. It is compatible with GPLv2 and GPLv3."

For the record, I've already added a function from the libmodbus code to NUT under LGPL2.1+, justifiably because libmodbus doesn't support sending device identification request, and thus Modbus CRC code was needed to perform this operation.

[biergaizi]

Do you know if the devices you added support for are ASCII-only, or if the proposed driver can be changed to work with RTU reliably, and so use the well-tested editions of libmodbus available in OS distributions?

+1. It would be the best if the device supports RTU.

[asperg]

The documentation specifies: modbus-ASCII, modbus-RTU. however, I was unable to switch the UPS to modbus-RTU mode. Modbus-ASCII is enabled by default

[gdt]

Am guessing this is temporary and you intend to remove it before making this not a draft. I can certainly understand wanting that for your debugging, but it seems aligned to local preferences rather than something that should be in the source tree.

[jimklimov]

Note: there was some recent commit traffic at the libmodbus project repo. Maybe some things would start getting merged eventually...

[jimklimov]

The libmodbus maintainer suggested to "Feel free to rebase the ASCII branch and update it" as a means of getting rtu-ascii in core library (and closed my question tickets as answered, which sort of makes sense although the practical issue is not addressed by this and the bookmark about this not-addressing "disappears"). After all, if the interest in the project has revived after some months/years of hibernation, they have a huge backlog of issues and PRs to clean up just to understand what should be acted upon.

@asperg @biergaizi : could any of you guys be enticed into (posting CLA forms for libmodbus and) bringing the ASCII branch into modern shape so that this new driver PR could just rely on sufficiently new libmodbus releases provided by an OS distro? I might at best fiddle with C code changes but have nowhere to test it. Or frankly no time to spare, either, with all the other backlogs and commitments already piled on me :)

A slightly worse/fallback alternative could be to at least have a NUT fork of sorts to have this codebase usable and battle-proven. Either way, some of the files added into NUT codebase by this PR should be part of libmodbus (packaged or not).

Currently we have a precedent with RTU USB branch that people can custom-build against per https://github.com/networkupstools/nut/wiki/APC-UPS-with-Modbus-protocol until their distro provides a version of libmodbus with that code.

At the moment the NUT fork only cared about that one branch (no work about sync from upstream/master etc.); if there are more to juggle there would be some need for automation (e.g. provide an auto-updated rtu_usb + serial + master as for-nut branch?)

[jimklimov]

Also ran a build recently, there are quite a few CI complaints about missing headers, possible use of uninitialized variables/arrays/buffers, and other things that upset C compilers:

  CC       modbus-ascii.o
modbus-ascii.c: In function ‘_sleep_response_timeout’:
modbus-ascii.c:153:12: error: implicit declaration of function ‘nanosleep’ [-Werror=implicit-function-declaration]
  153 |     while (nanosleep(&request, &remaining) == -1 && errno == EINTR) {
      |            ^~~~~~~~~
modbus-ascii.c: In function ‘_modbus_ascii_prepare_response_tid’:
modbus-ascii.c:222:62: error: unused parameter ‘req’ [-Werror=unused-parameter]
  222 | static int _modbus_ascii_prepare_response_tid(const uint8_t *req, int *req_length)
      |                                               ~~~~~~~~~~~~~~~^~~
modbus-ascii.c: In function ‘_modbus_ascii_recv’:
modbus-ascii.c:433:68: error: unused parameter ‘rsp_length’ [-Werror=unused-parameter]
  433 | static ssize_t _modbus_ascii_recv(modbus_t *ctx, uint8_t *rsp, int rsp_length)
      |                                                                ~~~~^~~~~~~~~~
modbus-ascii.c: In function ‘_modbus_ascii_pre_check_confirmation’:
modbus-ascii.c:458:71: error: unused parameter ‘rsp_length’ [-Werror=unused-parameter]
  458 |                                               const uint8_t *rsp, int rsp_length)
      |                                                                   ~~~~^~~~~~~~~~
modbus-ascii.c: In function ‘modbus_ascii_set_serial_mode’:
modbus-ascii.c:1026:53: error: unused parameter ‘mode’ [-Werror=unused-parameter]
 1026 | int modbus_ascii_set_serial_mode(modbus_t *ctx, int mode)
      |                                                 ~~~~^~~~
modbus-ascii.c: In function ‘_modbus_receive_msg’:
modbus-ascii.c:1598:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 1598 |                 if (length_to_read != 0) {
      |                    ^
modbus-ascii.c:1602:13: note: here
 1602 |             case _STEP_META:
      |             ^~~~
modbus-ascii.c: In function ‘modbus_poll’:
modbus-ascii.c:3180:20: error: this statement may fall through [-Werror=implicit-fallthrough=]
 3180 |                 nb = 0;
      |                 ~~~^~~
modbus-ascii.c:3181:13: note: here
 3181 |             case MODBUS_FC_WRITE_MULTIPLE_REGISTERS:
      |             ^~~~
  CC       upsdrvctl.o
  CC       libserial_nutscan_la-serial.lo
  CC       libserial_nutscan_la-bcmxcp_ser.lo
  CCLD     libdummy.la
  CCLD     libdummy_upsdrvquery.la
  CCLD     libdummy_serial.la
modbus-ascii.c:3149:29: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3149 |         const int func = msg[o+1];
      |                          ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3166:24: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3166 |                 if( msg[o+2] == datalen-1 )
      |                     ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3158:24: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3158 |                 if( msg[o+2] == datalen-1 )
      |                     ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3177:29: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3177 |                 addr = ( msg[o+2] << 8 ) | msg[o+3];
      |                          ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3177:47: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3177 |                 addr = ( msg[o+2] << 8 ) | msg[o+3];
      |                                            ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3158:24: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3158 |                 if( msg[o+2] == datalen-1 )
      |                     ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3190:43: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3190 |             addr = ( msg[o+2] << 8 ) | msg[o+3];
      |                                        ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3191:23: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3191 |             nb = ( msg[o+4] << 8 ) | msg[o+5];
      |                    ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3191:41: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3191 |             nb = ( msg[o+4] << 8 ) | msg[o+5];
      |                                      ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c:3148:30: error: ‘msg’ may be used uninitialized [-Werror=maybe-uninitialized]
 3148 |         const int slave = msg[o+0];
      |                           ~~~^~~~~
modbus-ascii.c:3135:13: note: ‘msg’ declared here
 3135 |     uint8_t msg[MAX_MESSAGE_LENGTH];
      |             ^~~
modbus-ascii.c: At top level:
cc1: all warnings being treated as errors
make[1]: *** [Makefile:2119: modbus-ascii.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:825: all-recursive] Error 1
Error: Process completed with exit code 2.

Note: for the unused values, if they are required by API syntax etc., a NUT_UNUSED_VARIABLE(varname) macro can be used early in method code (after var declarations).

[AppVeyorBot]

❌ Build nut 2.8.2.2137-master failed (commit a85fa6fdc2 by @jimklimov)