On Thu, 2020-02-06 at 13:13 +0100, Hans de Goede wrote: > Hi, > > On 2/6/20 12:51 PM, Filipe Laíns wrote: > > On Thu, 2020-02-06 at 12:30 +0100, Hans de Goede wrote: > > > HI, > > > > > > On 2/6/20 12:14 PM, Filipe Laíns wrote: > > > > Hello, > > > > > > > > Right now the hid-logitech-dj driver will export one node for each > > > > connected device, even when the device is not connected. That causes > > > > some trouble because in userspace we don't have have any way to know if > > > > the device is connected or not, so when we try to communicate, if the > > > > device is disconnected it will fail. > > > > > > I'm a bit reluctant to make significant changes to how the > > > hid-logitech-dj driver works. We have seen a number of regressions > > > when it was changed to handle the non unifying receivers and I would > > > like to avoid more regressions. > > > > > > Some questions: > > > 1. What is the specific use case where you are hitting this? > > > > For example, in libratbag we enumerate the devices and then probe them. > > Currently if the device is not connected, the communication fails. To > > get the device to show up we need to replug it, so it it triggers udev, > > or restart the daemon. > > Thanks, that is exactly the sort of context to your suggested changes > which I need. > > > > 2. Can't the userspace tools involved by modified to handle the errors > > > they are getting gracefully? > > > > They can, but the approaches I see are not optimal: > > - Wait for HID events coming from the device, which could never > > happen. > > - Poll the device until it wakes up. > > I guess we do get some (other or repeated?) event when the device does > actually connect, otherwise your suggested changes would not be possible. No, I was thinking to just send the HID++ version identification routine and see if the device replies. > So how about if we trigger a udev change event on the hid device instead > when this happens ? That seems like a less invasive change on the kernel > side and then libratbag could listen for these change events? Yes, that is a good idea :) I did not know this was possible but it seems like a better approach. > > > 3. Is there a bugreport open about this somewhere? > > > > Yes, https://github.com/libratbag/libratbag/issues/785 > > > > > > The reason we do this is because otherwise we would loose the first > > > > packets when the device is turned on by key press. When a device is > > > > turned on we would have to create the device node, and the packets > > > > received while we are creating the device node would be lost. > > > > > > I don't believe that this is the reason, we only create hid child > > > devices for devices reported by the receiver, but some of the non > > > unifying hid receiver send a list of all devices paired, rather > > > then the ones which are actually connected at that time. > > > > IIRC from my chats with Benjamin and Peter this is the reason, but > > please correct me if I'm wrong. > > Could be that we can distinguish between "paired" and "connected" > and that we are enumerating "paired" but not (yet) "connected" > devices already because of what you say, I've not touched this > code in a while. We create nodes for all paired devices, no matter if they are connected or not. > > > > This could solved by buffering those packets, but that is a bad solution as > > > > it would mess up the timings. > > > > > > > > At the moment the created node includes both normal HID and vendor > > > > usages. To solve this problem, I propose that instead of creating a > > > > single device node that contains all usages, we create one for normal > > > > HID, which would exist all the time, and one for the vendor usage, > > > > which would go away when the device disconnects. > > > > > This slight behavior change will affect userspace. Two hidraw nodes > > > > would be created instead of one. We need to make sure the current > > > > userspace stacks interfacing with this would be able to properly handle > > > > such changes. > > > > > > > > What do you think of this approach? Anyone has a better idea? > > > > > > The suggested approach sounds fragile and like it adds complexity to > > > an already not simple driver. > > > > I understand, that is totally reasonable. I am working on a CI for the > > driver if that helps. > > > > > It would be helpful to first describe the actual problem you are trying > > > to fix (rather then suggesting a solution without clearly defining the > > > problem) and then we can see from there. > > > > I though I described it good enough in the first paragraph but I guess > > not, sorry. You should be able to get it from my comments above, if not > > please let me know :) > > No problem, I have enough context now. I personally like my udev change > event idea, which seems more KISS. But ultimately this is Benjamin's call. Yes, I don't know about the application details (I'll have to find out :P) but it makes more sense to me. It avoids breaking the userspace behavior. Benjamin, what do you think? > Regards, > > Hans Thank you, Filipe Laíns
Attachment:
signature.asc
Description: This is a digitally signed message part
