Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HID Report Descriptor patcher #169

Open
clepple opened this issue Nov 9, 2014 · 12 comments
Open

HID Report Descriptor patcher #169

clepple opened this issue Nov 9, 2014 · 12 comments
Assignees
Labels
enhancement Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) USB USB-HID encoding/LogMin/LogMax Issues and solutions (PRs) specifically about incorrect values in bitstream

Comments

@clepple
Copy link
Member

clepple commented Nov 9, 2014

Many UPSes have bugs in their HID Report Descriptors, and it can be hard to work around them without peppering the HID code with special cases. What we need is a way to match a known buggy HID descriptor, and swap it out for a patched one.

@clepple clepple self-assigned this Nov 9, 2014
@weedy
Copy link

weedy commented Dec 3, 2015

+1
CyberPower is kinda poop

@clepple
Copy link
Member Author

clepple commented Dec 3, 2015

Potentially relevant: http://hidedit.org http://hidedit.org/

Does not include some of the UPS-related usage tables, and does not seem to have a way to import hex.

@clepple
Copy link
Member Author

clepple commented Dec 4, 2015

Update on hidedit: turns out it does read in hex descriptors internally (should be able to interpret NUT debug logs with only a little more effort). However, it seems to be interpreting all numbers as big-endian rather than the USB standard little-endian, and it does not interpret signed numbers correctly.

@clepple
Copy link
Member Author

clepple commented Dec 9, 2015

Starting to address some of the hidedit issues here: https://github.com/clepple/hidedit

@nemoinis
Copy link

Another point of info: with CyberPower CP1500PFCLCD (and probaly others from the reports I've seen) this also affects parameters written by upsrw (e.g. battery.charge.low, battery.runtime.low, etc...).
Wrong parameter values are written to the UPS by upsrw. The manufacturer's own software (powerpanel) writes proper values.

@MattHorn25
Copy link

I am also experiencing what appears to be the same issue with a CyberPower CP1500EPFCLCD, but only with one or two variables - input.voltage may possibly be very slightly incorrect, but output.voltage is definitely reported completely incorrectly. This in particular is the exact same issue as #581 and probably the same as #439 and likely the same as #815.

@juanejot
Copy link

juanejot commented Jan 5, 2021

Ditto to all above through #581 , #439 , #815 , and the note at https://networkupstools.org/ddl/Cyber_Power_Systems/CP1500PFCLCD.html , with my CP1500PFCLCD and its output.voltage reading hovering around 135 ~ 136 V in NUT packages version 2.7.4, even though a kill-a-watt taking output measurements reads 119.6 ~ 120 V. Hope the patched HID descriptor helps in future versions or a patch to this one. Thanks!

@slynn1324
Copy link

Has there been any progress on this? How would one start to help resolving this?

@jimklimov jimklimov added the USB label Nov 14, 2021
@wmigas
Copy link

wmigas commented Dec 22, 2021

@clepple I would like to work on it. I was thinking about following solution

  1. usbhid-ups.h - extending subdriver_t by adding there new method "fix_report"
  2. usbhid-ups.c - modify a callback method
    1. detect subdriver before storing report in buffer
    2. check if subdriver->fix_report != NULL and then execute it
  3. add method to fix_report to subdriver which will fix issue like CPS: patch HID Report Descriptor to fix "output.voltage" #439
static void cps_fix_report(HIDDesc_t *pDesc) {/* fix report issues */
    for (size_t i = 0; i < pDesc->nitems; i++) {
        HIDData_t *pData = &pDesc->item[i];
        if (pData->ReportID == 18) {
            pData->LogMin = 170;//@todo: add support for USA AC (120V), current value works for EU 230V
        }
    }
}

It is simplified version. final version should check product id or product name to apply fix only to specific devices. If there are multiple values stored in one report, then we need to use type and offset
any thoughts about it?

The main downside of this solution is that it requires hardcoding all of these fixes.
Maybe simple config file which will define a report to update by (ReportID, Type ,Offset) and full set of HIDData_t properties which will let override all values. Such approach will give user more flexibility and they will be able to fix HID Report without changing code.

@LJinFLA
Copy link

LJinFLA commented Jan 7, 2022

I am having the same problem. How is this coming along?

@jimklimov
Copy link
Member

A candidate fix is evolving at #1245 / #1246

@jimklimov
Copy link
Member

jimklimov commented Dec 18, 2024

PR #2718 is presumed to fix the broken CPS input/output voltage reports discussed in this issue.

Earlier PR #1245 is presumed to have fixed the broken CPS transfer voltage reports discussed in #439 in more detail.

Generally speaking, a series of improvements made in NUT v2.8.0 up till (upcoming) v2.8.3 have made possible the detection of presumed invalid encodings for LogMin/LogMax values, as well as mitigation in form of *_fix_report_desc() methods, pioneered in a couple of usbhid-ups subdrivers.

There is an idea floating around to devise a way for such fixes to be in external files, so users might apply them without recompiling a new NUT version. In that regard, DMF comes to mind as the transport. I hope to mainstream it as part of NUT v2.8.4, and it would deliver the core logic of loadable mapping tables and LUA scripted functions, and one example of applied logic for SNMP mappings with a leaner driver binary and a separate collection of XML files for the mapping data. The next stop was expected to be USB or QX mappings, as those codebases are mostly structured to easily plug this approach into them. I don't know how welcoming it would be to something as low-level'ish (and maybe performance critical in a loop) as investigation and tweak of every report descriptor, but we'd have a chance to at least experiment with that.

Not closing this issue, so we have an anchor for future development in this area.

@jimklimov jimklimov added the Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) USB USB-HID encoding/LogMin/LogMax Issues and solutions (PRs) specifically about incorrect values in bitstream
Projects
None yet
Development

No branches or pull requests

9 participants