Workaround for broken Eaton (and others?) string language ID descriptor #2604
Conversation
tormodvolden
force-pushed
the
eaton_broken_strings
branch
from
4eefec5
to
a2475e8
Compare
tormodvolden
force-pushed
the
eaton_broken_strings
branch
from
a2475e8
to
6d664a1
Compare
Wow , Thanks a lot , sure i will test it ... |
|
I will try to compile without unraid nut plugin as author of plugin don't have time for it... |
tormodvolden
force-pushed
the
eaton_broken_strings
branch
2 times, most recently
from
7d61ea3
to
a3d2773
Compare
|
Can I test now? |
|
Yes. I had only done minor/optical refinements. |
|
@tormodvolden Thanks a lot , YES, after compiled i have all info : @jimklimov just |
|
Also maybe it seems like the last character is missing in values, also in serial number |
|
Thanks for your testing. I probably made an off-by-one error in the string decoding. I'll fix that later. |
|
The missing last character should be fixed now. |
Now its fine and all values avaliable : |
|
@jimklimov when you will have time, please check it, the issue is solved :) |
|
Sounds great, thanks a lot! I'll take a closer look later though. Interesting how |
|
@tormodvolden : do you expect this fix is generic enough to drop the need for older |
I'm not sure but maybe |
|
@tormodvolden do you need to finish also |
tormodvolden
force-pushed
the
eaton_broken_strings
branch
from
5b904b1
to
91fd69d
Compare
|
@masterwishx I have reworked it to address the most important TODO items. It should now work equally well with libsb-1.0 and libusb0 so I would appreciate if you can test both. @jimklimov The USB code in nut seems complicated by the burden of supporting both libusb0 and libusb1. What are the reasons for supporting libusb0? There seems to be drivers using the libusb0.c and libusb1.c through the "API" and there seems to be some drivers that do their own stuff. And an outlier tools/nut-scanner/scan_usb.c which does its own dlopen() fun and supports both. Then there are wrappers trying to unify the APIs but not being consistently used. Unifying all this and tidying up would probably help in the long run but could be a lot of work. I would suggest as a next step to get rid of all direct use of usb_get_string_simple() and libusb_get_string_descriptor_ascii() and instead use the "API" functions or the new common nut_usb_get_string(). |
The fix in this PR doesn't honour the "langid_fix" option, but will always try to auto-detect by reading out the language ID. Having a way to override the fallback value of en_US could possibly be useful. This option should be a generic one, not limited to a single driver. I don't know if anyone used this option to set anything else than en_US (0x0409). If it only was used for setting 0x0409 and the affected devices (only seen in nutdrv_qx and blazer_usb) won't get hosed up by a langid request (but only return an invalid descriptor) then this option can be dropped now. EDIT: One device in data/driver.list.in uses 0x4095. I am not sure the "noscanlangid" option adds much value, unless some broken device get hosed up by such requests. Other code is already "scanning" it (reading out the language ID), like is being done here in nut_usb_get_string() as well. The option might be just leftovers from previous work. Please check with those who use it (only seen in nutdrv_qx/hunnox). EDIT: The "hunnox" protocol seems to use a port-knocking scheme involving a sequence of non-standard string descriptors retrievals to unlock the device communication, so it has to do precise low-level string descriptor fetching, incompatible with the USB standard. Maybe nut_usb_get_string() will work fine after the unlocking. |
Sure I can test it, but don't know how to test both? |
|
Regarding libusb-0.1, on one hand this code was with NUT much longer (1.0 only getting into mainline a couple of years back with 2.8.0, so much less exposed and tested). On the other, NUT aims to please older platforms (and machines) where it once ran, and those may lack a build/package of libusb-1.0, I suppose. |
After a fresh checkout or running "make distclean": |
|
As for testing, if the libusb-0.1-dev or similar is available, you can |
|
Let me know if there is any platform where nut builds but there is no libusb-1.0. |
What I compiled in Slackware VM last time for test was nut.SlackBuild from Unraid NUT plugin modified by me. So it was libusb 1.0.26 that my Unraid version has. So I can test latest PR tomorrow same way, but for libusb 0.1 I can add |
The oldest occasionally tested that I know of, because there still is practical interest, is Solaris 8. Gotta check if that involves USB, and which, though. |
Compiled with will check with |
…s#1925, networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
|
Bumped the PR with a NEWS entry and resync with current master. @tormodvolden : if you wish to squash some of the commits and rebase - feel free, just re-fetch to have this NEWS change attached. Or I can just merge as soon as CI does not bork on anything relevant. Law of big numbers: something random does hiccup eventually. And thanks again for all the hard work on analysis and implementation, and bearing with us :D |
|
❌ Build nut 2.8.2.2108-master failed (commit a87ba77d3c by @jimklimov) |
Not sure what Appveyor disliked here, build/test were ok. So this fault is unrelated to this PR. FWIW, it listed some DLLs that were not in mingw (FOSS) locations but that's normal (e.g. OS own libs). Not sure if the list is same as before, or is why the step failed. Maybe |
…Vendor, Product, Serial if NULL, after claiming it by other criteria [networkupstools#2562]" This reverts commit ad83b70. The reverted commit attempted to workaround a string descriptor retrieval issue by retrying after claiming an interface on the device. However, this did not help, and it was later found out that a broken langid descriptor was the root issue all along. Signed-off-by: Tormod Volden <[email protected]>
…ce->Vendor, Product, Serial if NULL, after claiming it by other criteria [networkupstools#2562]" This reverts commit ea99f96. The reverted commit attempted to workaround a string descriptor retrieval issue by retrying after claiming an interface on the device. However, this did not help, and it was later found out that a broken langid descriptor was the root issue all along. Signed-off-by: Tormod Volden <[email protected]>
…ial a few times if failed on the first" This reverts commit 0fa5687. The reverted commit introduced retries when failing to retrieve various device string descriptors. In the following commits we will attempt this in a more centralized way, and for the purpose of clarity it is easiest to retract this and start with cleaner sheets. Signed-off-by: Tormod Volden <[email protected]>
This function doesn't use usb_get_string_simple() from libusb0 or libusb_get_string_descriptor_ascii() from libusb1 but replaces them to be able to add a workaround for devices with broken langid descriptors. If the langid descriptor is invalid, the en_US language is assumed. (See networkupstools#1925) It also adds retries to the string descriptor fetching, which may help in some cases. (See networkupstools#414) A small delay is introduced between the retries. Signed-off-by: Tormod Volden <[email protected]>
Fixes networkupstools#1925 Signed-off-by: Tormod Volden <[email protected]>
Signed-off-by: Tormod Volden <[email protected]>
The string index is a number between 1 and 255, simply because it is the lower byte of wValue in the GET_DESCRIPTOR request. String index 0 is used for retrieving the langid array, so it is not valid for this function. There is no reason to impose a limit to the length of the buffer that the user is lending us, certainly not above the range of the involved types, so just a minimal sanity check is enough. Since in the minimal case we would return an empty zero-terminated string, it must at least have room for this. Signed-off-by: Tormod Volden <[email protected]>
The original names could also be confused with libusb-1.0 API functions. Also rename nut_libusb_strerror() although not a nut API function. Signed-off-by: Tormod Volden <[email protected]>
…s#1925, networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
Also I saw it was fine in test befor... |
…t_libusb_set_altinterface() to maintain a consistent code style [networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
…rkupstools#1925, networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
…)` method [networkupstools#1925, networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
|
✅ Build nut 2.8.2.2109-master completed (commit 0511491169 by @jimklimov) |
…t_libusb_set_altinterface() to maintain a consistent code style [networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
…rkupstools#1925, networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
…)` method [networkupstools#1925, networkupstools#2604] Signed-off-by: Jim Klimov <[email protected]>
tormodvolden
force-pushed
the
eaton_broken_strings
branch
from
d2b262c
to
45c07f3
Compare
|
Finally merge is possible |
Signed-off-by: Jim Klimov <[email protected]>
…on.c [networkupstools#2604, networkupstools#2615] Signed-off-by: Jim Klimov <[email protected]>
@masterwishx has tested this with his Eaton device, both on libusb0 and libusb1. Thanks!
The first two commits revert something that was an unsuccessful attempt at fixing something that turned out to be a buggy device. A more effective workaround is suggested in commits 4-5, which also tidies up and unifies things. Commit 3 reverts "retrials" for easier refactoring in 4-5 and similar functionality is added back in a more unified way in commit 4. Commits 6-7-8 are just tidying up things for future work, not strictly needed.
Fixes #1925
References #414
TODOs fixed in rework:
Ideas for future work in other PRs: