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
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:
