Merge pull request #2371 from jimklimov/belkin-hid-exponents

Unit-test and fix the Belkin/Liebert HID UPS exponents math

Author: jimklimov

NEWS.adoc

@@ -87,6 +87,10 @@ https://github.com/networkupstools/nut/milestone/10
      `configure` script option `--with-debuginfo`. Note that default autoconf
      behavior usually embeds moderate optimizations and debug information on
      its own. [PR #2310]
+   * A fix applied among clean-ups between NUT v2.7.4 and v2.8.0 releases
+     backfired for `usbhid-ups` subdriver `belkin-hid` which in practice
+     relied on the broken older behavior; more details in its entry below.
+     [PR #2371]
 
  - nut-usbinfo.pl, nut-scanner and libnutscan:
    * Library API version for `libnutscan` was bumped from 2.2.0 to 2.5.0
@@ -182,7 +186,9 @@ https://github.com/networkupstools/nut/milestone/10
      by cooperation with Cyber Power Systems. [#2312]
    * `belkin-hid` subdriver now supports Liebert PSI5 devices which have a
      different numeric reading scale than earlier handled models. [issue #2271,
-     PR #2272, PR #2369]
+     PR #2272, PR #2369] Generally the wrong-scale processing was addressed,
+     including a regression in NUT v2.8.0 which led to zero values
+     in voltage data points which NUT v2.7.4 reported well [#2371]
    * The `onlinedischarge` configuration flag name was too ambiguous and got
      deprecated (will be supported but no longer promoted by documentation),
      introducing `onlinedischarge_onbattery` as the meaningful alias. [#2213]

drivers/belkin-hid.c

@@ -3,7 +3,9 @@
  *  Copyright (C)
  *  2003 - 2008 Arnaud Quette <[email protected]>
  *  2005        Peter Selinger <[email protected]>
- *  2011, 2014  Charles Lepple <clepple+nut@gmail>
+ *  2011, 2014  Charles Lepple <[email protected]>
+ *  2024        James R. Parks <[email protected]>
+ *  2024        Jim Klimov <[email protected]>
  *
  *  Sponsored by MGE UPS SYSTEMS <http://www.mgeups.com>
  *
@@ -31,7 +33,7 @@
 
 #include <math.h>     /* for fabs() */
 
-#define BELKIN_HID_VERSION      "Belkin/Liebert HID 0.20"
+#define BELKIN_HID_VERSION      "Belkin/Liebert HID 0.21"
 
 /* Belkin */
 #define BELKIN_VENDORID	0x050d
@@ -88,32 +90,39 @@ static const char *liebert_charging_fun(double value);
 static const char *liebert_lowbatt_fun(double value);
 static const char *liebert_replacebatt_fun(double value);
 static const char *liebert_shutdownimm_fun(double value);
+
+/* These lookup functions also cover the 1e-7 factor which seems to
+ * be due to a broken report descriptor in certain Liebert units.
+ * Exposed for unit testing - not "static" */
 static const char *liebert_config_voltage_fun(double value);
 static const char *liebert_line_voltage_fun(double value);
-static const char *liebert_psi5_line_voltage_fun(double value);
+
+static double liebert_config_voltage_mult = 1.0;
+static double liebert_line_voltage_mult = 1.0;
+static char liebert_conversion_buf[10];
 
 static info_lkp_t liebert_online_info[] = {
 	{ 0, NULL, liebert_online_fun, NULL }
 };
 
 static info_lkp_t liebert_discharging_info[] = {
-        { 0, NULL, liebert_discharging_fun, NULL }
+	{ 0, NULL, liebert_discharging_fun, NULL }
 };
 
 static info_lkp_t liebert_charging_info[] = {
-        { 0, NULL, liebert_charging_fun, NULL }
+	{ 0, NULL, liebert_charging_fun, NULL }
 };
 
 static info_lkp_t liebert_lowbatt_info[] = {
-        { 0, NULL, liebert_lowbatt_fun, NULL }
+	{ 0, NULL, liebert_lowbatt_fun, NULL }
 };
 
 static info_lkp_t liebert_replacebatt_info[] = {
-        { 0, NULL, liebert_replacebatt_fun, NULL }
+	{ 0, NULL, liebert_replacebatt_fun, NULL }
 };
 
 static info_lkp_t liebert_shutdownimm_info[] = {
-        { 0, NULL, liebert_shutdownimm_fun, NULL }
+	{ 0, NULL, liebert_shutdownimm_fun, NULL }
 };
 
 static info_lkp_t liebert_config_voltage_info[] = {
@@ -124,17 +133,6 @@ static info_lkp_t liebert_line_voltage_info[] = {
 	{ 0, NULL, liebert_line_voltage_fun, NULL },
 };
 
-static info_lkp_t liebert_psi5_line_voltage_info[] = {
-	{ 0, NULL, liebert_psi5_line_voltage_fun, NULL },
-};
-
-static double liebert_config_voltage_mult = 1.0;
-static double liebert_line_voltage_mult = 1.0;
-static char liebert_conversion_buf[10];
-
-/* These lookup functions also cover the 1e-7 factor which seems to be due to a
- * broken report descriptor in certain Liebert units.
- */
 static const char *liebert_online_fun(double value)
 {
 	return value ? "online" : "!online";
@@ -171,8 +169,13 @@ static const char *liebert_shutdownimm_fun(double value)
  */
 static const char *liebert_config_voltage_fun(double value)
 {
-	if( value < 1 ) {
-		if( fabs(value - 1e-7) < 1e-9 ) {
+	/* Does not fire with devices seen diring investigation for
+	 *   https://github.com/networkupstools/nut/issues/2370
+	 * as the ones seen serve nominal "config" values as integers
+	 * (e.g. "230" for line and "24" for battery).
+	 */
+	if (value < 1) {
+		if (fabs(value - 1e-7) < 1e-9) {
 			liebert_config_voltage_mult = 1e8;
 			liebert_line_voltage_mult = 1e7; /* stomp this in case input voltage was low */
 			upsdebugx(2, "ConfigVoltage = %g -> assuming correction factor = %g",
@@ -189,26 +192,33 @@ static const char *liebert_config_voltage_fun(double value)
 
 static const char *liebert_line_voltage_fun(double value)
 {
-	if( value < 1 ) {
-		if( fabs(value - 1e-7) < 1e-9 ) {
+	/* Keep large readings like "230" or "24" as is */
+	if (value < 1) {
+		int picked_scale = 0;
+		/* NOTE: Start with tiniest scale first */
+
+		/* Practical use-case for mult=1e7:
+		 *   1.39e-06  =>  13.9
+		 *   2.201e-05 => 220.1
+		 * NOTE: The clause below is in fact broken for this use-case,
+		 * but was present in sources for ages (worked wrongly with an
+		 * integer-oriented abs() so collapsed into "if (0 < 1e-9) {")!
+		 *   if (fabs(value - 1e-7) < 1e-9) {
+		 */
+		if (fabs(value - 1e-5) < 4*1e-5) {
 			liebert_line_voltage_mult = 1e7;
-			upsdebugx(2, "Input/OutputVoltage = %g -> assuming correction factor = %g",
-				value, liebert_line_voltage_mult);
-		} else {
-			upslogx(LOG_NOTICE, "LineVoltage exponent looks wrong, but not correcting.");
+			picked_scale = 1;
+		} else
+		/* Practical use-case for mult=1e5:
+		 *   0.000273 =>  27.3
+		 *   0.001212 => 121.2
+		 */
+		if (fabs(value - 1e-3) < 4*1e-3) {
+			liebert_line_voltage_mult = 1e5;
+			picked_scale = 1;
 		}
-	}
-
-	snprintf(liebert_conversion_buf, sizeof(liebert_conversion_buf), "%.1f",
-			value * liebert_line_voltage_mult);
-	return liebert_conversion_buf;
-}
 
-static const char *liebert_psi5_line_voltage_fun(double value)
-{
-	if( value < 1 ) {
-		if( fabs(value - 1e-3) < 1e-3 ) {
-			liebert_line_voltage_mult = 1e5;
+		if (picked_scale) {
 			upsdebugx(2, "Input/OutputVoltage = %g -> assuming correction factor = %g",
 				value, liebert_line_voltage_mult);
 		} else {
@@ -507,12 +517,12 @@ static hid_info_t belkin_hid2nut[] = {
   /* Liebert PSI5 */
   { "input.voltage.nominal", 0, 0, "UPS.Flow.ConfigVoltage", NULL, "%.0f", 0, NULL },
   { "input.frequency", 0, 0, "UPS.PowerConverter.Input.Frequency", NULL, "%s", 0, divide_by_100_conversion },
-  { "input.voltage", 0, 0, "UPS.PowerConverter.Input.Voltage", NULL, "%s", 0, liebert_psi5_line_voltage_info },
+  { "input.voltage", 0, 0, "UPS.PowerConverter.Input.Voltage", NULL, "%s", 0, liebert_line_voltage_info },
   { "output.voltage.nominal", 0, 0, "UPS.Flow.ConfigVoltage", NULL, "%.0f", 0, NULL },
   { "output.frequency", 0, 0, "UPS.PowerConverter.Output.Frequency", NULL, "%s", 0, divide_by_100_conversion },
-  { "output.voltage", 0, 0, "UPS.PowerConverter.Output.Voltage", NULL, "%s", 0, liebert_psi5_line_voltage_info },
+  { "output.voltage", 0, 0, "UPS.PowerConverter.Output.Voltage", NULL, "%s", 0, liebert_line_voltage_info },
   { "ups.load", 0, 0, "UPS.OutletSystem.Outlet.PercentLoad", NULL, "%.0f", 0, NULL },
-  { "battery.voltage", 0, 0, "UPS.BatterySystem.Battery.Voltage", NULL, "%s", 0, liebert_psi5_line_voltage_info },
+  { "battery.voltage", 0, 0, "UPS.BatterySystem.Battery.Voltage", NULL, "%s", 0, liebert_line_voltage_info },
   { "battery.voltage.nominal", 0, 0, "UPS.BatterySystem.Battery.ConfigVoltage", NULL, "%.0f", 0, NULL },
   { "battery.capacity", 0, 0, "UPS.Flow.ConfigApparentPower", NULL, "%.0f", 0, NULL },
   /* status */

tests/.gitignore

@@ -16,6 +16,9 @@
 /nuttimetest
 /nuttimetest.log
 /nuttimetest.trs
+/getexponenttest-belkin-hid
+/getexponenttest-belkin-hid.log
+/getexponenttest-belkin-hid.trs
 /getvaluetest
 /getvaluetest.log
 /getvaluetest.trs

tests/Makefile.am

@@ -16,6 +16,7 @@ all: $(TESTS)
 EXTRA_DIST = nut-driver-enumerator-test.sh nut-driver-enumerator-test--ups.conf
 
 TESTS =
+noinst_LTLIBRARIES =
 CLEANFILES = *.trs *.log
 
 AM_CFLAGS = -I$(top_srcdir)/include -I$(top_srcdir)/drivers
@@ -65,7 +66,19 @@ hidparser.c: $(top_srcdir)/drivers/hidparser.c
 	test -s "$@" || ln -s -f "$(top_srcdir)/drivers/hidparser.c" "$@"
 
 if WITH_USB
-TESTS += getvaluetest
+TESTS += getvaluetest getexponenttest-belkin-hid
+
+# We only need to call a few methods, not use the whole source - so
+# not linking it as a getvaluetest_SOURCE file (has too many deps):
+noinst_LTLIBRARIES += libdriverstubusb.la
+nodist_libdriverstubusb_la_SOURCES = driver-stub-usb.c
+libdriverstubusb_la_CFLAGS = $(AM_CFLAGS) $(LIBUSB_CFLAGS)
+#libdriverstubusb_la_LIBADD = $(top_builddir)/common/libcommon.la
+
+getexponenttest-belkin-hid.c: $(top_srcdir)/drivers/belkin-hid.c
+getexponenttest_belkin_hid_SOURCES = getexponenttest-belkin-hid.c
+getexponenttest_belkin_hid_CFLAGS = $(AM_CFLAGS) $(LIBUSB_CFLAGS)
+getexponenttest_belkin_hid_LDADD = $(top_builddir)/common/libcommon.la libdriverstubusb.la
 
 getvaluetest_SOURCES = getvaluetest.c
 nodist_getvaluetest_SOURCES = hidparser.c
@@ -75,6 +88,7 @@ getvaluetest_LDADD = $(top_builddir)/common/libcommon.la
 else !WITH_USB
 EXTRA_DIST += getvaluetest.c hidparser.c
 endif !WITH_USB
+EXTRA_DIST += driver-stub-usb.c
 
 if WITH_GPIO
 TESTS += gpiotest
@@ -101,6 +115,7 @@ EXTRA_DIST += generic_gpio_utest.h generic_gpio_test.txt
 # Make sure out-of-dir dependencies exist (especially when dev-building parts):
 $(top_builddir)/drivers/libdummy_mockdrv.la \
 $(top_builddir)/common/libnutconf.la \
+$(top_builddir)/common/libcommonclient.la \
 $(top_builddir)/common/libcommon.la: dummy
 	+@cd $(@D) && $(MAKE) $(AM_MAKEFLAGS) $(@F)
 

tests/driver-stub-usb.c

@@ -0,0 +1,67 @@
+/* placeholder/mock method implementations to just directly use driver
+ * source code as done for belkin-hid.c (and eventually similar code)
+ * for almost-in-vivo testing (and minimal intrusion to that codebase).
+ * See also: getexponenttest-belkin-hid.c
+ *
+ * Copyright (C)
+ *      2024   Jim Klimov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include "common.h"
+
+#include "usb-common.h"
+int is_usb_device_supported(usb_device_id_t *usb_device_id_list, USBDevice_t *device) {
+	NUT_UNUSED_VARIABLE(usb_device_id_list);
+	NUT_UNUSED_VARIABLE(device);
+	return -1;
+}
+
+#include "usbhid-ups.h"
+hid_dev_handle_t udev = HID_DEV_HANDLE_CLOSED;
+info_lkp_t beeper_info[] = { { 0, NULL, NULL, NULL } };
+info_lkp_t date_conversion[] = { { 0, NULL, NULL, NULL } };
+info_lkp_t stringid_conversion[] = { { 0, NULL, NULL, NULL } };
+info_lkp_t divide_by_10_conversion[] = { { 0, NULL, NULL, NULL } };
+info_lkp_t divide_by_100_conversion[] = { { 0, NULL, NULL, NULL } };
+
+void possibly_supported(const char *mfr, HIDDevice_t *hd) {
+	NUT_UNUSED_VARIABLE(mfr);
+	NUT_UNUSED_VARIABLE(hd);
+}
+
+int fix_report_desc(HIDDevice_t *arg_pDev, HIDDesc_t *arg_pDesc) {
+	NUT_UNUSED_VARIABLE(arg_pDev);
+	NUT_UNUSED_VARIABLE(arg_pDesc);
+	return -1;
+}
+
+#include "libhid.h"
+usage_lkp_t hid_usage_lkp[] = { {NULL, 0} };
+char *HIDGetItemString(hid_dev_handle_t arg_udev, const char *hidpath, char *buf, size_t buflen, usage_tables_t *utab) {
+	NUT_UNUSED_VARIABLE(arg_udev);
+	NUT_UNUSED_VARIABLE(hidpath);
+	NUT_UNUSED_VARIABLE(buf);
+	NUT_UNUSED_VARIABLE(buflen);
+	NUT_UNUSED_VARIABLE(utab);
+	return NULL;
+}
+
+#include "main.h"
+char *getval(const char *var) {
+	NUT_UNUSED_VARIABLE(var);
+	return NULL;
+}