First commit of NHS drivers/nhs-nut.c #2692

Challado · 2024-11-25T23:20:31Z

First commit of driver NHS-NUT for NHS senoidal lines.
Signed-off-by: Lucas Willian Bocchi [email protected]

MAINTAINER UPDATE: Started from mailing list submission at https://alioth-lists.debian.net/pipermail/nut-upsdev/2024-November/008046.html

Signed-off-by: Lucas Willian Bocchi [email protected]

jimklimov · 2024-11-26T08:01:35Z

MAINTAINER NOTE: The initial driver code has some Linux-specific bits and can get a number of portability complaints from the NUT CI farm agents running on other platforms. This will be addressed later as the PR evolves to get merged.

jimklimov

First of all, thanks for a solid-looking driver that yields a lot of info. As a stand-alone program, it looks mostly good (some spots with C memory handling are commented, e.g. with upsdata returns).

Most of the requested fixes (maybe I'll handle at least some of those notes myself) would be required to ensure portability of this driver to other platforms than yours, and to improve maintainability by using more of common NUT methods and approaches - probably including the serial-port communications.

Some general concerns are about which logical code landed into which standard methods for NUT driver structure, as documented at docs/new-drivers.txt - fixes there would be needed, but probably simple ones, moving blocks of code into suitably named methods from wrong current locations.

Also a NEWS.adoc entry to announce the driver, and data/driver.list.in HCL entry (or several) to suggest use of this driver for some NHS device name(s), would be useful. You have a lot of names and VA values mapped in a method - perhaps these names can become the first pass of HCL data enhancement.

Also needs a manual page to describe the supported devices and the driver options (per addvar() in code) and maybe other nuances - known caveats, etc. Ideally this comes from a driver author, as they best know if there is anything special to know about the devices.

Finally, as "naming is one of the hardest problems in IT" (citation needed), nhs-nut would need to be changed, I think. There was a guideline (alas, mostly overlooked) to name new drivers as nutdrv_something to help identify them in process listing, and there are several precedents for that. There is also one nut-*ipmi* set of files. But none with nut in the middle/end...
I think following the precedent of bcmxcp, blazer, riello or sms drivers suffuxed with _ser and/or _usb for different media, e.g. drivers/sms_ser.c - this one deserves to be nhs_ser.

jimklimov · 2024-11-26T08:03:26Z

drivers/nhs-nut.c

+#include "nut_stdint.h"
+#include "serial.h"
+#include <stdio.h>
+#include <linux/serial.h>


Probably some NUT CI agents (and end-user non-Linux platforms) will complain about this one.

Generally this can be revised as needed, by comparing to other serial-media drivers (using methods in drivers/serial.c etc.)

UPDATE: As found later, initial code relies on struct serial_struct and TIOCSSERIAL which are only defined there. So platform expansion would need finding equivalents for whatever those bits solved.

drivers/nhs-nut.c

jimklimov · 2024-11-26T11:12:03Z

drivers/nhs-nut.c

+}
+
+void upsdrv_initups(void) {
+    upsdebugx(1,"Start initups()");


upsdrv_initups() should take care of hardware communications init (maybe fail with fatalx() if port is not accessible).

The upsdrv_initinfo() method is called later to detect the device details (maybe fail with fatalx() if no compatible device was found) and set write-once values like device model etc. and populate the list of supported commands and/or settable variables, if any.

Please do check docs/new-drivers.txt for general driver code structure overview.

jimklimov · 2024-11-26T11:12:42Z

drivers/nhs-nut.c

+            strcpy(porta,DEFAULTPORT);
+        else
+            strcpy(porta,device_path);
+        serial_fd = openfd(porta,baudrate);


upsdrv_initups() should take care of hardware communications init (maybe fail with fatalx() if port is not accessible).

The upsdrv_initinfo() method is called later to detect the device details (maybe fail with fatalx() if no compatible device was found) and set write-once values like device model etc. and populate the list of supported commands and/or settable variables, if any.

Please do check docs/new-drivers.txt for general driver code structure overview.

Signed-off-by: Lucas Willian Bocchi [email protected]

1) Miscalculations of battery voltage and amps 2) Miscalculations in battery capacity 3) Miscalculations in time remaining Signed-off-by: Lucas Willian Bocchi [email protected]

…s#2692] Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c: In function ‘print_pkt_data’: nhs-nut.c:464:17: error: zero-length gnu_printf format string [-Werror=format-zero-length] 464 | upsdebugx(1,""); | ^~ Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c:604:59: error: unused parameter ‘size’ [-Werror=unused-parameter] 604 | pkt_data mount_datapacket(unsigned char * datapacket, int size, double tempodecorrido,pkt_hwinfo pkt_upsinfo) { | ~~~~^~~~ nhs-nut.c:604:72: error: unused parameter ‘tempodecorrido’ [-Werror=unused-parameter] 604 | pkt_data mount_datapacket(unsigned char * datapacket, int size, double tempodecorrido,pkt_hwinfo pkt_upsinfo) { | ~~~~~~~^~~~~~~~~~~~~~ Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c:713:43: error: suggest braces around empty body in an ‘if’ statement [-Werror=empty-body] 713 | if (pktdata.perc_output > 100); | ^ nhs-nut.c:713:13: error: this ‘if’ clause does not guard... [-Werror=misleading-indentation] 713 | if (pktdata.perc_output > 100); | ^~ nhs-nut.c:714:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ 714 | pktdata.perc_output = 100; | ^~~~~~~ Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c:797:29: error: iteration 6 invokes undefined behavior [-Werror=aggressive-loop-optimizations] 797 | pkthwinfo.status[i] = get_bit_in_position(&datapacket[14],sizeof(datapacket[14]),i,0); | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c: In function ‘upsdrv_updateinfo’: nhs-nut.c:1733:53: error: format not a string literal and no format arguments [-Werror=format-security] 1733 | upsdebugx(1,alarm); | ^~~~~ Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c: In function ‘upsdrv_makevartable’: nhs-nut.c:2090:88: error: format ‘%f’ expects argument of type ‘double’, but argument 3 has type ‘int’ [-Werror=format=] 2090 | sprintf(help,"Voltage In Percentage to calculate warning low level. Default is %0.2f",DEFAULLTPERC); | ~~~~^ | | | double | %0.2d etc. for other default macros Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c: In function ‘get_bit_in_position’: nhs-nut.c:357:22: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] 357 | if (bit_position >= size * 8) { | ^~ Signed-off-by: Jim Klimov <[email protected]>

Copy-paste typo? nhs-nut.c: In function ‘mount_datapacket’: nhs-nut.c:693:29: error: operation on ‘pktdata.icarregrms_real’ may be undefined [-Werror=sequence-point] 693 | pktdata.icarregrms_real = pktdata.icarregrms_real = pktdata.icarregrms * 30; | ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Jim Klimov <[email protected]>

nhs-nut.c: In function ‘mount_hwinfo’: nhs-nut.c:786:5: error: missing initializer for field ‘end_marker’ of ‘pkt_hwinfo’ [-Werror=missing-field-initializers] 786 | }; | ^ nhs-nut.c:177:18: note: ‘end_marker’ declared here 177 | unsigned int end_marker; | ^~~~~~~~~~ Signed-off-by: Jim Klimov <[email protected]>

Signed-off-by: Jim Klimov <[email protected]>

AppVeyorBot · 2024-11-27T08:35:36Z

❌ Build nut 2.8.2.2413-master failed (commit 41316bc334 by @jimklimov)

Signed-off-by: Jim Klimov <[email protected]>

…ls#2692] Signed-off-by: Jim Klimov <[email protected]>

…etworkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…ix some more markup [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…etworkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

] Signed-off-by: Jim Klimov <[email protected]>

… __func__ [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…t_data() [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…rbosity 1, add __func__ [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…t verbosity 3-4_ [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov · 2024-11-29T02:30:58Z

@Challado : note, large changes of raw C text have landed as NUT code style was applied and some comments reformatted.
Logging verbosity also revised. The nhs.* values moved to experimental.nhs.* namespace.

Overall, most (not all) of the initial review comments above got addressed and resolved.

It would be great if you can confirm that this code base state still builds and works for your device, before I get into some more significant refactoring to share some of your code with common serial.c/.h and vice versa, and/or shuffle it around the file's methods.

…_ser.txt, NEWS.adoc: temporarily constrain nhs_ser to Linux-only NUT builds [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

AppVeyorBot · 2024-11-29T05:11:32Z

✅ Build nut 2.8.2.2435-master completed (commit 3fbaf07f11 by @jimklimov)

jimklimov · 2024-11-29T07:53:13Z

@Challado : upon the closer inspection, I found that you parse hwinfo details (ups.mfr etc.) for every run of upsdrv_updateinfo(). Is that information expected to change often/ever? Usually that is seeded in upsdrv_initinfo() once and left for the duration of the program. (Some devices and drivers, e.g. SNMP, have a notion of semi-static values - like admin contacts or device location - that are re-fetched rarely, because they can be changed out of band e.g. in Web-UI of that UPS).

jimklimov · 2024-11-29T08:02:26Z

NOTE: I've also constrained builds of this driver and its man page to happen by default just on Linux for now, and added respective wording to the NEWS.adoc and the man page itself. Hopefully git revert c857014d62c7e0a4a5c4675d78294386e1be0bb0 some time later would be all that is needed to restore its glory (in some other PR after merging this one, using common "contributor-owned" git workflow without rubbing elbows in someone else's repository/branch :) ).

I've checked that this arrangement does not preclude (cd drivers && make nhs_ser) attempts on those other systems, so experiments to extend the platform support should be easy to iterate.

Challado · 2024-11-29T15:37:45Z

@Challado : note, large changes of raw C text have landed as NUT code style was applied and some comments reformatted. Logging verbosity also revised. The nhs.* values moved to experimental.nhs.* namespace.

Overall, most (not all) of the initial review comments above got addressed and resolved.

It would be great if you can confirm that this code base state still builds and works for your device, before I get into some more significant refactoring to share some of your code with common serial.c/.h and vice versa, and/or shuffle it around the file's methods.

Yep. Compiled and still working here in arm / amd4 platforms.

…upstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov · 2024-12-01T22:19:59Z

NOTE: CI hiccups about openbsd are due to build agent reconfiguration.

jimklimov · 2024-12-04T15:15:11Z

Probably some clean-up remains e.g. to reshuffle code between methods, improve portability, etc. - but as a first code drop that passes CI this is "good enough" to not keep the PR indefinitely open.

…d upsdrv_initups() [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…indent main code a bit [networkupstools#2692] Fixed the retries from being an infinite loop along the way... Signed-off-by: Jim Klimov <[email protected]>

…a-stale related messages and status changes [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…f_needed() [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

Signed-off-by: Jim Klimov <[email protected]>

…packet data interpretation; un-indent more code [networkupstools#2692] Add range-check to not overflow the datapacket[] buffer. Signed-off-by: Jim Klimov <[email protected]>

…winfo() and interpret_pkt_data() logic, separated by domain [networkupstools#2692] Also rename some data points that do not conform to NUT standards, or drop if redundant. Signed-off-by: Jim Klimov <[email protected]>

…at and min_input_power in static vars [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

…ls#2692] Signed-off-by: Jim Klimov <[email protected]>

Lucas Bocchi added 2 commits November 25, 2024 20:16

First commit of NHS drivers/nhs-nut.c

571db05

Some minor alterations

020f12a

Signed-off-by: Lucas Willian Bocchi [email protected]

jimklimov requested changes Nov 26, 2024

View reviewed changes

jimklimov added feature serial port portability We want NUT to build and run everywhere possible labels Nov 26, 2024

jimklimov added this to the 2.8.3 milestone Nov 26, 2024

Lucas Bocchi and others added 15 commits November 26, 2024 11:44

Some small fixes in get_bit_in_position()

f609782

Function have some errors and again need to re-commit code.

a46b4a1

Signed-off-by: Lucas Willian Bocchi [email protected]

Some fixes in

18049ec

1) Miscalculations of battery voltage and amps 2) Miscalculations in battery capacity 3) Miscalculations in time remaining Signed-off-by: Lucas Willian Bocchi [email protected]

drivers/Makefile.am: add nhs-nut to SERIAL_DRIVERLIST [networkupstool…

adfbaa6

…s#2692] Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs-nut.c: fix compiler complaints [networkupstools#2692]

723e062

nhs-nut.c: In function ‘print_pkt_data’: nhs-nut.c:464:17: error: zero-length gnu_printf format string [-Werror=format-zero-length] 464 | upsdebugx(1,""); | ^~ Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs-nut.c: fix trailing whitespace [networkupstools#2692]

5143fce

Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs-nut.c: fix upsdrv_shutdown() [networkupstools#2692]

a4d08cf

Signed-off-by: Jim Klimov <[email protected]>

jimklimov added 6 commits November 27, 2024 10:12

NEWS.adoc: introduce nhs-nut (sic) driver [networkupstools#2692]

c219c15

Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs-nut.c: fix DRIVER_VERSION to double-digits [networkupstoo…

01f01d9

…ls#2692] Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs-nut.c: revise upsdrv_makevartable() content and markup [n…

0e7068e

…etworkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs-nut.c: revise upsdrv_makevartable(): use snprintf() and f…

90d715e

…ix some more markup [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs-nut.c: revise upsdrv_makevartable(): fix help messages [n…

29235a0

…etworkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

docs/man/nhs-nut.txt: introduce new driver manpage [networkupstools#2692

341d424

] Signed-off-by: Jim Klimov <[email protected]>

jimklimov added 4 commits November 29, 2024 01:50

drivers/nhs_ser.c: log method starting/ending quieter and with use of…

f85b97b

… __func__ [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs_ser.c: quieter logging in print_pkt_hwinfo() and print_pk…

8063a21

…t_data() [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs_ser.c: keep debug log of main milestones and errors at ve…

3c6df5b

…rbosity 1, add __func__ [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

drivers/nhs_ser.c: hide upsdrv_updateinfo() debug log of processing a…

c375863

…t verbosity 3-4_ [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov force-pushed the master branch from cb5b4a0 to c375863 Compare November 29, 2024 02:22

configure.ac, drivers/Makefile.am, docs/man/Makefile.am, docs/man/nhs…

c857014

…_ser.txt, NEWS.adoc: temporarily constrain nhs_ser to Linux-only NUT builds [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov added the Linux Some issues are specific to Linux as a platform label Nov 29, 2024

jimklimov and others added 3 commits November 30, 2024 18:02

drivers/nhs_ser.c: populate missed explicit method parameter [network…

112dca3

…upstools#2692] Signed-off-by: Jim Klimov <[email protected]>

Merge branch 'master' into master

1f2ec12

Merge branch 'master' into master

eaad9bb

Merge branch 'master' into master

0864767

jimklimov merged commit 8514451 into networkupstools:master Dec 4, 2024
30 checks passed

jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 9, 2024

drivers/nhs_ser.c: reshuffle HW init code between upsdrv_initups() an…

622b91a

…d upsdrv_initups() [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 9, 2024

drivers/nhs_ser.c: upsdrv_updateinfo(): revise reconnections with dat…

badad3e

…a-stale related messages and status changes [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 9, 2024

drivers/nhs_ser.c: upsdrv_updateinfo(): refactor with reconnect_ups_i…

fd5c353

…f_needed() [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 9, 2024

drivers/nhs_ser.c: use strncpy() to init porta[] [networkupstools#2692]

5a7ee6a

Signed-off-by: Jim Klimov <[email protected]>

jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 9, 2024

drivers/nhs_ser.c: size porta[] as PATH_MAX [networkupstools#2692]

ea015ec

Signed-off-by: Jim Klimov <[email protected]>

jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 9, 2024

drivers/nhs_ser.c: interpret_pkt_data(): cache ah, pf, va, numbat, vb…

8e36408

…at and min_input_power in static vars [networkupstools#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 9, 2024

drivers/nhs_ser.c: pass buffer sizes as size_t not int [networkupstoo…

fb4b88c

…ls#2692] Signed-off-by: Jim Klimov <[email protected]>

jimklimov mentioned this pull request Dec 9, 2024

Refactor nhs_ser driver #2715

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First commit of NHS drivers/nhs-nut.c #2692

First commit of NHS drivers/nhs-nut.c #2692

Challado commented Nov 25, 2024 •

edited by jimklimov

Loading

jimklimov commented Nov 26, 2024

jimklimov left a comment •

edited

Loading

jimklimov Nov 26, 2024

jimklimov Dec 8, 2024

jimklimov Nov 26, 2024

jimklimov Nov 26, 2024

AppVeyorBot commented Nov 27, 2024

jimklimov commented Nov 29, 2024 •

edited

Loading

AppVeyorBot commented Nov 29, 2024

jimklimov commented Nov 29, 2024 •

edited

Loading

jimklimov commented Nov 29, 2024 •

edited

Loading

Challado commented Nov 29, 2024

jimklimov commented Dec 1, 2024

jimklimov commented Dec 4, 2024

First commit of NHS drivers/nhs-nut.c #2692

First commit of NHS drivers/nhs-nut.c #2692

Conversation

Challado commented Nov 25, 2024 • edited by jimklimov Loading

jimklimov commented Nov 26, 2024

jimklimov left a comment • edited Loading

Choose a reason for hiding this comment

jimklimov Nov 26, 2024

Choose a reason for hiding this comment

jimklimov Dec 8, 2024

Choose a reason for hiding this comment

jimklimov Nov 26, 2024

Choose a reason for hiding this comment

jimklimov Nov 26, 2024

Choose a reason for hiding this comment

AppVeyorBot commented Nov 27, 2024

jimklimov commented Nov 29, 2024 • edited Loading

AppVeyorBot commented Nov 29, 2024

jimklimov commented Nov 29, 2024 • edited Loading

jimklimov commented Nov 29, 2024 • edited Loading

Challado commented Nov 29, 2024

jimklimov commented Dec 1, 2024

jimklimov commented Dec 4, 2024

Challado commented Nov 25, 2024 •

edited by jimklimov

Loading

jimklimov left a comment •

edited

Loading

jimklimov commented Nov 29, 2024 •

edited

Loading

jimklimov commented Nov 29, 2024 •

edited

Loading

jimklimov commented Nov 29, 2024 •

edited

Loading