Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices

["Peter F. Patel-Schneider" <pfpschneider@gmail.com>]

This patch will cause more use of a part of the driver that constructs
messages that do not conform to the HID++ 2.0 specification.  This
makes now a good time to fix the parts of the driver that construct
non-conforming messages.  More information follows.


The Logitech HID++2.0 Draft Specification at
https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf
specifies (on pages 2 and 3) that the low-order nibble of the function
(command) byte is non-zero.

The HID++ driver at
https://github.com/torvalds/linux/blob/master/drivers/hid/hid-logitech-hidpp.c
has 1 in that nibble for some commands, 

#define CMD_ROOT_GET_FEATURE				0x01
#define CMD_ROOT_GET_PROTOCOL_VERSION			0x11

#define CMD_GET_DEVICE_NAME_TYPE_GET_COUNT		0x01
#define CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME	0x11
#define CMD_GET_DEVICE_NAME_TYPE_GET_TYPE		0x21

But other commands have zero in that nibble, namely

#define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS	0x00
#define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY		0x10

#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00

#define CMD_UNIFIED_BATTERY_GET_CAPABILITIES			0x00
#define CMD_UNIFIED_BATTERY_GET_STATUS				0x10

#define CMD_HI_RESOLUTION_SCROLLING_SET_HIGHRES_SCROLLING_MODE	0x10

#define CMD_HIRES_WHEEL_GET_WHEEL_CAPABILITY	0x00
#define CMD_HIRES_WHEEL_SET_WHEEL_MODE		0x20

#define CMD_SOLAR_SET_LIGHT_MEASURE			0x00

#define CMD_TOUCHPAD_FW_ITEMS_SET			0x10

This results in non-conforming messages being sent to devices.  As
devices are obligated to return this nibble intact they produce non-
conforming responses as well.  (Their other option would be to reject
the messages.) This confuses other software that correctly uses this
nibble to distinguish between device response messages and device event
messages.

In particular, the response to the unified battery command to get the
capabilities comes back with a 0x00 function byte which is
indistinguishable from a spontaneous notification message from the
device for a battery status event.  Other software trying to
communicate with the device (e.g., Solaar) sees a unified battery
status notification and will generally end up with incorrect
information about the device.  I suspect that this is actually
happening and is the cause of the Solaar bug report
https://github.com/pwr-Solaar/Solaar/issues/1718

There is also the possibility that the driver confuses a notification
from the device as the response to a command that it sent.  When this
happens it would be likely that the actual response would be treated as
a notification.


The fix is to modify all the CMD definitions in the code to have 1 in
their low-order nibble.


peter

PS: There is another HID++ 2.0 feature that reports battery
information, 0x1F20 ADC Measurement, but that is not in the driver
code.



On Thu, 2022-08-25 at 17:59 +0200, Nestor Lopez Casado wrote:
> Adding Peter, as he maintains Solaar, one popular application that
> supports Logitech bluetooth devices.
> @Peter F. Patel-Schneider
> -nestor
> 
> On Thu, Aug 25, 2022 at 2:29 PM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > Probe for HID++ support over Bluetooth for all the Logitech
> > Bluetooth
> > devices. As Logitech doesn't have a list of Bluetooth devices that
> > support HID++ over Bluetooth, probe every device. The HID++ driver
> > will fall back to plain HID if the device does not support HID++.
> > 
> > Note that this change might cause upower to export 2 batteries for
> > certain Bluetooth LE devices which export their battery information
> > through the Bluetooth BATT profile. This particular bug is tracked
> > at:
> > https://gitlab.freedesktop.org/upower/upower/-/issues/166
> > 
> > Tested with a Logitech Signature M650 mouse, over Bluetooth
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> > 
> > Note that I could not test whether the Harmony PS3 (handled in hid-
> > sony.c)
> > or DiNovo Edge keyboard (handled in hid-input.c) devices would
> > correctly fallback
> > to those drivers in that case.
> > 
> > Ways to test this would be appreciated (or merge this, and wait for
> > feedback...)
> > 
> >  drivers/hid/hid-logitech-hidpp.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index 81de88ab2ecc..86e7a38d8a9a 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -4347,6 +4347,9 @@ static const struct hid_device_id
> > hidpp_devices[] = {
> >         { /* MX Master 3 mouse over Bluetooth */
> >           HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023),
> >           .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > +
> > +       { /* And try to enable HID++ for all the Logitech Bluetooth
> > devices */
> > +         HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_ANY,
> > USB_VENDOR_ID_LOGITECH, HID_ANY_ID) },
> >         {}
> >  };
> > 
> > --
> > 2.37.2
> >