| Message ID | 20190410145459.11430-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | HID: logitech: Handling of non DJ receivers in hid-logitech-dj | expand |
Hi Hans, On Wed, Apr 10, 2019 at 4:55 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > Here is v2 of Benjamin's and mine series to enumerate the devices behind > various non DJ receivers instead of relying on their HID proxy mode. Thanks for the re-spin. I spent a good amount of time today reviewing the series and testing it, and I am happy with it. There are a couple of nitpicks I'd like to see addressed, but otherwise, I think this is ready for 5.2. Checkpatch complains about 2 "ERROR: do not use assignment in if condition" in patches 19/37 and 27/37. Though I can see the benefit of having it that way, I think we should stick to checkpatch here as it is too tempting to say "hey, there's a typo here, let's replace `=` by `==`" Checkpatch also complains about: - trailing statements should be on next line: but I do not really mind those, the blocks it complains are clearer that way - lines over 80 characters -> these are fine in that context - Block comments use a trailing */ on a separate line -> please fix those (patches 7/37 and 8/37) as the rest of the code has the correct block comments style > > There are 3 major changes in v2: > > 1) Add a new HID group for 27 MHz devices, this has 2 advantages: > 1a) It allows the logitech-hidpp code to identity 27 Mhz devices and to > automatically enable some HID++ quirls for these, such as e.g. HID++ > mouse-wheel handling (fixing some hwheel issues) > 1b) Since the logitech-hidpp code can now identify these the logitech-dj > code for 27 MHz devices no longer needs separate 27 MHz descriptors for the > consumer and HID++ reports. I am not entirely convinced we need a new group. But OTOH, being able to quickly and reliably filter those legacy device is something that is useful. > > 2) Add support Logitech Bluetooth receivers in HID proxy mode, tested with a > MX5000 keyboard + receiver. This allows e.g. battery monitoring of the kbd. > I played today with a diNovo mini which has a slightly different Logitech BT receiver. This part works fine except for the integrated touchapd which provides reports with an ID 5. I locally hacked something to have the mouse working, but it's a hack. The only problem I have with integrating the diNovo mini is that hid-lg.c does an input_mapping for one key on the keyboard... And I think we will need it too in hid-logitech-hidpp :/ I also added the G700 locally, and a small modification to support unnumbered mouse reports of size 8 is required, but I can send that as a followup patch. Cheers, Benjamin > 3) Rework HID++ mouse-wheel and extra-buttons support making it more > generic and cleaner and add HID++ consumer-keys report. Using the HID++ > consumer keys reports fixes several keys not working on some kbds. > > > Per patch info, unlisted patches are unchanged: > > [PATCH 01/37] HID: quirks: do not blacklist Logitech devices > -New patch split of from a cleanup patch from Benjamin which was touching > many different drivers at once. This removes a dep on that patch. > > [PATCH 12/37] HID: logitech-dj: support sharing struct dj_receiver_dev between USB-interfaces > -Fixed calling hid_err on a NULL hdev > > [PATCH 16/37] HID: logitech-dj: add support for 27 MHz receivers > -Remove chunk to pick a better name for hidpp devices, see new patch below > -Add support for mouse on index 2, numeric-keypad on index 4 > -Use a new group for 27MHz devices > > [PATCH 20/37] HID: logitech-dj: pick a better name for non-unifying receivers > -New patch replacing the dropped chunk from patch 16 as well as replacing > "HID: logitech-hidpp: create a name based on the type if non available" > from v1 > > [PATCH 21/37] HID: logitech-dj: remove false-positive error ... > -New misc. fix patch > > [PATCH 22/37] HID: logitech-dj: make appending of the HID++ ... > [PATCH 23/37] HID: logitech-dj: add support for Logitech Bluetooth > -2 new patches adding support for Logitech BT receivers in HID proxy mode > > [PATCH 26/37] HID: logitech-hidpp: ignore very-short or empty > [PATCH 27/37] HID: logitech-hidpp: do not make failure to get the > [PATCH 28/37] HID: logitech-hidpp: remove double assignment from > [PATCH 29/37] HID: logitech-hidpp: remove unused > -4 new patches (misc. fixes / cleanups) > > [PATCH 31/37] HID: logitech-hidpp: handle devices attached to > -Add support for the new 27 MHz lg devices hid group > > [PATCH 32/37] HID: logitech-hidpp: do not hardcode very long > -This fixes logitech-hidpp rejecting the mx5000 keyboard when paired over BT > > [PATCH 34/37] HID: logitech-hidpp: make hidpp10_set_register_bit a > [PATCH 35/37] HID: logitech-hidpp: add support for HID++ 1.0 wheel > [PATCH 36/37] HID: logitech-hidpp: add support for HID++ 1.0 extra > [PATCH 37/37] HID: logitech-hidpp: add support for HID++ 1.0 > -Reworked HID++ mouse-wheel and extra-buttons support making it more > generic and add HID++ consumer-keys report support > > > > For reference here is the cover letter of v1 of the series: > > Here is a series Benjamin and I have been working on, before this series we > treat Logitech wireless mice / keyboards connected through a non-unifying > receiver as generic HID devices, using the generic HID emulation of the > receiver. This causes several problems: > > -We cannot properly support some special keys / buttons on some keyboards / > mice since we cannot properly identify / talk to the device behind the recv. > -We cannot monitor the battery in these devices > > This series addresses this by actually enumerating (and talking to) the > devices behind the receiver, rather then solely relying on the receiver's > generic HID emulation. > > Note this series applies on top of the: > "[PATCH] HID: force setting drvdata to NULL when removing the driver" > patch which Benjamin send out 2 days ago. > > Regards, > > Hans >
Hi Benjamin, On 4/19/19 5:52 PM, Benjamin Tissoires wrote: > Hi Hans, > > On Wed, Apr 10, 2019 at 4:55 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> Here is v2 of Benjamin's and mine series to enumerate the devices behind >> various non DJ receivers instead of relying on their HID proxy mode. > > Thanks for the re-spin. I spent a good amount of time today reviewing > the series and testing it, and I am happy with it. > There are a couple of nitpicks I'd like to see addressed, but > otherwise, I think this is ready for 5.2. > > Checkpatch complains about 2 "ERROR: do not use assignment in if > condition" in patches 19/37 and 27/37. Though I can see the benefit of > having it that way, I think we should stick to checkpatch here as it > is too tempting to say "hey, there's a typo here, let's replace `=` by > `==`" Hmm, ok, refactoring this to not have the assignments in the if is going to be not pretty, but as you wish. > Checkpatch also complains about: > - trailing statements should be on next line: but I do not really mind > those, the blocks it complains are clearer that way > - lines over 80 characters -> these are fine in that context > - Block comments use a trailing */ on a separate line -> please fix > those (patches 7/37 and 8/37) as the rest of the code has the correct > block comments style Ok, will fix. >> There are 3 major changes in v2: >> >> 1) Add a new HID group for 27 MHz devices, this has 2 advantages: >> 1a) It allows the logitech-hidpp code to identity 27 Mhz devices and to >> automatically enable some HID++ quirls for these, such as e.g. HID++ >> mouse-wheel handling (fixing some hwheel issues) >> 1b) Since the logitech-hidpp code can now identify these the logitech-dj >> code for 27 MHz devices no longer needs separate 27 MHz descriptors for the >> consumer and HID++ reports. > > I am not entirely convinced we need a new group. But OTOH, being able > to quickly and reliably filter those legacy device is something that > is useful. Right, I use this in several hid-logitech-hidpp.c patches. >> 2) Add support Logitech Bluetooth receivers in HID proxy mode, tested with a >> MX5000 keyboard + receiver. This allows e.g. battery monitoring of the kbd. >> > > I played today with a diNovo mini which has a slightly different > Logitech BT receiver. This part works fine except for the integrated > touchapd which provides reports with an ID 5. I locally hacked > something to have the mouse working, but it's a hack. > The only problem I have with integrating the diNovo mini is that > hid-lg.c does an input_mapping for one key on the keyboard... And I > think we will need it too in hid-logitech-hidpp :/ Hmm, hid-lg.c also does mapping for a bunch of 27MHz keyboards, see lg_wireless_mapping. I've prepared patch to add similar mappings in hwdb. But if you re worried about this causing regressions then we should probably add a similar set of mappings to hid-logitech-hidpp.c Ideally we would drop the mapping from hid-lg.c at the same time, but even though I've 7 27MHz receivers, spreading 4 different models, they _all_ use the same vid:pid. I've been unable to find any receivers using the other 2 PIDs marked as LG_WIRELESS in hid-lg.c . I strongly suspect that the other 2 PIDs will work fine with the new 27MHz support too, but first we need to find users who can confirm this. Once this is confirmed lg_wireless_mapping can be dropped, so if we decide to add this to hid-logitech-hidpp.c then in essence we are just moving code, not adding it. I can understand if you're worried about regressions for the 27Mhz kbds, but IMHO just going with hwdb mappings is fine. The keys which need mapping are mostly Fn +F1 .. Fn + F12 and the mappings of those are different per keyboard model. So the current in kernel mappings are wrong as often as they are right. OTOH not having any mappings means the keys do not do anything, while with the mappings from lg_wireless_mapping they at least do something... > I also added the G700 locally, and a small modification to support > unnumbered mouse reports of size 8 is required, but I can send that as > a followup patch. Sounds good. Regards, Hans
Hi, On 19-04-19 17:52, Benjamin Tissoires wrote: > Hi Hans, > > On Wed, Apr 10, 2019 at 4:55 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> Here is v2 of Benjamin's and mine series to enumerate the devices behind >> various non DJ receivers instead of relying on their HID proxy mode. > > Thanks for the re-spin. I spent a good amount of time today reviewing > the series and testing it, and I am happy with it. > There are a couple of nitpicks I'd like to see addressed, but > otherwise, I think this is ready for 5.2. > > Checkpatch complains about 2 "ERROR: do not use assignment in if > condition" in patches 19/37 and 27/37. Though I can see the benefit of > having it that way, I think we should stick to checkpatch here as it > is too tempting to say "hey, there's a typo here, let's replace `=` by > `==`" > > Checkpatch also complains about: > - trailing statements should be on next line: but I do not really mind > those, the blocks it complains are clearer that way > - lines over 80 characters -> these are fine in that context > - Block comments use a trailing */ on a separate line -> please fix > those (patches 7/37 and 8/37) as the rest of the code has the correct > block comments style Heh, patches 7/37 and 8/37 are actually unmodified patches from you, I was already wondering why I did not catch this myself. Anyways I'm preparing a v3 series addressing the above comments now, I will send this out shortly. Regards, Hans > >> >> There are 3 major changes in v2: >> >> 1) Add a new HID group for 27 MHz devices, this has 2 advantages: >> 1a) It allows the logitech-hidpp code to identity 27 Mhz devices and to >> automatically enable some HID++ quirls for these, such as e.g. HID++ >> mouse-wheel handling (fixing some hwheel issues) >> 1b) Since the logitech-hidpp code can now identify these the logitech-dj >> code for 27 MHz devices no longer needs separate 27 MHz descriptors for the >> consumer and HID++ reports. > > I am not entirely convinced we need a new group. But OTOH, being able > to quickly and reliably filter those legacy device is something that > is useful. > >> >> 2) Add support Logitech Bluetooth receivers in HID proxy mode, tested with a >> MX5000 keyboard + receiver. This allows e.g. battery monitoring of the kbd. >> > > I played today with a diNovo mini which has a slightly different > Logitech BT receiver. This part works fine except for the integrated > touchapd which provides reports with an ID 5. I locally hacked > something to have the mouse working, but it's a hack. > The only problem I have with integrating the diNovo mini is that > hid-lg.c does an input_mapping for one key on the keyboard... And I > think we will need it too in hid-logitech-hidpp :/ > > I also added the G700 locally, and a small modification to support > unnumbered mouse reports of size 8 is required, but I can send that as > a followup patch. > > Cheers, > Benjamin > >> 3) Rework HID++ mouse-wheel and extra-buttons support making it more >> generic and cleaner and add HID++ consumer-keys report. Using the HID++ >> consumer keys reports fixes several keys not working on some kbds. >> >> >> Per patch info, unlisted patches are unchanged: >> >> [PATCH 01/37] HID: quirks: do not blacklist Logitech devices >> -New patch split of from a cleanup patch from Benjamin which was touching >> many different drivers at once. This removes a dep on that patch. >> >> [PATCH 12/37] HID: logitech-dj: support sharing struct dj_receiver_dev between USB-interfaces >> -Fixed calling hid_err on a NULL hdev >> >> [PATCH 16/37] HID: logitech-dj: add support for 27 MHz receivers >> -Remove chunk to pick a better name for hidpp devices, see new patch below >> -Add support for mouse on index 2, numeric-keypad on index 4 >> -Use a new group for 27MHz devices >> >> [PATCH 20/37] HID: logitech-dj: pick a better name for non-unifying receivers >> -New patch replacing the dropped chunk from patch 16 as well as replacing >> "HID: logitech-hidpp: create a name based on the type if non available" >> from v1 >> >> [PATCH 21/37] HID: logitech-dj: remove false-positive error ... >> -New misc. fix patch >> >> [PATCH 22/37] HID: logitech-dj: make appending of the HID++ ... >> [PATCH 23/37] HID: logitech-dj: add support for Logitech Bluetooth >> -2 new patches adding support for Logitech BT receivers in HID proxy mode >> >> [PATCH 26/37] HID: logitech-hidpp: ignore very-short or empty >> [PATCH 27/37] HID: logitech-hidpp: do not make failure to get the >> [PATCH 28/37] HID: logitech-hidpp: remove double assignment from >> [PATCH 29/37] HID: logitech-hidpp: remove unused >> -4 new patches (misc. fixes / cleanups) >> >> [PATCH 31/37] HID: logitech-hidpp: handle devices attached to >> -Add support for the new 27 MHz lg devices hid group >> >> [PATCH 32/37] HID: logitech-hidpp: do not hardcode very long >> -This fixes logitech-hidpp rejecting the mx5000 keyboard when paired over BT >> >> [PATCH 34/37] HID: logitech-hidpp: make hidpp10_set_register_bit a >> [PATCH 35/37] HID: logitech-hidpp: add support for HID++ 1.0 wheel >> [PATCH 36/37] HID: logitech-hidpp: add support for HID++ 1.0 extra >> [PATCH 37/37] HID: logitech-hidpp: add support for HID++ 1.0 >> -Reworked HID++ mouse-wheel and extra-buttons support making it more >> generic and add HID++ consumer-keys report support >> >> >> >> For reference here is the cover letter of v1 of the series: >> >> Here is a series Benjamin and I have been working on, before this series we >> treat Logitech wireless mice / keyboards connected through a non-unifying >> receiver as generic HID devices, using the generic HID emulation of the >> receiver. This causes several problems: >> >> -We cannot properly support some special keys / buttons on some keyboards / >> mice since we cannot properly identify / talk to the device behind the recv. >> -We cannot monitor the battery in these devices >> >> This series addresses this by actually enumerating (and talking to) the >> devices behind the receiver, rather then solely relying on the receiver's >> generic HID emulation. >> >> Note this series applies on top of the: >> "[PATCH] HID: force setting drvdata to NULL when removing the driver" >> patch which Benjamin send out 2 days ago. >> >> Regards, >> >> Hans >>