Workaround for broken Eaton (and others?) string language ID descriptor#2604
Conversation
4eefec5 to
a2475e8
Compare
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... |
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 |
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 |
Fixes networkupstools#1925 Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
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 <debian.tormod@gmail.com>
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 <debian.tormod@gmail.com>
…s#1925, networkupstools#2604] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
Also I saw it was fine in test befor... |
|
✅ 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 <jimklimov+nut@gmail.com>
…rkupstools#1925, networkupstools#2604] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…)` method [networkupstools#1925, networkupstools#2604] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
d2b262c to
45c07f3
Compare
|
Finally merge is possible |
Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…on.c [networkupstools#2604, networkupstools#2615] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…c::nut_usb_get_string(): trace the bytes going around [networkupstools#3201, networkupstools#2604] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…c::nut_usb_get_string(): trace the bytes going around [networkupstools#3201, networkupstools#2604] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@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: