On Tue, Feb 09, 2021 at 08:34:00AM +0100, Anton Lindqvist wrote:
> Hi,
>
> On Mon, Feb 08, 2021 at 02:50:39PM -0800, Anindya Mukherjee wrote:
> > Hi, I have a Logitech M570 which seems to be handled by this new driver.
> > However I don't see any battery level information.
> >
> > dmesg:
> > uhidpp0 at uhidev4 device 1 mouse "M570" serial ef-81-ff-80
> >
> > sysctl kern.version:
> > kern.version=OpenBSD 6.8-current (GENERIC.MP) #313: Fri Feb 5 18:31:44 MST
> > 2021
> > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >
> > hw.sensors.uhidpp0 does not seem to exist.
> >
> > Regards,
> > Anindya
> >
>
> Could you try the following diff and send me the complete dmesg. I've
> also prepared an amd64 kernel with the patch applied:
>
> https://www.basename.se/tmp/bsd.uhidpp.battery
Thanks! I'll try it as soon as I can and report back.
>
> diff --git sys/dev/usb/uhidpp.c sys/dev/usb/uhidpp.c
> index b041d86fecd..27f6137ec06 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 { \
> @@ -84,12 +84,16 @@ int uhidpp_debug = 1;
> #define HIDPP_GET_LONG_REGISTER 0x83
>
> #define HIDPP_REG_ENABLE_REPORTS 0x00
> +#define HIDPP_REG_BATTERY_STATUS 0x07
> #define HIDPP_REG_PAIRING_INFORMATION 0xb5
>
> #define HIDPP_NOTIF_DEVICE_BATTERY_STATUS (1 << 4)
> #define HIDPP_NOTIF_RECEIVER_WIRELESS (1 << 0)
> #define HIDPP_NOTIF_RECEIVER_SOFTWARE_PRESENT (1 << 3)
>
> +/* Number of battery levels supported by HID++ 1.0 devices. */
> +#define HIDPP_BATTERY_NLEVELS 7
> +
> /* HID++ 1.0 error codes. */
> #define HIDPP_ERROR 0x8f
> #define HIDPP_ERROR_SUCCESS 0x00
> @@ -112,7 +116,6 @@ int uhidpp_debug = 1;
> * greater than zero which is reserved for notifications.
> */
> #define HIDPP_SOFTWARE_ID 0x01
> -#define HIDPP_SOFTWARE_ID_MASK 0x0f
> #define HIDPP_SOFTWARE_ID_LEN 4
>
> #define HIDPP20_FEAT_ROOT_IDX 0x00
> @@ -154,8 +157,8 @@ int uhidpp_debug = 1;
>
> /* Feature access report used by the HID++ 2.0 (and greater) protocol. */
> struct fap {
> - uint8_t feature_index;
> - uint8_t funcindex_clientid;
> + uint8_t feature_idx;
> + uint8_t funcidx_swid;
> uint8_t params[HIDPP_REPORT_LONG_PARAMS_MAX];
> };
>
> @@ -185,6 +188,8 @@ struct uhidpp_notification {
> struct uhidpp_device {
> uint8_t d_id;
> uint8_t d_connected;
> + uint8_t d_major;
> + uint8_t d_minor;
> struct {
> struct ksensor b_sens[UHIDPP_NSENSORS];
> uint8_t b_feature_idx;
> @@ -237,8 +242,10 @@ struct uhidpp_notification
> *uhidpp_claim_notification(struct uhidpp_softc *);
> int uhidpp_consume_notification(struct uhidpp_softc *, struct uhidpp_report
> *);
> int uhidpp_is_notification(struct uhidpp_softc *, struct uhidpp_report *);
>
> -int hidpp_get_protocol_version(struct uhidpp_softc *, uint8_t, int *, int
> *);
> +int hidpp_get_protocol_version(struct uhidpp_softc *, uint8_t, uint8_t *,
> + uint8_t *);
>
> +int hidpp10_get_battery_status(struct uhidpp_softc *, uint8_t, uint8_t *);
> int hidpp10_get_name(struct uhidpp_softc *, uint8_t, char *, size_t);
> int hidpp10_get_serial(struct uhidpp_softc *, uint8_t, uint8_t *, size_t);
> int hidpp10_get_type(struct uhidpp_softc *, uint8_t, const char **);
> @@ -520,7 +527,7 @@ void
> uhidpp_device_connect(struct uhidpp_softc *sc, struct uhidpp_device *dev)
> {
> struct ksensor *sens;
> - int error, major, minor;
> + int error;
> uint8_t feature_type;
>
> MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
> @@ -529,30 +536,40 @@ uhidpp_device_connect(struct uhidpp_softc *sc, struct
> uhidpp_device *dev)
> if (dev->d_connected)
> return;
>
> - error = hidpp_get_protocol_version(sc, dev->d_id, &major, &minor);
> + error = hidpp_get_protocol_version(sc, dev->d_id,
> + &dev->d_major, &dev->d_minor);
> if (error) {
> - DPRINTF("%s: protocol version failure: device_id=%d,
> error=%d\n",
> + DPRINTF("%s: protocol version failure: device_id=%d, "
> + "error=%d\n",
> __func__, dev->d_id, error);
> return;
> }
>
> DPRINTF("%s: device_id=%d, version=%d.%d\n",
> - __func__, dev->d_id, major, minor);
> + __func__, dev->d_id, dev->d_major, dev->d_minor);
>
> - error = hidpp20_root_get_feature(sc, dev->d_id,
> - HIDPP20_FEAT_BATTERY_IDX,
> - &dev->d_battery.b_feature_idx, &feature_type);
> - if (error) {
> - DPRINTF("%s: battery feature index failure: device_id=%d, "
> - "error=%d\n", __func__, dev->d_id, error);
> - return;
> - }
> + if (dev->d_major >= 2) {
> + error = hidpp20_root_get_feature(sc, dev->d_id,
> + HIDPP20_FEAT_BATTERY_IDX,
> + &dev->d_battery.b_feature_idx, &feature_type);
> + if (error) {
> + DPRINTF("%s: battery feature index failure: "
> + "device_id=%d, error=%d\n",
> + __func__, dev->d_id, error);
> + return;
> + }
>
> - error = hidpp20_battery_get_capability(sc, dev->d_id,
> - dev->d_battery.b_feature_idx, &dev->d_battery.b_nlevels);
> - if (error) {
> - DPRINTF("%s: battery capability failure: device_id=%d, "
> - "error=%d\n", __func__, dev->d_id, error);
> + error = hidpp20_battery_get_capability(sc, dev->d_id,
> + dev->d_battery.b_feature_idx, &dev->d_battery.b_nlevels);
> + if (error) {
> + DPRINTF("%s: battery capability failure: device_id=%d, "
> + "error=%d\n", __func__, dev->d_id, error);
> + return;
> + }
> +
> + } else if (dev->d_major == 1) {
> + dev->d_battery.b_nlevels = HIDPP_BATTERY_NLEVELS;
> + } else {
> return;
> }
>
> @@ -579,44 +596,58 @@ uhidpp_device_refresh(struct uhidpp_softc *sc, struct
> uhidpp_device *dev)
>
> MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
>
> - error = hidpp20_battery_get_level_status(sc, dev->d_id,
> - dev->d_battery.b_feature_idx,
> - &dev->d_battery.b_level, &dev->d_battery.b_next_level,
> - &dev->d_battery.b_status);
> - if (error) {
> - DPRINTF("%s: battery level status failure: device_id=%d, "
> - "error=%d\n", __func__, dev->d_id, error);
> - return;
> - }
> + if (dev->d_major >= 2) {
> + error = hidpp20_battery_get_level_status(sc, dev->d_id,
> + dev->d_battery.b_feature_idx,
> + &dev->d_battery.b_level, &dev->d_battery.b_next_level,
> + &dev->d_battery.b_status);
> + if (error) {
> + DPRINTF("%s: battery status failure: device_id=%d, "
> + "error=%d\n",
> + __func__, dev->d_id, error);
> + return;
> + }
>
> - dev->d_battery.b_sens[0].value = dev->d_battery.b_level * 1000;
> - dev->d_battery.b_sens[0].flags &= ~SENSOR_FUNKNOWN;
> - if (dev->d_battery.b_nlevels < 10) {
> - /*
> - * According to the HID++ 2.0 specification, less than 10 levels
> - * should be mapped to the following 4 levels:
> - *
> - * [0, 10] critical
> - * [11, 30] low
> - * [31, 80] good
> - * [81, 100] full
> - *
> - * Since sensors are limited to 3 valid statuses, clamp it even
> - * further.
> - */
> - if (dev->d_battery.b_level <= 10)
> - dev->d_battery.b_sens[0].status = SENSOR_S_CRIT;
> - else if (dev->d_battery.b_level <= 30)
> - dev->d_battery.b_sens[0].status = SENSOR_S_WARN;
> - else
> - dev->d_battery.b_sens[0].status = SENSOR_S_OK;
> + dev->d_battery.b_sens[0].value = dev->d_battery.b_level * 1000;
> + dev->d_battery.b_sens[0].flags &= ~SENSOR_FUNKNOWN;
> + if (dev->d_battery.b_nlevels < 10) {
> + /*
> + * According to the HID++ 2.0 specification, less than
> + * 10 levels should be mapped to the following 4 levels:
> + *
> + * [0, 10] critical
> + * [11, 30] low
> + * [31, 80] good
> + * [81, 100] full
> + *
> + * Since sensors are limited to 3 valid statuses, clamp
> + * it even further.
> + */
> + if (dev->d_battery.b_level <= 10)
> + dev->d_battery.b_sens[0].status = SENSOR_S_CRIT;
> + else if (dev->d_battery.b_level <= 30)
> + dev->d_battery.b_sens[0].status = SENSOR_S_WARN;
> + else
> + dev->d_battery.b_sens[0].status = SENSOR_S_OK;
> + } else {
> + /*
> + * XXX the device supports battery mileage. The current
> + * level must be checked against resp.fap.params[3]
> + * given by hidpp20_battery_get_capability().
> + */
> + dev->d_battery.b_sens[0].status = SENSOR_S_UNKNOWN;
> + }
> } else {
> - /*
> - * XXX the device supports battery mileage. The current level
> - * must be checked against resp.fap.params[3] given by
> - * hidpp20_battery_get_capability().
> - */
> - dev->d_battery.b_sens[0].status = SENSOR_S_UNKNOWN;
> + error = hidpp10_get_battery_status(sc, dev->d_id,
> + &dev->d_battery.b_level);
> + if (error) {
> + DPRINTF("%s: battery status failure: device_id=%d, "
> + "error=%d\n",
> + __func__, dev->d_id, error);
> + return;
> + }
> + dev->d_battery.b_sens[0].value = dev->d_battery.b_level * 1000;
> + dev->d_battery.b_sens[0].flags &= ~SENSOR_FUNKNOWN;
> }
> }
>
> @@ -692,9 +723,9 @@ uhidpp_is_notification(struct uhidpp_softc *sc, struct
> uhidpp_report *rep)
>
> /* An error must always be a response. */
> if ((rep->rap.sub_id == HIDPP_ERROR ||
> - rep->fap.feature_index == HIDPP20_ERROR) &&
> - rep->fap.funcindex_clientid == sc->sc_req->fap.feature_index &&
> - rep->fap.params[0] == sc->sc_req->fap.funcindex_clientid)
> + rep->fap.feature_idx == HIDPP20_ERROR) &&
> + rep->fap.funcidx_swid == sc->sc_req->fap.feature_idx &&
> + rep->fap.params[0] == sc->sc_req->fap.funcidx_swid)
> return 0;
>
> return 1;
> @@ -702,7 +733,7 @@ uhidpp_is_notification(struct uhidpp_softc *sc, struct
> uhidpp_report *rep)
>
> int
> hidpp_get_protocol_version(struct uhidpp_softc *sc, uint8_t device_id,
> - int *major, int *minor)
> + uint8_t *major, uint8_t *minor)
> {
> struct uhidpp_report resp;
> uint8_t params[3] = { 0, 0, HIDPP_FEAT_ROOT_PING_DATA };
> @@ -729,6 +760,37 @@ hidpp_get_protocol_version(struct uhidpp_softc *sc,
> uint8_t device_id,
> return 0;
> }
>
> +int
> +hidpp10_get_battery_status(struct uhidpp_softc *sc, uint8_t device_id,
> + uint8_t *level)
> +{
> + struct uhidpp_report resp;
> + int error;
> +
> + error = hidpp_send_rap_report(sc,
> + HIDPP_REPORT_ID_SHORT,
> + device_id,
> + HIDPP_GET_REGISTER,
> + HIDPP_REG_BATTERY_STATUS,
> + NULL, 0, &resp);
> + DPRINTF("XXX %s: error=%d, device_id=%d\n",
> + __func__, error, device_id);
> + if (error)
> + return error;
> + if (resp.rap.params[0] < 1 ||
> + resp.rap.params[0] > HIDPP_BATTERY_NLEVELS)
> + return -ERANGE;
> +
> + DPRINTF("XXX %s: p0=%x, p1=%x, p2=%x, device_id=%d\n",
> + __func__,
> + resp.rap.params[0],
> + resp.rap.params[1],
> + resp.rap.params[2],
> + device_id);
> + *level = (resp.rap.params[0] * 100) / HIDPP_BATTERY_NLEVELS;
> + return 0;
> +}
> +
> int
> hidpp10_get_name(struct uhidpp_softc *sc, uint8_t device_id,
> char *buf, size_t bufsiz)
> @@ -848,7 +910,7 @@ hidpp10_enable_notifications(struct uhidpp_softc *sc,
> uint8_t device_id)
>
> int
> hidpp20_root_get_feature(struct uhidpp_softc *sc, uint8_t device_id,
> - uint16_t feature, uint8_t *feature_index, uint8_t *feature_type)
> + uint16_t feature, uint8_t *feature_idx, uint8_t *feature_type)
> {
> struct uhidpp_report resp;
> uint8_t params[2] = { feature >> 8, feature & 0xff };
> @@ -866,14 +928,14 @@ hidpp20_root_get_feature(struct uhidpp_softc *sc,
> uint8_t device_id,
> if (resp.fap.params[0] == 0)
> return -ENOENT;
>
> - *feature_index = resp.fap.params[0];
> + *feature_idx = resp.fap.params[0];
> *feature_type = resp.fap.params[1];
> return 0;
> }
>
> int
> hidpp20_battery_get_level_status(struct uhidpp_softc *sc, uint8_t device_id,
> - uint8_t feature_index, uint8_t *level, uint8_t *next_level, uint8_t
> *status)
> + uint8_t feature_idx, uint8_t *level, uint8_t *next_level, uint8_t
> *status)
> {
> struct uhidpp_report resp;
> int error;
> @@ -881,7 +943,7 @@ hidpp20_battery_get_level_status(struct uhidpp_softc *sc,
> uint8_t device_id,
> error = hidpp_send_fap_report(sc,
> HIDPP_REPORT_ID_LONG,
> device_id,
> - feature_index,
> + feature_idx,
> HIDPP20_FEAT_BATTERY_LEVEL_FUNC,
> NULL, 0, &resp);
> if (error)
> @@ -895,7 +957,7 @@ hidpp20_battery_get_level_status(struct uhidpp_softc *sc,
> uint8_t device_id,
>
> int
> hidpp20_battery_get_capability(struct uhidpp_softc *sc, uint8_t device_id,
> - uint8_t feature_index, uint8_t *nlevels)
> + uint8_t feature_idx, uint8_t *nlevels)
> {
> struct uhidpp_report resp;
> int error;
> @@ -903,7 +965,7 @@ hidpp20_battery_get_capability(struct uhidpp_softc *sc,
> uint8_t device_id,
> error = hidpp_send_fap_report(sc,
> HIDPP_REPORT_ID_LONG,
> device_id,
> - feature_index,
> + feature_idx,
> HIDPP20_FEAT_BATTERY_CAPABILITY_FUNC,
> NULL, 0, &resp);
> if (error)
> @@ -929,7 +991,7 @@ hidpp_send_validate(uint8_t report_id, int nparams)
>
> int
> hidpp_send_fap_report(struct uhidpp_softc *sc, uint8_t report_id,
> - uint8_t device_id, uint8_t feature_index, uint8_t funcindex_clientid,
> + uint8_t device_id, uint8_t feature_idx, uint8_t funcidx_swid,
> uint8_t *params, int nparams, struct uhidpp_report *resp)
> {
> struct uhidpp_report req;
> @@ -941,9 +1003,9 @@ hidpp_send_fap_report(struct uhidpp_softc *sc, uint8_t
> report_id,
>
> memset(&req, 0, sizeof(req));
> req.device_id = device_id;
> - req.fap.feature_index = feature_index;
> - req.fap.funcindex_clientid =
> - (funcindex_clientid << HIDPP_SOFTWARE_ID_LEN) | HIDPP_SOFTWARE_ID;
> + req.fap.feature_idx = feature_idx;
> + req.fap.funcidx_swid =
> + (funcidx_swid << HIDPP_SOFTWARE_ID_LEN) | HIDPP_SOFTWARE_ID;
> memcpy(req.fap.params, params, nparams);
> return hidpp_send_report(sc, report_id, &req, resp);
> }
> @@ -1024,7 +1086,7 @@ hidpp_send_report(struct uhidpp_softc *sc, uint8_t
> report_id,
> resp->rap.sub_id == HIDPP_ERROR)
> error = resp->rap.params[1];
> else if (sc->sc_resp_state == HIDPP_REPORT_ID_LONG &&
> - resp->fap.feature_index == HIDPP20_ERROR)
> + resp->fap.feature_idx == HIDPP20_ERROR)
> error = resp->fap.params[1];
>
> out:
Regards,
Anindya
