On Thu, Nov 10, 2022 at 1:24 PM Andreas Bergmeier <abergmeier@xxxxxxx> wrote: > > > > On Thu, 10 Nov 2022, Benjamin Tissoires wrote: > > > On Thu, Nov 10, 2022 at 4:29 AM Andreas Bergmeier <abergmeier@xxxxxxx> wrote: > > > > > > On Wed, 9 Nov 2022 at 21:27, Andreas Bergmeier <abergmeier@xxxxxxx> wrote: > > > > > > > > Finally I have an environment where I can test my kernel code. > > > > > > > > On Mon, 31 Oct 2022 at 10:29, Benjamin Tissoires > > > > <benjamin.tissoires@xxxxxxxxxx> wrote: > > > > > For identifying the GLOW device you should be adding an id in the > > > > > table of hid-logitech-hidpp, with a driver data that tells the driver > > > > > to look for 0x1990. > > > > > > > > > > > > > > > > > > - you need to add a hook in connect_event to register the led class > > > > > > > device that will hook on to the actual LED of the device > > > > Sadly my tests did not go very far. The code fails already when > > > > calling the `probe` callback (`hidpp_probe`). > > > > When it calls into `hidpp_root_get_protocol_version` it seems to > > > > receive `HIDPP_ERROR_RESOURCE_ERROR`. > > > > Which then leads to an error message: Device not connected > > > > Upon looking at `HIDPP_ERROR_RESOURCE_ERROR` (9) there is no > > > > documentation what it means in code. > > > > From a look into the docs it says that 9 is UNSUPPORTED error for 2.0 > > > > devices. Thus I am wondering how the code knows > > > > that it is a problem with connectivity. > > > > From the top of my memory, this was told to us that this is the way we > > detect if the device was connected or not in the unifying case. Though > > in your case, it's a USB device, so there is no such thing as "not > > connected"... > So isn't the current error handling at a minimum misleading? > Maybe, but this is the first time we have that error on a non wireless device... so it is not for the supported devices :) > > > > > Couldn't it also mean that the > > > > device is not supporting getting the protocol version? > > > > Probably. What happens if you comment out that protocol version > > request and force connected to be true? > Well I went the other way around. I had a look at the hidpp utility > sources: > https://github.com/cvuchener/hidpp/blob/057407fbb7248bbc6cefcfaa860758d0711c01b9/src/libhidpp/hidpp/Device.cpp#L82 > Which seems to do a similar thing. From the top of my head the only > difference seems to be that they are sending `0x1` as a ping value instead > of `0x5a`. Might give that a shot next. > Anyway hidpp-list-features successfully reads the protocol version in > userspace (4, 2) as seen here: > https://github.com/abergmeier/litra_glow_linux/blob/main/hidpp-list-features Hmm... It would seem wrong for me if the firmware suddenly expects to have a specific ping value. If it works in userspace, it might also mean that the timing is not right and we are talking to the device too early, and it can't answer yet. > > > In your case though, it would be interesting to know if we should > > bypass that verification. > Since reading the protocol version seems generally possible I think we > should understand what logitech-hidpp does wrong first. > > > > > Also, looking into `supported_reports` turned out to be 2 (very long). > > > > Oops, you mistook the bit definition with the value: > > #define HIDPP_REPORT_SHORT_SUPPORTED BIT(0) -> value of 1 > > #define HIDPP_REPORT_LONG_SUPPORTED BIT(1) -> value of 2 > > #define HIDPP_REPORT_VERY_LONG_SUPPORTED BIT(2) -> value of 4 > Ah indeed, thx. > > > And this is expected because you don't have VERY_LONG support on your device. > True. The question remains whether the upgrade from LONG to VERY_LONG > could be needed should a device only support VERY_LONG. > I don't think we want that. AFAICT, devices supporting VERY_LONG are not very common, and also the length of the report makes it not convenient for the generic protocol to be used there. I am not in the head of Logitech's engineers, but removing the 20 bytes long report in favor of 64 bytes long seems a little bit overkill (the 7 bytes long is a different story). And given that we generally add manual support of new devices, I think we are safe not implementing something we haven't seen in the wild. Cheers, Benjamin
