Merge pull request #2718 from jimklimov/issue-1575

`usbhid-ups` generally: apply disable_fix_report_desc…; `cps-hid`: fix mismatched LogMax between input/output voltages (bad encoding)

Author: jimklimov

NEWS.adoc

@@ -77,6 +77,14 @@ https://github.com/networkupstools/nut/milestone/11
      use of `struct timeval={-1,-1}` as a trigger to `select(..., NULL)`,
      as logged in one part of code and not handled in the other, for the
      indefinite wait [#1922, #2392, #2686, #2670]
+   * The `disable_fix_report_desc` option introduced for `usbhid-ups` driver
+     since NUT v2.8.1 was not applied for early dialog with the device while
+     its report descriptors were being discovered. Now this flag, as well as
+     `interruptsize` and `interruptonly`, are considered before we first try
+     to open the USB device handle. [#1575, #1512]
+   * In `cps_fix_report_desc()` we intended to fix-up input and output voltages
+     in certain cases against high voltage transfer, we only fixed-up one of
+     them. [#1245]
 
  - development iterations of NUT should now identify with not only the semantic
    version of a preceding release, but with git-derived information about the
@@ -195,6 +203,9 @@ https://github.com/networkupstools/nut/milestone/11
      by default: `powercom_sdcmd_byte_order_fallback`. [PR #2480]
    * `cps-hid` subdriver now supports more variables, as available on e.g.
      CP1350EPFCLCD model, including temperature. [PRs #2540, #2711]
+   * in `cps-hid` subdriver, `cps_fix_report_desc()` method should now handle
+     mismatched `LogMax` ranges for input and output voltages, whose USB Report
+     Descriptors are wrongly encoded by some firmware versions. [#1512]
    * USB parameters (per `usb_communication_subdriver_t`) are now set back to
      their default values during enumeration after probing each subdriver.
      Having an unrelated device connected with a VID:PID matching the
@@ -229,6 +240,8 @@ https://github.com/networkupstools/nut/milestone/11
    * ...should now not log "insufficient permissions on everything" alone when
      some devices were accessible but just did not match -- clarify that case
      in the next line, when applicable. [PR #2699]
+   * ...should now track the fact of `assumed_LogMax` (typically when firmware
+     encoding logic is wrong, and `-1` is resolved by parser). [#1512, #1040]
 
  - Introduced a new driver concept for interaction with OS-reported hardware
    monitoring readings. Currently instantiated as `hwmon_ina219` specifically

drivers/cps-hid.c

@@ -317,20 +317,26 @@ static int cps_claim(HIDDevice_t *hd) {
 	}
 }
 
-/* CPS Models like CP900EPFCLCD/CP1500PFCLCDa return a syntactically legal but incorrect
- * Report Descriptor whereby the Input High Transfer Max/Min values
- * are used for the Output Voltage Usage Item limits.
+/* CPS Models like CP900EPFCLCD/CP1500PFCLCDa return a syntactically
+ * legal but incorrect Report Descriptor whereby the Input High Transfer
+ * Max/Min values are used for the Output Voltage Usage Item limits.
  * Additionally the Input Voltage LogMax is set incorrectly for EU models.
  * This corrects them by finding and applying fixed
  * voltage limits as being more appropriate.
  */
 
 static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) {
 	HIDData_t *pData;
+	int	retval = 0;
 
 	int vendorID = pDev->VendorID;
 	int productID = pDev->ProductID;
 	if (vendorID != CPS_VENDORID || (productID != 0x0501 && productID != 0x0601)) {
+		upsdebugx(3,
+			"NOT Attempting Report Descriptor fix for UPS: "
+			"Vendor: %04x, Product: %04x "
+			"(vendor/product not matched)",
+			vendorID, productID);
 		return 0;
 	}
 
@@ -343,43 +349,168 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) {
 		return 0;
 	}
 
-	upsdebugx(3, "Attempting Report Descriptor fix for UPS: Vendor: %04x, Product: %04x", vendorID, productID);
+	upsdebugx(3, "Attempting Report Descriptor fix for UPS: "
+		"Vendor: %04x, Product: %04x", vendorID, productID);
 
-	/* Apply the fix cautiously by looking for input voltage, high voltage transfer and output voltage report usages.
-	 * If the output voltage log min/max equals high voltage transfer log min/max then the bug is present.
-	 * To fix it Set both the input and output voltages to pre-defined settings.
+	/* Apply the fix cautiously by looking for input voltage,
+	 * high voltage transfer and output voltage report usages.
+	 * If the output voltage log min/max equals high voltage
+	 * transfer log min/max, then the bug is present.
+	 *
+	 * To fix it set both the input and output voltages to our
+	 * pre-defined settings CPS_VOLTAGE_LOGMIN/CPS_VOLTAGE_LOGMAX.
 	 */
 
-	if ((pData=FindObject_with_ID_Node(pDesc_arg, 16, USAGE_POW_HIGH_VOLTAGE_TRANSFER))) {
+	if ((pData=FindObject_with_ID_Node(pDesc_arg, 16 /* 0x10 */, USAGE_POW_HIGH_VOLTAGE_TRANSFER))) {
 		long hvt_logmin = pData->LogMin;
 		long hvt_logmax = pData->LogMax;
-		upsdebugx(4, "Report Descriptor: hvt input LogMin: %ld LogMax: %ld", hvt_logmin, hvt_logmax);
+		upsdebugx(4, "Original Report Descriptor: hvt input "
+			"LogMin: %ld LogMax: %ld", hvt_logmin, hvt_logmax);
 
-		if ((pData=FindObject_with_ID_Node(pDesc_arg, 18, USAGE_POW_VOLTAGE))) {
+		if ((pData=FindObject_with_ID_Node(pDesc_arg, 18 /* 0x12 */, USAGE_POW_VOLTAGE))) {
 			long output_logmin = pData->LogMin;
 			long output_logmax = pData->LogMax;
-			upsdebugx(4, "Report Descriptor: output LogMin: %ld LogMax: %ld",
-					output_logmin, output_logmax);
+			upsdebugx(4, "Original Report Descriptor: output "
+				"LogMin: %ld LogMax: %ld",
+				output_logmin, output_logmax);
 
 			if (hvt_logmin == output_logmin && hvt_logmax == output_logmax) {
 				pData->LogMin = CPS_VOLTAGE_LOGMIN;
 				pData->LogMax = CPS_VOLTAGE_LOGMAX;
-				upsdebugx(3, "Fixing Report Descriptor. Set Output Voltage LogMin = %d, LogMax = %d",
-							CPS_VOLTAGE_LOGMIN , CPS_VOLTAGE_LOGMAX);
-				if ((pData=FindObject_with_ID_Node(pDesc_arg, 15, USAGE_POW_VOLTAGE))) {
+				upsdebugx(3, "Fixing Report Descriptor: "
+					"set Output Voltage LogMin = %d, LogMax = %d",
+					CPS_VOLTAGE_LOGMIN, CPS_VOLTAGE_LOGMAX);
+
+				if ((pData=FindObject_with_ID_Node(pDesc_arg, 15 /* 0x0F */, USAGE_POW_VOLTAGE))) {
 					long input_logmin = pData->LogMin;
 					long input_logmax = pData->LogMax;
-					upsdebugx(4, "Report Descriptor: input LogMin: %ld LogMax: %ld",
-							input_logmin, input_logmax);
-					upsdebugx(3, "Fixing Report Descriptor. Set Input Voltage LogMin = %d, LogMax = %d",
-							CPS_VOLTAGE_LOGMIN , CPS_VOLTAGE_LOGMAX);
+					upsdebugx(4, "Original Report Descriptor: input "
+						"LogMin: %ld LogMax: %ld",
+						input_logmin, input_logmax);
+
+					pData->LogMin = CPS_VOLTAGE_LOGMIN;
+					pData->LogMax = CPS_VOLTAGE_LOGMAX;
+					upsdebugx(3, "Fixing Report Descriptor: "
+						"set Input Voltage LogMin = %d, LogMax = %d",
+						CPS_VOLTAGE_LOGMIN, CPS_VOLTAGE_LOGMAX);
 				}
 
-				return 1;
+				retval = 1;
 			}
 		}
 	}
-	return 0;
+
+	if ((pData=FindObject_with_ID_Node(pDesc_arg, 18 /* 0x12 */, USAGE_POW_VOLTAGE))) {
+		HIDData_t *output_pData = pData;
+		long output_logmin = output_pData->LogMin;
+		long output_logmax = output_pData->LogMax;
+		bool output_logmax_assumed = output_pData->assumed_LogMax;
+
+		if ((pData=FindObject_with_ID_Node(pDesc_arg, 15 /* 0x0F */, USAGE_POW_VOLTAGE))) {
+			HIDData_t *input_pData = pData;
+			long input_logmin = input_pData->LogMin;
+			long input_logmax = input_pData->LogMax;
+			bool input_logmax_assumed = input_pData->assumed_LogMax;
+
+			if ( (output_logmax_assumed || input_logmax_assumed)
+			/* &&   output_logmax != input_logmax */
+			) {
+				/* We often get 0x0F ReportdId LogMax=65535
+				 * and 0x12 ReportdId LogMax=255 because of
+				 * wrong encoding. See e.g. analysis at
+				 * https://github.com/networkupstools/nut/issues/1512#issuecomment-1224652911
+				 */
+				upsdebugx(4, "Original Report Descriptor: output 0x12 "
+					"LogMin: %ld LogMax: %ld (assumed: %s) Size: %" PRIu8,
+					output_logmin, output_logmax,
+					output_logmax_assumed ? "yes" : "no",
+					output_pData->Size);
+				upsdebugx(4, "Original Report Descriptor: input 0x0f "
+					"LogMin: %ld LogMax: %ld (assumed: %s) Size: %" PRIu8,
+					input_logmin, input_logmax,
+					input_logmax_assumed ? "yes" : "no",
+					input_pData->Size);
+
+				/* First pass: try our hard-coded limits */
+				if (output_logmax_assumed && output_logmax < CPS_VOLTAGE_LOGMAX) {
+					output_logmax = CPS_VOLTAGE_LOGMAX;
+				}
+
+				if (input_logmax_assumed && input_logmax < CPS_VOLTAGE_LOGMAX) {
+					input_logmax = CPS_VOLTAGE_LOGMAX;
+				}
+
+				/* Second pass: align the two */
+				if (output_logmax_assumed && output_logmax < input_logmax) {
+					output_logmax = input_logmax;
+				} else if (input_logmax_assumed && input_logmax < output_logmax) {
+					input_logmax = output_logmax;
+				}
+
+				/* Second pass: cut off according to bit-size
+				 * of each value */
+				if (input_logmax_assumed
+				 && input_pData->Size > 1
+				 && input_pData->Size <= sizeof(long)*8
+				) {
+					/* Note: usually values are signed, but
+					 * here we are about compensating for
+					 * poorly encoded maximums, so limit by
+					 * 2^(size)-1, e.g. for "size==16" the
+					 * limit should be "2^16 - 1 = 65535";
+					 * note that in HIDParse() we likely
+					 * set 65535 here in that case. See
+					 * also comments there (hidparser.c)
+					 * discussing signed/unsigned nuances.
+					 */
+					/* long sizeMax = (1L << (input_pData->Size - 1)) - 1; */
+					long sizeMax = (1L << (input_pData->Size)) - 1;
+					if (input_logmax > sizeMax) {
+						input_logmax = sizeMax;
+					}
+				}
+
+				if (output_logmax_assumed
+				 && output_pData->Size > 1
+				 && output_pData->Size <= sizeof(long)*8
+				) {
+					/* See comment above */
+					/* long sizeMax = (1L << (output_pData->Size - 1)) - 1; */
+					long sizeMax = (1L << (output_pData->Size)) - 1;
+					if (output_logmax > sizeMax) {
+						output_logmax = sizeMax;
+					}
+				}
+
+				if (input_logmax != input_pData->LogMax) {
+					upsdebugx(3, "Fixing Report Descriptor: "
+						"set Input Voltage LogMax = %ld",
+						input_logmax);
+					input_pData->LogMax = input_logmax;
+					retval = 1;
+				}
+
+				if (output_logmax != output_pData->LogMax) {
+					upsdebugx(3, "Fixing Report Descriptor: "
+						"set Output Voltage LogMax = %ld",
+						output_logmax);
+					output_pData->LogMax = output_logmax;
+					retval = 1;
+				}
+			}
+		}
+	}
+
+	if (!retval) {
+		/* We did not `return 1` above, so... */
+		upsdebugx(3,
+			"SKIPPED Report Descriptor fix for UPS: "
+			"Vendor: %04x, Product: %04x "
+			"(problematic conditions not matched)",
+			vendorID, productID);
+	}
+
+	return retval;
 }
 
 subdriver_t cps_subdriver = {

drivers/hidparser.c

@@ -316,12 +316,99 @@ static int HIDParse(HIDParser_t *pParser, HIDData_t *pData)
 
 		case ITEM_LOG_MAX :
 			pParser->Data.LogMax = FormatValue(pParser->Value, ItemSize[pParser->Item & SIZE_MASK]);
-			/* HACK: If treating the value as signed (FormatValue(...)) results in a LogMax that is
-			 * less than the LogMin value then it is likely that the LogMax value has been
-			 * incorrectly encoded by the UPS firmware (field too short and overflowed into sign
-			 * bit).  In that case, reinterpret it as an unsigned number and log the problem.
-			 * This hack is not correct in the sense that it only looks at the LogMin value for
-			 * this item, whereas the HID spec says that Logical values persist in global state.
+			/* HACK: If treating the value as signed (FormatValue(...))
+			 *  results in a LogMax that is less than the LogMin
+			 *  value then it is likely that the LogMax value has
+			 *  been incorrectly encoded by the UPS firmware
+			 *  (field was too short and overflowed into sign bit).
+			 * In that case, reinterpret it as an unsigned number
+			 *  and log the problem. See also *_fix_report_desc()
+			 *  methods that follow up in some *-hid.c subdrivers.
+			 * This hack is not correct in the sense that it only
+			 *  looks at the LogMin value for this item, whereas
+			 *  the HID spec says that Logical values persist in
+			 *  global state.
+			 * Note the values MAY be signed or unsigned, according
+			 *  to rules and circumstances explored below.
+			 *
+			 * RATIONALE: per discussions such as:
+			 * * https://github.com/networkupstools/nut/issues/1512#issuecomment-1238310056
+			 *   The encoding of small integers in the logical/physical
+			 *    min/max fields (in fact, I think in anywhere they
+			 *    encode integers) is independent of the size of
+			 *    the (feature) field they end up referring to.
+			 *   One should use the smallest size encoding
+			 *    (0, 1, 2, or 4 bytes are the options) that can
+			 *    represent, as a signed quantity, the value you
+			 *    need to encode. See HID spec 1.11, sec 6.2.2.2
+			 *    "Short Items". Given a 16 bit report field, with
+			 *    logical values 0..65535, it should use a 0 byte
+			 *    encoding for the logical minimum (0x14, rather
+			 *    than 0x15 0x00) and a 4-byte encoding for the
+			 *    logical maximum (0x27 0xFF 0xFF 0x00 0x00).
+			 *   Their encoding choice does suggest you cannot
+			 *    have an unsigned 32-bit report item with logical
+			 *    maximum >2147483647 (unless you assume, as I did,
+			 *    that "if max < min" then it's just a bad encoding
+			 *    of a positive number that ran into the sign bit).
+			 * * https://github.com/networkupstools/nut/pull/2718#issuecomment-2547021458
+			 *   This is what the spec says (page labelled 19 of
+			 *   hid1_11.pdf, physical page 29 of 97) --
+			 *   5.8 Format of Multibyte Numeric Values
+			 *    Multibyte numeric values in reports are
+			 *    represented in little-endian format, with the
+			 *    least significant byte at the lowest address.
+			 *    The Logical Minimum and Logical Maximum values
+			 *    identify the range of values that will be found
+			 *    in a report.
+			 *    If Logical Minimum and Logical Maximum are
+			 *    both positive values then a sign bit is
+			 *    unnecessary in the report field and the
+			 *    contents of a field can be assumed to
+			 *    be an unsigned value.
+			 *    Otherwise, all integer values are signed
+			 *    values represented in 2's complement format.
+			 *    Floating point values are not allowed.
+			 * * https://github.com/networkupstools/nut/pull/2718#issuecomment-2547065141
+			 *    The number of bytes in the encoding of the
+			 *     LogMin and LogMax fields is only loosely tied
+			 *     to the "Size" of the field that they are
+			 *     describing -- but the implementers on the
+			 *     UPS side don't seem to quite get that.
+			 *    It's all starting to come back to me...
+			 *    If you're trying to describe a report field
+			 *     that is 16-bits and has (unsigned) values
+			 *     from 0..65535 range, then you SHOULD have
+			 *     a LogMin field containing value 0, and
+			 *     a LogMax field that contains value 65535.
+			 *    Since all numeric fields are interpreted as
+			 *     signed "two's-complement" values (except for
+			 *     that note above about the *report values*,
+			 *     NOT the values in the report description), to
+			 *     encode such a LogMax field you would have to
+			 *     express the *LogMax field* in a 4-byte encoding
+			 *     in the *report description*.
+			 *    That's independent of the ultimate 2-byte
+			 *     *report value* that these LogMin and LogMax
+			 *     are describing.
+			 *    We suppose that some coder at the UPS company
+			 *     took a shortcut, and set not only "LogMin = 0",
+			 *     but also effectively "LogMax = -1" (because they
+			 *     used a 2-byte encoding with all bits set, not a
+			 *     4-byte encoding), and then NUT is left to decide
+			 *     what they actually intended.
+			 *    My interpretation of that is that they're trying
+			 *     to say e.g. 0..65535, because if they had meant
+			 *     0..32767 they would have just written that (as
+			 *     0..7FFF which fits in the signed 2-byte repr.),
+			 *     but unless they're actually trying to represent
+			 *     e.g. voltages over 327 V, deciding that the
+			 *     limit is a signed 32767 should also be fine.
+			 *
+			 * ...and maybe some in other tickets
+			 *
+			 * TL;DR: there is likely a mis-understanding
+			 *  of the USB spec by firmware developers.
 			 */
 			if (pParser->Data.LogMax < pParser->Data.LogMin) {
 				upslogx(LOG_WARNING,
@@ -332,6 +419,7 @@ static int HIDParse(HIDParser_t *pParser, HIDData_t *pData)
 					pParser->Data.LogMax,
 					pParser->Value,
 					pParser->Data.ReportID);
+				pParser->Data.assumed_LogMax = true;
 				pParser->Data.LogMax = (long) pParser->Value;
 			}
 			break;

drivers/hidtypes.h

@@ -35,6 +35,7 @@ extern "C" {
 #include <sys/types.h>
 
 #include "nut_stdint.h"
+#include "nut_bool.h"
 
 /*
  * Constants
@@ -308,6 +309,8 @@ typedef struct {
 
 	long		LogMin;				/* Logical Min			*/
 	long		LogMax;				/* Logical Max			*/
+	bool		assumed_LogMax;			/* Logical Max assumed (e.g. "-1" initially)?	*/
+
 	long		PhyMin;				/* Physical Min			*/
 	long		PhyMax;				/* Physical Max			*/
 	int8_t		have_PhyMin;			/* Physical Min defined?		*/