From fc4602b47682c43eb6bf88fa8fc969824d80f6b1 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Thu, 12 Dec 2024 17:34:25 +0100 Subject: [PATCH 01/12] drivers/usbhid-ups.c, NEWS.adoc: apply disable_fix_report_desc, interruptsize and interruptonly before comm_driver->open_dev() [#1512, #1575] Signed-off-by: Jim Klimov --- NEWS.adoc | 3 +++ drivers/usbhid-ups.c | 42 +++++++++++++++++++++++------------------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/NEWS.adoc b/NEWS.adoc index 68e8b9d049..8fc33116fd 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -77,6 +77,9 @@ 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. [#1575, #1512] - development iterations of NUT should now identify with not only the semantic version of a preceding release, but with git-derived information about the diff --git a/drivers/usbhid-ups.c b/drivers/usbhid-ups.c index 286ae36e97..ce1607792d 100644 --- a/drivers/usbhid-ups.c +++ b/drivers/usbhid-ups.c @@ -29,7 +29,7 @@ */ #define DRIVER_NAME "Generic HID driver" -#define DRIVER_VERSION "0.59" +#define DRIVER_VERSION "0.60" #define HU_VAR_WAITBEFORERECONNECT "waitbeforereconnect" @@ -1399,6 +1399,27 @@ void upsdrv_initups(void) subdriver_matcher->next = regex_matcher; #endif /* SHUT_MODE / USB */ + /* First activate the few tweaks which can impact device detection */ + + /* Activate Powercom tweaks */ + if (testvar("interruptonly")) { + interrupt_only = 1; + } + + val = getval("interruptsize"); + if (val) { + int ipv = atoi(val); + if (ipv > 0) { + interrupt_size = (unsigned int)ipv; + } else { + fatalx(EXIT_FAILURE, "Error: invalid interruptsize: %d", ipv); + } + } + + if (testvar("disable_fix_report_desc")) { + disable_fix_report_desc = 1; + } + /* Search for the first supported UPS matching the regular expression (USB) or device_path (SHUT) */ ret = comm_driver->open_dev(&udev, &curDevice, subdriver_matcher, &callback); @@ -1411,10 +1432,7 @@ void upsdrv_initups(void) hd->Vendor ? hd->Vendor : "unknown", hd->Product ? hd->Product : "unknown"); - /* Activate Powercom tweaks */ - if (testvar("interruptonly")) { - interrupt_only = 1; - } + /* Later activate the relatively cosmetic tweaks */ /* Activate Cyberpower/APC tweaks */ if (testvar("onlinedischarge") || testvar("onlinedischarge_onbattery")) { @@ -1544,20 +1562,6 @@ void upsdrv_initups(void) lbrb_log_delay_without_calibrating = 1; } - if (testvar("disable_fix_report_desc")) { - disable_fix_report_desc = 1; - } - - val = getval("interruptsize"); - if (val) { - int ipv = atoi(val); - if (ipv > 0) { - interrupt_size = (unsigned int)ipv; - } else { - fatalx(EXIT_FAILURE, "Error: invalid interruptsize: %d", ipv); - } - } - if (hid_ups_walk(HU_WALKMODE_INIT) == FALSE) { fatalx(EXIT_FAILURE, "Can't initialize data from HID UPS"); } From 732af11088945088e79a6097e1b94b5aed64a0d7 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 13 Dec 2024 10:22:34 +0100 Subject: [PATCH 02/12] drivers/cps-hid.c: cps_fix_report_desc(): line-wrap big comments and debug prints Signed-off-by: Jim Klimov --- NEWS.adoc | 4 +++- drivers/cps-hid.c | 43 +++++++++++++++++++++++++++---------------- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/NEWS.adoc b/NEWS.adoc index 8fc33116fd..1ab00591bb 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -79,7 +79,9 @@ https://github.com/networkupstools/nut/milestone/11 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. [#1575, #1512] + 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] - development iterations of NUT should now identify with not only the semantic version of a preceding release, but with git-derived information about the diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index f29d974046..d167a094a8 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -317,9 +317,9 @@ 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. @@ -343,36 +343,47 @@ 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))) { 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))) { 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); + 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))) { 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); + upsdebugx(3, "Fixing Report Descriptor: " + "set Input Voltage LogMin = %d, LogMax = %d", + CPS_VOLTAGE_LOGMIN, CPS_VOLTAGE_LOGMAX); } return 1; From 3de756d07110cadb40a9eb9745f0e8fa5f724629 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 13 Dec 2024 10:29:40 +0100 Subject: [PATCH 03/12] drivers/cps-hid.c: cps_fix_report_desc(): comment HEX variants of ReportIDs Signed-off-by: Jim Klimov --- drivers/cps-hid.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index d167a094a8..a8f62d4ad9 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -355,13 +355,13 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { * 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, "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, "Original Report Descriptor: output " @@ -375,7 +375,7 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { "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))) { + 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, "Original Report Descriptor: input " From 9bc391e26865fddf634b9216752bdcd546e33634 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 13 Dec 2024 10:40:05 +0100 Subject: [PATCH 04/12] drivers/cps-hid.c: cps_fix_report_desc(): debug-log if we skipped work, to be clear in troubleshooting Signed-off-by: Jim Klimov --- drivers/cps-hid.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index a8f62d4ad9..cae4a2ac60 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -331,6 +331,11 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { 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; } @@ -390,6 +395,14 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { } } } + + /* 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 0; } From f3a9bcc6a18b1214754f32eccedb11d44a52c01d Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 13 Dec 2024 11:32:08 +0100 Subject: [PATCH 05/12] drivers/hidparser.c, drivers/hidtypes.h, NEWS.adoc: track the fact of assumed_LogMax [#1512] Signed-off-by: Jim Klimov --- NEWS.adoc | 2 ++ drivers/hidparser.c | 1 + drivers/hidtypes.h | 3 +++ 3 files changed, 6 insertions(+) diff --git a/NEWS.adoc b/NEWS.adoc index 1ab00591bb..de18bbe552 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -234,6 +234,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 diff --git a/drivers/hidparser.c b/drivers/hidparser.c index a7fb05ec4c..66cc6725da 100644 --- a/drivers/hidparser.c +++ b/drivers/hidparser.c @@ -332,6 +332,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; diff --git a/drivers/hidtypes.h b/drivers/hidtypes.h index 87d42df3c9..6bce592f0b 100644 --- a/drivers/hidtypes.h +++ b/drivers/hidtypes.h @@ -35,6 +35,7 @@ extern "C" { #include #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? */ From f98c8e508d4cb50842447b7a7880ce638081a44a Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 13 Dec 2024 11:56:39 +0100 Subject: [PATCH 06/12] drivers/cps-hid.c, NEWS.adoc: cps_fix_report_desc(): do actually "set Input Voltage" for a fix [#1245] Signed-off-by: Jim Klimov --- NEWS.adoc | 3 +++ drivers/cps-hid.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/NEWS.adoc b/NEWS.adoc index de18bbe552..be2ceec345 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -82,6 +82,9 @@ https://github.com/networkupstools/nut/milestone/11 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 diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index cae4a2ac60..5b8b07e9e0 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -386,6 +386,9 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { 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); From 94d65f67797cb3b2e9563d6f5073cc29837872f1 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Fri, 13 Dec 2024 12:12:46 +0100 Subject: [PATCH 07/12] drivers/cps-hid.c, NEWS.adoc: cps_fix_report_desc(): address mismatched LogMax for input/output voltages [#1512] Note: not bumping version, as it was done in another commit of same PR #2718 Signed-off-by: Jim Klimov --- NEWS.adoc | 3 ++ drivers/cps-hid.c | 114 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/NEWS.adoc b/NEWS.adoc index be2ceec345..04c0e083df 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -203,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 diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index 5b8b07e9e0..fdbd80e0e1 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -327,6 +327,7 @@ static int cps_claim(HIDDevice_t *hd) { 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; @@ -394,19 +395,116 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { CPS_VOLTAGE_LOGMIN, CPS_VOLTAGE_LOGMAX); } - return 1; + retval = 1; + } + } + } + + 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 " + "LogMin: %ld LogMax: %ld (assumed: %s)", + output_logmin, output_logmax, + output_logmax_assumed ? "yes" : "no"); + upsdebugx(4, "Original Report Descriptor: input " + "LogMin: %ld LogMax: %ld (assumed: %s)", + input_logmin, input_logmax, + input_logmax_assumed ? "yes" : "no"); + + /* 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: values are signed, so limit by + * 2^(size-1)-1, e.g. for "size==16" the + * limit should be "2^15 - 1 = 32767"; + * note that in HIDParse() we likely + * set 65535 here in that case. + * Also had to split last "-1" due to + * misfire of "-Werror=parentheses". + */ + long sizeMax = 2^(input_pData->Size - 1); + if (input_logmax >= sizeMax) { + input_logmax = sizeMax - 1; + } + } + + if (output_logmax_assumed + && output_pData->Size > 1 + && output_pData->Size <= sizeof(long)*8 + ) { + /* See comment above */ + long sizeMax = 2^(output_pData->Size - 1); + if (output_logmax >= sizeMax) { + output_logmax = sizeMax - 1; + } + } + + 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; + } } } } - /* 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); + 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 0; + return retval; } subdriver_t cps_subdriver = { From da1b608aab8b14b3ba0ebb0bec425384a8ff9708 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 16 Dec 2024 17:59:58 +0100 Subject: [PATCH 08/12] drivers/cps-hid.c: cps_fix_report_desc(): update debug-logging [#2718] Signed-off-by: Jim Klimov --- drivers/cps-hid.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index fdbd80e0e1..930f4a8401 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -420,14 +420,16 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { * wrong encoding. See e.g. analysis at * https://github.com/networkupstools/nut/issues/1512#issuecomment-1224652911 */ - upsdebugx(4, "Original Report Descriptor: output " - "LogMin: %ld LogMax: %ld (assumed: %s)", + upsdebugx(4, "Original Report Descriptor: output 0x12 " + "LogMin: %ld LogMax: %ld (assumed: %s) Size: %" PRIu8, output_logmin, output_logmax, - output_logmax_assumed ? "yes" : "no"); - upsdebugx(4, "Original Report Descriptor: input " - "LogMin: %ld LogMax: %ld (assumed: %s)", + 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_logmax_assumed ? "yes" : "no", + input_pData->Size); /* First pass: try our hard-coded limits */ if (output_logmax_assumed && output_logmax < CPS_VOLTAGE_LOGMAX) { From cff8cfd431058b8612384349bf68c7922cb2f739 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 16 Dec 2024 18:08:40 +0100 Subject: [PATCH 09/12] drivers/cps-hid.c: cps_fix_report_desc(): fix powers of 2 maths [#2718] Signed-off-by: Jim Klimov --- drivers/cps-hid.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index 930f4a8401..4376b31368 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -461,7 +461,7 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { * Also had to split last "-1" due to * misfire of "-Werror=parentheses". */ - long sizeMax = 2^(input_pData->Size - 1); + long sizeMax = 1L << (input_pData->Size - 1); if (input_logmax >= sizeMax) { input_logmax = sizeMax - 1; } @@ -472,7 +472,7 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { && output_pData->Size <= sizeof(long)*8 ) { /* See comment above */ - long sizeMax = 2^(output_pData->Size - 1); + long sizeMax = 1L << (output_pData->Size - 1); if (output_logmax >= sizeMax) { output_logmax = sizeMax - 1; } From 26998382e1ea3ad444da70d6bcdcc7ee4d875028 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 17 Dec 2024 12:27:38 +0100 Subject: [PATCH 10/12] drivers/hidparser.c, drivers/cps-hid.c: add comments about presumed LogMin/LogMax error origins [#1024, #1512, #2718] Signed-off-by: Jim Klimov --- drivers/cps-hid.c | 3 +- drivers/hidparser.c | 99 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index 4376b31368..2d92b01300 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -457,7 +457,8 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { * 2^(size-1)-1, e.g. for "size==16" the * limit should be "2^15 - 1 = 32767"; * note that in HIDParse() we likely - * set 65535 here in that case. + * set 65535 here in that case. See + * also comments there (hidparser.c). * Also had to split last "-1" due to * misfire of "-Werror=parentheses". */ diff --git a/drivers/hidparser.c b/drivers/hidparser.c index 66cc6725da..e87addd987 100644 --- a/drivers/hidparser.c +++ b/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, From b249c99d1dc18b2393e8342f0836218b73667f6b Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 17 Dec 2024 12:29:22 +0100 Subject: [PATCH 11/12] drivers/cps-hid.c: cps_fix_report_desc(): simplify sizeMax now that original error was caught [#2718] Signed-off-by: Jim Klimov --- drivers/cps-hid.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index 2d92b01300..2a4ee57ac7 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -459,12 +459,10 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { * note that in HIDParse() we likely * set 65535 here in that case. See * also comments there (hidparser.c). - * Also had to split last "-1" due to - * misfire of "-Werror=parentheses". */ - long sizeMax = 1L << (input_pData->Size - 1); - if (input_logmax >= sizeMax) { - input_logmax = sizeMax - 1; + long sizeMax = (1L << (input_pData->Size - 1)) - 1; + if (input_logmax > sizeMax) { + input_logmax = sizeMax; } } @@ -473,9 +471,9 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { && output_pData->Size <= sizeof(long)*8 ) { /* See comment above */ - long sizeMax = 1L << (output_pData->Size - 1); - if (output_logmax >= sizeMax) { - output_logmax = sizeMax - 1; + long sizeMax = (1L << (output_pData->Size - 1)) - 1; + if (output_logmax > sizeMax) { + output_logmax = sizeMax; } } From 315a18321e321ec7131173a3f7f6f49a50a3be82 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 17 Dec 2024 14:32:19 +0100 Subject: [PATCH 12/12] drivers/cps-hid.c: cps_fix_report_desc(): take the higher ground [#2718] Signed-off-by: Jim Klimov --- drivers/cps-hid.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/drivers/cps-hid.c b/drivers/cps-hid.c index 2a4ee57ac7..0882feafb7 100644 --- a/drivers/cps-hid.c +++ b/drivers/cps-hid.c @@ -453,14 +453,18 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { && input_pData->Size > 1 && input_pData->Size <= sizeof(long)*8 ) { - /* Note: values are signed, so limit by - * 2^(size-1)-1, e.g. for "size==16" the - * limit should be "2^15 - 1 = 32767"; + /* 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). + * also comments there (hidparser.c) + * discussing signed/unsigned nuances. */ - long sizeMax = (1L << (input_pData->Size - 1)) - 1; + /* long sizeMax = (1L << (input_pData->Size - 1)) - 1; */ + long sizeMax = (1L << (input_pData->Size)) - 1; if (input_logmax > sizeMax) { input_logmax = sizeMax; } @@ -471,7 +475,8 @@ static int cps_fix_report_desc(HIDDevice_t *pDev, HIDDesc_t *pDesc_arg) { && output_pData->Size <= sizeof(long)*8 ) { /* See comment above */ - long sizeMax = (1L << (output_pData->Size - 1)) - 1; + /* long sizeMax = (1L << (output_pData->Size - 1)) - 1; */ + long sizeMax = (1L << (output_pData->Size)) - 1; if (output_logmax > sizeMax) { output_logmax = sizeMax; }