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 <dev/usb/usbdevs.h>
> #include <dev/usb/uhidev.h>
>
> -/* #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 0x0000
> +#define HIDPP20_CAPABILITES_RECHARGEABLE 0x0002
> +#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_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, "
> "error=%d\n", __func__, dev->d_id, error);
> @@ -635,7 +677,12 @@ uhidpp_device_refresh(struct uhidpp_softc *sc, struct
> uhidpp_device *dev)
> if (dev->d_major <= 1)
> return;
>
> - error = hidpp20_battery_get_level_status(sc, dev);
> + if (dev->d_features & UHIDPP_DEVICE_FEATURE_BATTERY)
> + error = hidpp20_battery_get_level_status(sc, dev);
> + else if (dev->d_features & UHIDPP_DEVICE_FEATURE_UNIFIIED_BATTERY)
> + error = hidpp20_unified_battery_get_status(sc, dev);
> + else
> + error = -ENOTSUP;
> if (error) {
> DPRINTF("%s: battery status failure: device_id=%d, error=%d\n",
> __func__, dev->d_id, error);
> @@ -682,6 +729,9 @@ uhidpp_device_features(struct uhidpp_softc *sc, struct
> uhidpp_device *dev)
> if (id == HIDPP20_FEAT_BATTERY_ID) {
> dev->d_features |= UHIDPP_DEVICE_FEATURE_BATTERY;
> dev->d_battery.feature_idx = i;
> + } else if (id == HIDPP20_FEAT_UNIFIED_BATTERY_ID) {
> + dev->d_features |=
> UHIDPP_DEVICE_FEATURE_UNIFIIED_BATTERY;
> + dev->d_battery.feature_idx = i;
> }
>
> DPRINTF("%s: idx=%d, id=%x, type=%x device_id=%d\n",
> @@ -815,24 +865,40 @@ hidpp10_get_name(struct uhidpp_softc *sc, uint8_t
> device_id,
> {
> struct uhidpp_report resp;
> int error;
> - uint8_t params[1] = { 0x40 + (device_id - 1) };
> + const uint8_t *name;
> uint8_t len;
>
> - error = hidpp_send_rap_report(sc,
> - HIDPP_REPORT_ID_SHORT,
> - HIDPP_DEVICE_ID_RECEIVER,
> - HIDPP_GET_LONG_REGISTER,
> - HIDPP_REG_PAIRING_INFORMATION,
> - params, sizeof(params), &resp);
> - if (error)
> - return error;
> + if (sc->sc_receiver == UHIDPP_RECEIVER_BOLT) {
> + uint8_t params[2] = { 0x60 + device_id, 0x01 };
> +
> + error = hidpp_send_rap_report(sc,
> + HIDPP_REPORT_ID_SHORT,
> + HIDPP_DEVICE_ID_RECEIVER,
> + HIDPP_GET_LONG_REGISTER,
> + HIDPP_REG_PAIRING_INFORMATION,
> + params, sizeof(params), &resp);
> + if (error)
> + return error;
> + len = resp.rap.params[2];
> + name = &resp.rap.params[3];
> + } else {
> + uint8_t params[1] = { 0x40 + (device_id - 1) };
> +
> + error = hidpp_send_rap_report(sc,
> + HIDPP_REPORT_ID_SHORT,
> + HIDPP_DEVICE_ID_RECEIVER,
> + HIDPP_GET_LONG_REGISTER,
> + HIDPP_REG_PAIRING_INFORMATION,
> + params, sizeof(params), &resp);
> + if (error)
> + return error;
> + len = resp.rap.params[1];
> + name = &resp.rap.params[2];
> + }
>
> - len = resp.rap.params[1];
> - if (len + 2 > sizeof(resp.rap.params))
> - return -ENAMETOOLONG;
> if (len > bufsiz - 1)
> len = bufsiz - 1;
> - memcpy(buf, &resp.rap.params[2], len);
> + memcpy(buf, name, len);
> buf[len] = '\0';
> return 0;
> }
> @@ -842,18 +908,35 @@ hidpp10_get_type(struct uhidpp_softc *sc, uint8_t
> device_id, const char **buf)
> {
> struct uhidpp_report resp;
> int error;
> - uint8_t params[1] = { 0x20 + (device_id - 1) };
> + uint8_t type;
>
> - error = hidpp_send_rap_report(sc,
> - HIDPP_REPORT_ID_SHORT,
> - HIDPP_DEVICE_ID_RECEIVER,
> - HIDPP_GET_LONG_REGISTER,
> - HIDPP_REG_PAIRING_INFORMATION,
> - params, sizeof(params), &resp);
> - if (error)
> - return error;
> + if (sc->sc_receiver == UHIDPP_RECEIVER_BOLT) {
> + uint8_t params[1] = { 0x50 + device_id };
>
> - switch (resp.rap.params[7]) {
> + error = hidpp_send_rap_report(sc,
> + HIDPP_REPORT_ID_SHORT,
> + HIDPP_DEVICE_ID_RECEIVER,
> + HIDPP_GET_LONG_REGISTER,
> + HIDPP_REG_PAIRING_INFORMATION,
> + params, sizeof(params), &resp);
> + if (error)
> + return error;
> + type = resp.rap.params[1] & 0xf;
> + } else {
> + uint8_t params[1] = { 0x20 + (device_id - 1) };
> +
> + error = hidpp_send_rap_report(sc,
> + HIDPP_REPORT_ID_SHORT,
> + HIDPP_DEVICE_ID_RECEIVER,
> + HIDPP_GET_LONG_REGISTER,
> + HIDPP_REG_PAIRING_INFORMATION,
> + params, sizeof(params), &resp);
> + if (error)
> + return error;
> + type = resp.rap.params[7];
> + }
> +
> + switch (type) {
> case 0x00:
> *buf = "unknown";
> return 0;
> @@ -1059,6 +1142,81 @@ hidpp20_battery_get_capability(struct uhidpp_softc *sc,
> return 0;
> }
>
> +int
> +hidpp20_unified_battery_get_capabilities(struct uhidpp_softc *sc,
> + struct uhidpp_device *dev)
> +{
> + struct uhidpp_report resp;
> + int error;
> +
> + error = hidpp_send_fap_report(sc,
> + HIDPP_REPORT_ID_LONG,
> + dev->d_id,
> + dev->d_battery.feature_idx,
> + HIDPP20_FEAT_UNIFIED_BATTERY_CAPABILITIES_FUNC,
> + NULL, 0, &resp);
> + if (error)
> + return error;
> + dev->d_battery.nlevels = nlevels(resp.fap.params[0]);
> + dev->d_battery.unified_level_mask = resp.fap.params[0];
> + dev->d_battery.rechargeable = resp.fap.params[1] &
> + HIDPP20_CAPABILITES_RECHARGEABLE;
> + return 0;
> +}
> +
> +int
> +hidpp20_unified_battery_get_status(struct uhidpp_softc *sc,
> + struct uhidpp_device *dev)
> +{
> + struct uhidpp_report resp;
> + int charging, error;
> + uint8_t level, percentage, status;
> +
> + error = hidpp_send_fap_report(sc,
> + HIDPP_REPORT_ID_LONG,
> + dev->d_id,
> + dev->d_battery.feature_idx,
> + HIDPP20_FEAT_UNIFIED_BATTERY_STATUS_FUNC,
> + NULL, 0, &resp);
> + if (error)
> + return error;
> + percentage = resp.fap.params[0];
> + level = resp.fap.params[1] & dev->d_battery.unified_level_mask;
> + status = resp.fap.params[2];
> + /* external_power_status = resp.fap.params[3]; */
> +
> + charging = hidpp20_unified_battery_status_is_charging(status);
> + dev->d_battery.sens[0].value = percentage * 1000;
> + dev->d_battery.sens[0].flags &= ~SENSOR_FUNKNOWN;
> + dev->d_battery.sens[0].status = SENSOR_S_UNKNOWN;
> + /* Do not trust the level while charging. */
> + if (!charging) {
> + if (level & HIDPP20_BATTERY_STATUS_CRITICAL)
> + dev->d_battery.sens[0].status = SENSOR_S_CRIT;
> + else if (level & HIDPP20_BATTERY_STATUS_LOW)
> + dev->d_battery.sens[0].status = SENSOR_S_WARN;
> + else if (level & HIDPP20_BATTERY_STATUS_GOOD)
> + dev->d_battery.sens[0].status = SENSOR_S_OK;
> + else if (level & HIDPP20_BATTERY_STATUS_FULL)
> + dev->d_battery.sens[0].status = SENSOR_S_OK;
> + }
> + if (dev->d_battery.rechargeable)
> + dev->d_battery.sens[2].value = charging;
> + return 0;
> +}
> +
> +int
> +hidpp20_unified_battery_status_is_charging(uint8_t status)
> +{
> + switch (status) {
> + case 1: /* charging */
> + case 2: /* charging slow */
> + return 1;
> + default:
> + return 0;
> + }
> +}
> +
> int
> hidpp20_battery_status_is_charging(uint8_t status)
> {
>
