Re: uhidpp(4): add support for Bolt receivers and Unified Battery feature

2022-11-26 Thread David Higgs
On Sat, Nov 26, 2022 at 1:47 AM Anton Lindqvist  wrote:

> Hi,
> This diff adds supports for the following to uhidpp:
>
> 1. Bolt receivers, they use a different set of registers for querying
>paired devices.
>
> 2. The Unified Battery feature, this is a more competent feature
>function used to report battery status compared to the Battery
>feature which is already supported by uhidpp.
>
> Makes battery sensors appear for my Logitech Lift mouse and Paul de
> Weerd's MX Anywhere 3 mouse.
>
> If you have a Logitech HID++ device currently lacking sensors under
> sysctl hw.sensors, please give this diff a try. Send me your dmesg and
> output of sysctl hw.sensors after moving the mouse to ensure a connect
> interrupt has fired off. Note that the diff must be applied on top of
> revision 1.37 of sys/dev/usb/uhidpp.c.
>
> Comments? OK?
>


> 
> @@ -193,17 +202,20 @@ struct uhidpp_device {
> uint8_t d_features;
>  #define UHIDPP_DEVICE_FEATURE_ROOT 0x01
>  #define UHIDPP_DEVICE_FEATURE_BATTERY  0x02
> +#define UHIDPP_DEVICE_FEATURE_UNIFIIED_BATTERY 0x04
>
> 
>

Do you actually mean "unifiied" with an extra "i" here?

--david


Re: uhidpp(4): add support for Bolt receivers and Unified Battery feature

2022-11-26 Thread Klemens Nanni
On Sat, Nov 26, 2022 at 02:39:48PM +, Klemens Nanni wrote:
> On Sat, Nov 26, 2022 at 07:46:08AM +0100, Anton Lindqvist wrote:
> > Hi,
> > This diff adds supports for the following to uhidpp:
> > 
> > 1. Bolt receivers, they use a different set of registers for querying
> >paired devices.
> > 
> > 2. The Unified Battery feature, this is a more competent feature
> >function used to report battery status compared to the Battery
> >feature which is already supported by uhidpp.
> > 
> > Makes battery sensors appear for my Logitech Lift mouse and Paul de
> > Weerd's MX Anywhere 3 mouse.
> > 
> > If you have a Logitech HID++ device currently lacking sensors under
> > sysctl hw.sensors, please give this diff a try. Send me your dmesg and
> > output of sysctl hw.sensors after moving the mouse to ensure a connect
> > interrupt has fired off. Note that the diff must be applied on top of
> > revision 1.37 of sys/dev/usb/uhidpp.c.
> 
> No idea what Bolt or Logitech HID++ is, but my MX Ergo doesn't report
> any new sensors with this diff.  Should it do so?

Nevermind, it just took a while for the device to send battery reports:

hw.sensors.uhidpp0.indicator0=Off (charger)
hw.sensors.uhidpp0.raw0=4 (number of battery levels)
hw.sensors.uhidpp0.percent0=50.00% (battery level), OK

So all working as intended, I just didn't wait long enough after attach
to mail out my findings.

These lines/values are logged every thirty seconds, I guess those are
the actual battery reports?

hidpp_send_report: 11 01 08 01 [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00]
uhidpp_intr: 11 01 08 01 [32 14 00 00 00 00 00 00 00 00 00 00 00 00 00 00]

Thanks!



Re: uhidpp(4): add support for Bolt receivers and Unified Battery feature

2022-11-26 Thread Klemens Nanni
On Sat, Nov 26, 2022 at 07:46:08AM +0100, Anton Lindqvist wrote:
> Hi,
> This diff adds supports for the following to uhidpp:
> 
> 1. Bolt receivers, they use a different set of registers for querying
>paired devices.
> 
> 2. The Unified Battery feature, this is a more competent feature
>function used to report battery status compared to the Battery
>feature which is already supported by uhidpp.
> 
> Makes battery sensors appear for my Logitech Lift mouse and Paul de
> Weerd's MX Anywhere 3 mouse.
> 
> If you have a Logitech HID++ device currently lacking sensors under
> sysctl hw.sensors, please give this diff a try. Send me your dmesg and
> output of sysctl hw.sensors after moving the mouse to ensure a connect
> interrupt has fired off. Note that the diff must be applied on top of
> revision 1.37 of sys/dev/usb/uhidpp.c.

No idea what Bolt or Logitech HID++ is, but my MX Ergo doesn't report
any new sensors with this diff.  Should it do so?

The trackball keeps working, though.

Here's the dmesg from when I attach the USB receiver with your diff:

uhidev0 at uhub3 port 2 configuration 1 interface 0 "Logitech USB Receiver" rev 
2.00/12.03 addr 3
uhidev0: iclass 3/1
ukbd0 at uhidev0: 8 variable keys, 6 key codes
wskbd1 at ukbd0 mux 1
wskbd1: connecting to wsdisplay0
uhidev1 at uhub3 port 2 configuration 1 interface 1 "Logitech USB Receiver" rev 
2.00/12.03 addr 3
uhidev1: iclass 3/1, 8 report ids
ums0 at uhidev1 reportid 2: 16 buttons, Z and W dir
wsmouse2 at ums0 mux 0
ucc0 at uhidev1 reportid 3: 767 usages, 20 keys, array
wskbd2 at ucc0 mux 1
wskbd2: connecting to wsdisplay0
uhid0 at uhidev1 reportid 4: input=1, output=0, feature=0
uhid1 at uhidev1 reportid 8: input=1, output=0, feature=0
uhidev2 at uhub3 port 2 configuration 1 interface 2 "Logitech USB Receiver" rev 
2.00/12.03 addr 3
uhidev2: iclass 3/0, 33 report ids
uhidpp0 at uhidev2hidpp_send_report: 10 ff 83 b5 [20 00 00]
uhidpp_intr: 11 ff 83 b5 [20 0b 08 40 6f 04 02 08 07 00 00 00 00 00 00 00]
hidpp_send_report: 10 ff 83 b5 [40 00 00]
uhidpp_intr: 11 ff 83 b5 [40 07 4d 58 20 45 72 67 6f 00 00 00 00 00 00 00]
 device 1 trackball "MX Ergo"hidpp_send_report: 10 ff 83 b5 [21 00 00]
uhidpp_intr: 10 ff 8f 83 [b5 03 00]
hidpp_send_report: 10 ff 83 b5 [22 00 00]
uhidpp_intr: 10 ff 8f 83 [b5 03 00]
hidpp_send_report: 10 ff 83 b5 [23 00 00]
uhidpp_intr: 10 ff 8f 83 [b5 03 00]
hidpp_send_report: 10 ff 83 b5 [24 00 00]
uhidpp_intr: 10 ff 8f 83 [b5 03 00]
hidpp_send_report: 10 ff 83 b5 [25 00 00]
uhidpp_intr: 10 ff 8f 83 [b5 03 00]
hidpp_send_report: 10 ff 80 00 [10 09 00]
uhidpp_intr: 10 ff 80 00 [00 00 00]
uhidpp_intr: 10 ff 80 00 [00 00 00]
uhid2 at uhidev2 reportid 32: input=14, output=14, feature=0
uhid3 at uhidev2 reportid 33: input=31, output=31, feature=0


> 
> Comments? OK?
> 
> diff --git sys/dev/usb/uhidpp.c sys/dev/usb/uhidpp.c
> index 83e58d56fc9..5b9ef198948 100644
> --- sys/dev/usb/uhidpp.c
> +++ sys/dev/usb/uhidpp.c
> @@ -29,7 +29,7 @@
>  #include 
>  #include 
>  
> -/* #define UHIDPP_DEBUG */
> +#define UHIDPP_DEBUG
>  #ifdef UHIDPP_DEBUG
>  
>  #define DPRINTF(x...) do {   \
> @@ -125,6 +125,15 @@ int uhidpp_debug = 1;
>  #define HIDPP20_FEAT_BATTERY_CAPABILITY_FUNC 0x0001
>  #define  HIDPP20_CAPABILITY_RECHARGEABLE 0x0004
>  
> +#define HIDPP20_FEAT_UNIFIED_BATTERY_ID  0x1004
> +#define HIDPP20_FEAT_UNIFIED_BATTERY_CAPABILITIES_FUNC   0x
> +#define  HIDPP20_CAPABILITES_RECHARGEABLE0x0002
> +#define HIDPP20_FEAT_UNIFIED_BATTERY_STATUS_FUNC 0x0001
> +#define  HIDPP20_BATTERY_STATUS_CRITICAL 0x0001
> +#define  HIDPP20_BATTERY_STATUS_LOW  0x0002
> +#define  HIDPP20_BATTERY_STATUS_GOOD 0x0004
> +#define  HIDPP20_BATTERY_STATUS_FULL 0x0008
> +
>  /* HID++ 2.0 error codes. */
>  #define HIDPP20_ERROR0xff
>  #define HIDPP20_ERROR_NO_ERROR   0x00
> @@ -193,17 +202,20 @@ struct uhidpp_device {
>   uint8_t d_features;
>  #define UHIDPP_DEVICE_FEATURE_ROOT   0x01
>  #define UHIDPP_DEVICE_FEATURE_BATTERY0x02
> +#define UHIDPP_DEVICE_FEATURE_UNIFIIED_BATTERY   0x04
>  
>   struct {
>   struct ksensor sens[UHIDPP_NSENSORS];
>   uint8_t feature_idx;
>   uint8_t nlevels;
> + uint8_t unified_level_mask;
>   uint8_t rechargeable;
>   } d_battery;
>  };
>  
>  /*
>   * Locking:
> + *   [I] immutable
>   *   [m] sc_mtx
>   */
>  struct uhidpp_softc {
> @@ -226,6 +238,11 @@ struct uhidpp_softc {
>   struct uhidpp_report *sc_req;   /* [m] synchronous request buffer */
>   struct uhidpp_report *sc_resp;  /* [m] synchronous response buffer */
>   u_int sc_resp_state;/* [m] synchronous response state */
> +
> + enum {
> + UHIDPP_RECEIVER_UNIFYING,
> + UHIDPP_RECEIVER_BOLT,
> + } sc_receiver;

uhidpp(4): add support for Bolt receivers and Unified Battery feature

2022-11-25 Thread Anton Lindqvist
Hi,
This diff adds supports for the following to uhidpp:

1. Bolt receivers, they use a different set of registers for querying
   paired devices.

2. The Unified Battery feature, this is a more competent feature
   function used to report battery status compared to the Battery
   feature which is already supported by uhidpp.

Makes battery sensors appear for my Logitech Lift mouse and Paul de
Weerd's MX Anywhere 3 mouse.

If you have a Logitech HID++ device currently lacking sensors under
sysctl hw.sensors, please give this diff a try. Send me your dmesg and
output of sysctl hw.sensors after moving the mouse to ensure a connect
interrupt has fired off. Note that the diff must be applied on top of
revision 1.37 of sys/dev/usb/uhidpp.c.

Comments? OK?

diff --git sys/dev/usb/uhidpp.c sys/dev/usb/uhidpp.c
index 83e58d56fc9..5b9ef198948 100644
--- sys/dev/usb/uhidpp.c
+++ sys/dev/usb/uhidpp.c
@@ -29,7 +29,7 @@
 #include 
 #include 
 
-/* #define UHIDPP_DEBUG */
+#define UHIDPP_DEBUG
 #ifdef UHIDPP_DEBUG
 
 #define DPRINTF(x...) do { \
@@ -125,6 +125,15 @@ int uhidpp_debug = 1;
 #define HIDPP20_FEAT_BATTERY_CAPABILITY_FUNC   0x0001
 #define  HIDPP20_CAPABILITY_RECHARGEABLE   0x0004
 
+#define HIDPP20_FEAT_UNIFIED_BATTERY_ID0x1004
+#define HIDPP20_FEAT_UNIFIED_BATTERY_CAPABILITIES_FUNC 0x
+#define  HIDPP20_CAPABILITES_RECHARGEABLE  0x0002
+#define HIDPP20_FEAT_UNIFIED_BATTERY_STATUS_FUNC   0x0001
+#define  HIDPP20_BATTERY_STATUS_CRITICAL   0x0001
+#define  HIDPP20_BATTERY_STATUS_LOW0x0002
+#define  HIDPP20_BATTERY_STATUS_GOOD   0x0004
+#define  HIDPP20_BATTERY_STATUS_FULL   0x0008
+
 /* HID++ 2.0 error codes. */
 #define HIDPP20_ERROR  0xff
 #define HIDPP20_ERROR_NO_ERROR 0x00
@@ -193,17 +202,20 @@ struct uhidpp_device {
uint8_t d_features;
 #define UHIDPP_DEVICE_FEATURE_ROOT 0x01
 #define UHIDPP_DEVICE_FEATURE_BATTERY  0x02
+#define UHIDPP_DEVICE_FEATURE_UNIFIIED_BATTERY 0x04
 
struct {
struct ksensor sens[UHIDPP_NSENSORS];
uint8_t feature_idx;
uint8_t nlevels;
+   uint8_t unified_level_mask;
uint8_t rechargeable;
} d_battery;
 };
 
 /*
  * Locking:
+ * [I] immutable
  * [m] sc_mtx
  */
 struct uhidpp_softc {
@@ -226,6 +238,11 @@ struct uhidpp_softc {
struct uhidpp_report *sc_req;   /* [m] synchronous request buffer */
struct uhidpp_report *sc_resp;  /* [m] synchronous response buffer */
u_int sc_resp_state;/* [m] synchronous response state */
+
+   enum {
+   UHIDPP_RECEIVER_UNIFYING,
+   UHIDPP_RECEIVER_BOLT,
+   } sc_receiver;  /* [I] */
 };
 
 int uhidpp_match(struct device *, void *, void *);
@@ -264,6 +281,11 @@ int hidpp20_battery_get_level_status(struct uhidpp_softc *,
 int hidpp20_battery_get_capability(struct uhidpp_softc *,
 struct uhidpp_device *);
 int hidpp20_battery_status_is_charging(uint8_t);
+int hidpp20_unified_battery_get_capabilities(struct uhidpp_softc *,
+struct uhidpp_device *);
+int hidpp20_unified_battery_get_status(struct uhidpp_softc *,
+struct uhidpp_device *);
+int hidpp20_unified_battery_status_is_charging(uint8_t);
 
 int hidpp_send_validate(uint8_t, int);
 int hidpp_send_rap_report(struct uhidpp_softc *, uint8_t, uint8_t, uint8_t,
@@ -273,6 +295,18 @@ int hidpp_send_fap_report(struct uhidpp_softc *, uint8_t, 
uint8_t, uint8_t,
 int hidpp_send_report(struct uhidpp_softc *, uint8_t, struct uhidpp_report *,
 struct uhidpp_report *);
 
+static uint8_t
+nlevels(uint8_t mask)
+{
+   uint8_t nbits = 0;
+
+   for (; mask > 0; mask >>= 1) {
+   if (mask & 1)
+   nbits++;
+   }
+   return nbits;
+}
+
 struct cfdriver uhidpp_cd = {
NULL, "uhidpp", DV_DULL
 };
@@ -369,6 +403,11 @@ uhidpp_attach(struct device *parent, struct device *self, 
void *aux)
return;
}
 
+   if (uaa->product == 0xc548)
+   sc->sc_receiver = UHIDPP_RECEIVER_BOLT;
+   else
+   sc->sc_receiver = UHIDPP_RECEIVER_UNIFYING;
+
/* Probe paired devices. */
for (i = 0; i < UHIDPP_NDEVICES; i++) {
char name[16];
@@ -573,7 +612,10 @@ uhidpp_device_connect(struct uhidpp_softc *sc, struct 
uhidpp_device *dev)
return;
}
 
-   error = hidpp20_battery_get_capability(sc, dev);
+   if (dev->d_features & UHIDPP_DEVICE_FEATURE_BATTERY)
+   error = hidpp20_battery_get_capability(sc, dev);
+   else if (dev->d_features & UHIDPP_DEVICE_FEATURE_UNIFIIED_BATTERY)
+   error = hidpp20_unified_battery_get_capabilities(sc, dev);
if (error) {
DPRINTF("%s: battery capability failure: device_id=%d, "