Nut upsdev - Feb 2018 - [PATCH 2/3] Add generic facility to override HID report descriptor in usbhid-ups

Hi,

I've been checking out NUT with an OpenUPS board over the last couple
of weekends, and have noticed that it doesn't seem to report sensible
values.  This lead me to investigate usbhid-ups and delve into various
issues.

As mentioned in a github issue, one of the problems is with the report
descriptor - dumping this from the usbhid-ups debug output and picking
through it reveals that global items that are supposed to be attached
to main items appear after the corresponding main item, in violation of
the HID standard.  This violation also means that the parser fails to
associate (eg) the correct logical min/max with the proper usages.

For instance, this means that the signed current value reported through
UPS.PowerSummary.Current is interpreted as unsigned.

Some other issues include no logical max/min before the first few
usages, which mean various items describing strings are clamped to zero,
causing the strings to be directed to string 0 rather than their
intended string.

Also, the input current appeared to fall to zero when the battery was
fully charged, despite the UPS taking power from the input supply, and
the output current would change depending on whether the UPS was taking
power from the battery or from the input supply.

Being experienced with switch mode power supplies (to the point of
designing and building them) has allowed me to investigate what is
going on, and resolve these issues.

There is another issue which I haven't addressed yet, which is that
some of the current and voltage values appear to have a zero offset
error - the output voltage, for example, reads 80mV higher than it
actually is (as measured on the capacitors themselves) equivalent to
an error of 5.  The effect of these errors, when combined, is to
inflate the power by about 5 to 10%.  I've chosen not to fix this so
far as the coulomb counter is likely subject to the same error when
calculating the runtime.

The patches attached to this message:

1. Avoid reading non-feature usages.  usbhid-ups does not request
   input or output reports, it always uses the feature type for all
   report requests.  Asking for a report ID for an input or output
   report (which is supposed to be done over the interrupt endpoints)
   with a feature type may be responsible for the growing list of
   driver specific workarounds...

2. Implement a generic method to replace the HID report descriptor.

3. Fix the reporting of input/output currents with openups.

I haven't supplied my replacement HID report descriptor with this set
of patches - my current way of editing this is as assembly, eg:

        .byte 0x05, 0x84                        // UsagePage(0x0084)
        .byte 0x09, 0x04                        // Usage(UPS)
        .byte 0xa1, 0x01                        // Collection(0x01)
        .byte 0x09, 0x24                        // Usage(PowerSummary)
        .byte 0xa1, 0x00                        // Collection(0x00)
	...

assembling that, and the converting the result to binary:

	gcc -c -o openups-hid.o openups-hid.S
	objcopy -O binary openups-hid.o openups-hid.bin

and then using:

	hid_descriptor = /etc/nut/openups-hid.bin

in the /etc/nut/ups.conf section.  Thoughts on how to include this
are welcome.

-- 
Russell King

Input and Output reports are used for interrupt endpoints rather than
control endpoints.  HIDGetItemData() only ever requests feature
report IDs, and requesting non-feature report IDs as feature IDs may
lead to undesirable results and errors.
---
 drivers/libhid.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/libhid.c b/drivers/libhid.c
index f6ec644..1cec74a 100644
--- a/drivers/libhid.c
+++ b/drivers/libhid.c
@@ -301,6 +301,13 @@ void HIDDumpTree(hid_dev_handle_t udev, usage_tables_t
*utab)
 			continue;
 		}
 #else
+		/* Skip non-feature reports, primarily for OpenUPS - but
+		 * as HIDGetItemData() only cares about feature items,
+		 * do it for all.
+		 */
+		if (pData->Type != ITEM_FEATURE)
+			continue;
+
 		if ((vendorID == 0x0463) || (vendorID == 0x047c)) {
 			if ((pData->ReportID == 254) || (pData->ReportID == 255)) {
 				continue;
-- 
2.7.4


-- 
Russell King

Some UPSes contain HID report descriptors that fail to correctly
describe the contents of reports.  For example, the OpenUPS
descriptor fails to set the units, logical maximum and logical
minimum before the corresponding `main item' as required by the HID
specification.

This commit allows an override HID report descriptor to be given to
usbhid-ups which replaces the one produced by the UPS, allowing the
report descriptors to be properly parsed.

With an appropriately fixed report descriptor, OpenUPS reports correct
battery current (+ve for charging, -ve for discharging.)
---
 drivers/usbhid-ups.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/usbhid-ups.c b/drivers/usbhid-ups.c
index 954fa54..5a4d6db 100644
--- a/drivers/usbhid-ups.c
+++ b/drivers/usbhid-ups.c
@@ -748,6 +748,8 @@ void upsdrv_makevartable(void)
 #else
 	addvar(VAR_VALUE, "notification", "Set notification type,
(ignored, only for backward compatibility)");
 #endif
+
+	addvar(VAR_VALUE, "hid_descriptor", "Replacement HID descriptor
file");
 }
 
 #define	MAX_EVENT_NUM	32
@@ -1093,16 +1095,52 @@ static void process_boolean_info(const char *nutvalue)
 static int callback(hid_dev_handle_t udev, HIDDevice_t *hd, unsigned char
*rdbuf, int rdlen)
 {
 	int i;
-	const char *mfr = NULL, *model = NULL, *serial = NULL;
+	const char *mfr = NULL, *model = NULL, *serial = NULL, *descfile;
 #ifndef SHUT_MODE
 	int ret;
 #endif
 	upsdebugx(2, "Report Descriptor size = %d", rdlen);
 	upsdebug_hex(3, "Report Descriptor", rdbuf, rdlen);
 
+	/* Check whether we have an override descriptor */
+	descfile = getval("hid_descriptor");
+	if (descfile) {
+		unsigned char *buf;
+		size_t size;
+		FILE *f;
+
+		f = fopen(descfile, "r");
+		if (!f) {
+			upslog_with_errno(LOG_ERR, "Can't open %s", descfile);
+			return 0;
+		}
+
+		fseek(f, 0, SEEK_END);
+		size = ftell(f);
+
+		fseek(f, 0, SEEK_SET);
+
+		buf = xmalloc(size);
+
+		if (fread(buf, size, 1, f) != 1) {
+			upslog_with_errno(LOG_CRIT, "Short read of %s", descfile);
+			fclose(f);
+			free(buf);
+			return 0;
+		}
+		fclose(f);
+
+		rdbuf = buf;
+		rdlen = size;
+	}
+
 	/* Parse Report Descriptor */
 	Free_ReportDesc(pDesc);
 	pDesc = Parse_ReportDesc(rdbuf, rdlen);
+
+	if (descfile)
+		free(rdbuf);
+
 	if (!pDesc) {
 		upsdebug_with_errno(1, "Failed to parse report descriptor!");
 		return 0;
-- 
2.7.4


-- 
Russell King

Monitoring the input and output currents reported through upsc for an
OpenUPS device suggests that it is an energy creation device - the
power out is greater than the power into the system once the battery
is fully charged.

Analysis and measurement reveals several issues:

1. "UPS.PowerStatus.Output.Current" is scaled for NUTs
"output.current"
   value, which should be the output current from the UPS.

   However, the value reported for "UPS.PowerStatus.Output.Current" is
   not the output from the UPS terminals to the load, but is the
   current into the LTC3780 buck-boost converter on the output.  The
   evidence for this is that with a known constant load, the current
   reported is dependent on whether we are using battery voltage or
   input voltage, and varies as one would expect if it were reporting
   the current supplied to the LTC3780.

2. "UPS.PowerStatus.Input.Current" is scaled for NUTs
"input.current"
   value, which should be the input current to the UPS.

   The value reported for "UPS.PowerStatus.Input.Current" follows and
   matches the current supplied to the battery while charging is taking
   place, whether bulk charging or floating.

In order to fix these issues, the general principle of switch mode
power conversion is employed:

  Power out = Power in * Efficiency (where Efficiency is between 0..1)

The fix for "output.current" involves knowing whether the UPS was
powered from the battery or the input voltage (which is the voltage
fed to the LTC3780), calculating the power into the LTC3780 converter,
and using the above equation to refer it to the output.

Fixing "input.current" requires a similar process, except in reverse.
We calculate the power being supplied into the battery, and refer that
back to the input terminals, remembering to sum the current draw from
the LTC3780.
---
 drivers/openups-hid.c | 100 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 27 deletions(-)

diff --git a/drivers/openups-hid.c b/drivers/openups-hid.c
index 6bd00b6..30e9f26 100644
--- a/drivers/openups-hid.c
+++ b/drivers/openups-hid.c
@@ -42,6 +42,8 @@ static double vin_scale = 1;
 static double vout_scale= 1;
 static double ccharge_scale = 1;
 static double cdischarge_scale = 1;
+static double v_in, v_bat, v_out, i_ltc;
+static int on_input;
 
 static char openups_scratch_buf[20];
 
@@ -129,9 +131,9 @@ static const char *openups_off_fun(double value);
 
 static const char *openups_scale_vin_fun(double value);
 static const char *openups_scale_vout_fun(double value);
-/* static const char *openups_scale_vbat_fun(double value); */
-static const char *openups_scale_ccharge_fun(double value);
-static const char *openups_scale_cdischarge_fun(double value);
+static const char *openups_scale_vbat_fun(double value);
+static const char *openups_scale_input_current_fun(double value);
+static const char *openups_scale_output_current_fun(double value);
 static const char *openups_temperature_fun(double value);
 
 static info_lkp_t openups_charging_info[] = {
@@ -162,16 +164,16 @@ static info_lkp_t openups_vout_info[] = {
 	{0, NULL, openups_scale_vout_fun}
 };
 
-/* static info_lkp_t openups_vbat_info[] = {
+static info_lkp_t openups_vbat_info[] = {
 	{0, NULL, openups_scale_vbat_fun}
-};*/
+};
 
-static info_lkp_t openups_ccharge_info[] = {
-	{0, NULL, openups_scale_ccharge_fun}
+static info_lkp_t openups_input_current_info[] = {
+	{0, NULL, openups_scale_input_current_fun}
 };
 
-static info_lkp_t openups_cdischarge_info[] = {
-	{0, NULL, openups_scale_cdischarge_fun}
+static info_lkp_t openups_output_current_info[] = {
+	{0, NULL, openups_scale_output_current_fun}
 };
 
 static info_lkp_t openups_temperature_info[] = {
@@ -190,6 +192,8 @@ static const char *openups_discharging_fun(double value)
 
 static const char *openups_online_fun(double value)
 {
+	/* Save charging status for input current calculation */
+	on_input = value != 0;
 	return value ? "online" : "!online";
 }
 
@@ -205,31 +209,65 @@ static const char *openups_off_fun(double value)
 
 static const char *openups_scale_vin_fun(double value)
 {
-	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.2f",
value * vin_scale);
+	/* Save v_in for input current calculation */
+	v_in = value * vin_scale;
+	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.2f",
v_in);
 	return openups_scratch_buf;
 }
 
 static const char *openups_scale_vout_fun(double value)
 {
-	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.2f",
value * vout_scale);
+	v_out = value * vout_scale;
+	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.2f",
v_out);
 	return openups_scratch_buf;
 }
 
-/* static const char *openups_scale_vbat_fun(double value)
+static const char *openups_scale_vbat_fun(double value)
 {
-	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.2f",
value * vbat_scale);
+	/* Save v_bat for input current calculation */
+	v_bat = value;
+	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.2f",
value);
 	return openups_scratch_buf;
-}*/
+}
 
-static const char *openups_scale_ccharge_fun(double value)
+/* Input.Current is not the line in current but seems to be the charge
+ * current into the battery terminals.  The real line input current is
+ * determined from the charger power plus the current fed to the LTC3780
+ * buck/boost.
+ */
+static const char *openups_scale_input_current_fun(double value)
 {
-	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.3f",
value * ccharge_scale);
+	double i_in;
+
+	if (on_input && v_in != 0)
+		i_in = (v_bat * value * ccharge_scale) / (v_in * .9) + i_ltc;
+	else
+		i_in = 0;
+
+	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.3f",
i_in);
 	return openups_scratch_buf;
 }
 
-static const char *openups_scale_cdischarge_fun(double value)
+/* Output.Current is not the current flowing out of the terminals of the
+ * board, but is the current flowing into the LTC3780 buck/boost converter.
+ * Hence, in order to calculate the output current, we need to calculate the
+ * power into the LTC3780, take account of its efficiency, and then calculate
+ * the output current.
+ */
+static const char *openups_scale_output_current_fun(double value)
 {
-	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.3f",
value * cdischarge_scale);
+	double v_ltc, i_out;
+
+ 	/* Voltage supplied to the LTC3780 */
+	v_ltc = on_input ? v_in : v_bat;
+
+	/* Current drawn by the LTC3780, save for input current calculation */
+	i_ltc = value * cdischarge_scale;
+
+	/* Output current = Power-in * Efficiency / Vout */
+	i_out = v_ltc * i_ltc * .965 / v_out;
+
+	snprintf(openups_scratch_buf, sizeof(openups_scratch_buf), "%.3f",
i_out);
 	return openups_scratch_buf;
 }
 
@@ -304,7 +342,7 @@ static hid_info_t openups_hid2nut[] = {
 	/* Battery */
 	{"battery.type", 0, 0,
"UPS.PowerSummary.iDeviceChemistry", NULL, "%s",
HU_FLAG_STATIC, stringid_conversion},
 	{"battery.mfr.date", 0, 0,
"UPS.PowerSummary.iOEMInformation", NULL, "%s",
HU_FLAG_STATIC, stringid_conversion},
-	{"battery.voltage", 0, 0, "UPS.PowerSummary.Voltage",
NULL, "%.2f", HU_FLAG_QUICK_POLL, NULL},
+	{"battery.voltage", 0, 0, "UPS.PowerSummary.Voltage",
NULL, NULL, HU_FLAG_QUICK_POLL, openups_vbat_info },
 	/* { "battery.voltage.nominal", 0, 0,
"UPS.PowerSummary.ConfigVoltage", NULL, NULL, HU_FLAG_QUICK_POLL,
openups_vbat_info }, */
 	{"battery.current", 0, 0, "UPS.PowerSummary.Current",
NULL, "%.3f", HU_FLAG_QUICK_POLL, NULL},
 	{"battery.capacity", 0, 0,
"UPS.PowerSummary.DesignCapacity", NULL, "%.0f",
HU_FLAG_STATIC, NULL},
@@ -320,14 +358,6 @@ static hid_info_t openups_hid2nut[] = {
 	{"battery.cell5.voltage", 0, 0,
"UPS.PowerSummary.Battery.Cell5", NULL, NULL, HU_FLAG_QUICK_POLL,
openups_vbat_info},
 	{"battery.cell6.voltage", 0, 0,
"UPS.PowerSummary.Battery.Cell6", NULL, NULL, HU_FLAG_QUICK_POLL,
openups_vbat_info},
 */
-	/* Output */
-	{"output.voltage", 0, 0,
"UPS.PowerSummary.Output.Voltage", NULL, NULL, HU_FLAG_QUICK_POLL,
openups_vout_info},
-	{"output.current", 0, 0,
"UPS.PowerSummary.Output.Current", NULL, NULL, HU_FLAG_QUICK_POLL,
openups_cdischarge_info},
-
-	/* Input */
-	{"input.voltage", 0, 0, "UPS.PowerSummary.Input.Voltage",
NULL, NULL, HU_FLAG_QUICK_POLL, openups_vin_info},
-	{"input.current", 0, 0, "UPS.PowerSummary.Input.Current",
NULL, NULL, HU_FLAG_QUICK_POLL, openups_ccharge_info},
-
 	/* Status */
 	{"BOOL", 0, 0, "UPS.PowerSummary.PresentStatus.Good",
NULL, NULL, HU_FLAG_QUICK_POLL, openups_off_info},
 	{"BOOL", 0, 0,
"UPS.PowerSummary.PresentStatus.InternalFailure", NULL, NULL,
HU_FLAG_QUICK_POLL, commfault_info},
@@ -342,6 +372,22 @@ static hid_info_t openups_hid2nut[] = {
 	{"BOOL", 0, 0, "UPS.PowerSummary.PresentStatus.ACPresent",
NULL, NULL, HU_FLAG_QUICK_POLL, openups_online_info},
 	{"BOOL", 0, 0,
"UPS.PowerSummary.PresentStatus.BatteryPresent", NULL, NULL,
HU_FLAG_QUICK_POLL, openups_nobattery_info},
 
+	/* Output/Input voltages */
+	{"output.voltage", 0, 0,
"UPS.PowerSummary.Output.Voltage", NULL, NULL, HU_FLAG_QUICK_POLL,
openups_vout_info},
+	{"input.voltage", 0, 0, "UPS.PowerSummary.Input.Voltage",
NULL, NULL, HU_FLAG_QUICK_POLL, openups_vin_info},
+
+
+	/* Output/Input currents */
+	/* UPS.PowerSummary.PresentStatus.ACPresent, UPS.PowerSummary.Voltage,
+	 * and UPS.PowerSummary.Output.Voltage must precede this.
+	 */
+	{"output.current", 0, 0,
"UPS.PowerSummary.Output.Current", NULL, NULL, HU_FLAG_QUICK_POLL,
openups_output_current_info},
+	/* UPS.PowerSummary.PresentStatus.ACPresent, UPS.PowerSummary.Voltage,
+	 * UPS.PowerSummary.Output.Current, and
+	 * UPS.PowerSummary.Input.Voltage must precede this.
+	 */
+	{"input.current", 0, 0, "UPS.PowerSummary.Input.Current",
NULL, NULL, HU_FLAG_QUICK_POLL, openups_input_current_info},
+
 	/* end of structure. */
 	{NULL, 0, 0, NULL, NULL, NULL, 0, NULL}
 };
-- 
2.7.4


-- 
Russell King

On Feb 3, 2018, at 7:19 PM, Russell King wrote:
> 
> Input and Output reports are used for interrupt endpoints rather than
> control endpoints.  HIDGetItemData() only ever requests feature
> report IDs, and requesting non-feature report IDs as feature IDs may
> lead to undesirable results and errors.
This one made me scratch my head a bit.

I don't think it's unreasonable to send a control request for an Input
report value-- it seems to be the recommended way for a host to fetch the
initial value (before the device sends updates over the interrupt pipe).

Also, HIDGetItemData() searches the parsed descriptor to match the type as well
as the Usage path, so I'm not sure if I can figure out a way that it would
get an Input report ID confused with a Feature report ID (unless interrupt_only
is set, as it is for powercom devices).

That said, if I am overlooking a case where that might happen, I think we might
need to put this check somewhere deeper in the call tree. HIDDumpTree() is
effectively a NOP if debugging is not enabled, so this is mostly just removing
some partially redundant information from the debug output (redundant in that
most descriptors have both Input and Feature entries for the same Usage IDs).

-- 
Charles Lepple
clepple at gmail

On Feb 3, 2018, at 7:19 PM, Russell King wrote:
> 
> +	addvar(VAR_VALUE, "hid_descriptor", "Replacement HID
descriptor file");
Hi Russell,

I was adding a quick note to the usbhid-ups man page about this parameter, and I
had a few thoughts:

 * Maybe we should call it "read_hid_descriptor_file", and have a
complementary "write_hid_descriptor_file" option? We don't need
the corresponding write code now, but I don't want to rename the read side
after releasing it.

 * I'm wondering if we should require users to specify at least the length
of the descriptor they are replacing (and possibly more info, like a checksum
that the driver could print out in debug mode). My concern is that someone is
going to post their replacement descriptor online, and someone else will just
blindly use it without verifying that it is indeed for their firmware revision.
This case is not as uncommon as one would hope, especially given how
line-powered UPS hardware changes while the model names stay the same.