With much help from mpi@, I have made a first big step towards improving
upd(4). I’m not sure when tree lock ends, but I’m still happy to accept
feedback if right now isn’t the time to commit. There’s plenty more to do, but
I’d like to get this ironed out before moving on.
New behavior with this diff:
- Leverage new USB async reporting (must have up-to-date tree)
- Sensor dependencies ensure reports are gathered in useful order
- Track pending reports to minimize USB queries
- Relevant sensors immediately invalidated when battery is removed (instead of
waiting for the next refresh)
Things that may need sanity checks:
- Simplified upd_attach; the old code seemed to have redundant / confusing logic
- Integer promotion/truncation with batpres and hdata/adjust variables
Suggestions, bug reports, and feedback welcome.
--david
Index: upd.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/upd.c,v
retrieving revision 1.13
diff -u -p -r1.13 upd.c
--- upd.c 11 Jan 2015 03:08:38 -0000 1.13
+++ upd.c 20 Feb 2015 02:28:04 -0000
@@ -39,44 +39,15 @@
#define DPRINTF(x)
#endif
-struct upd_usage_entry {
- uint8_t usage_pg;
- uint8_t usage_id;
- enum sensor_type senstype;
- char *usage_name; /* sensor string */
-};
-
-static struct upd_usage_entry upd_usage_table[] = {
- { HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
- SENSOR_PERCENT, "RelativeStateOfCharge" },
- { HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
- SENSOR_PERCENT, "AbsoluteStateOfCharge" },
- { HUP_BATTERY, HUB_REM_CAPACITY,
- SENSOR_PERCENT, "RemainingCapacity" },
- { HUP_BATTERY, HUB_FULLCHARGE_CAPACITY,
- SENSOR_PERCENT, "FullChargeCapacity" },
- { HUP_BATTERY, HUB_CHARGING,
- SENSOR_INDICATOR, "Charging" },
- { HUP_BATTERY, HUB_DISCHARGING,
- SENSOR_INDICATOR, "Discharging" },
- { HUP_BATTERY, HUB_BATTERY_PRESENT,
- SENSOR_INDICATOR, "BatteryPresent" },
- { HUP_POWER, HUP_SHUTDOWN_IMMINENT,
- SENSOR_INDICATOR, "ShutdownImminent" },
- { HUP_BATTERY, HUB_AC_PRESENT,
- SENSOR_INDICATOR, "ACPresent" },
- { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
- SENSOR_TIMEDELTA, "AtRateTimeToFull" }
-};
-
struct upd_report {
size_t size;
- int enabled;
+ int pending;
};
struct upd_sensor {
struct ksensor ksensor;
struct hid_item hitem;
+ struct upd_usage_entry *entry;
int attached;
};
@@ -85,6 +56,7 @@ struct upd_softc {
int sc_num_sensors;
u_int sc_max_repid;
u_int sc_max_sensors;
+ char sc_buf[256];
/* sensor framework */
struct ksensordev sc_sensordev;
@@ -93,6 +65,17 @@ struct upd_softc {
struct upd_sensor *sc_sensors;
};
+struct upd_usage_entry {
+ uint8_t usage_pg;
+ uint8_t usage_id;
+ uint8_t parent_pg;
+ uint8_t parent_id;
+ enum sensor_type senstype;
+ char *usage_name; /* sensor string */
+ void (*update_sensor)(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+};
+
int upd_match(struct device *, void *, void *);
void upd_attach(struct device *, struct device *, void *);
int upd_detach(struct device *, int);
@@ -100,8 +83,58 @@ int upd_detach(struct device *, int);
void upd_refresh(void *);
void upd_update_sensors(struct upd_softc *, uint8_t *, unsigned int, int);
void upd_intr(struct uhidev *, void *, uint);
+void upd_refresh_sensor_tree(struct upd_softc *, uint8_t, uint8_t, int);
+void upd_get_report_async_cb(void *, int, void *, int);
struct upd_usage_entry *upd_lookup_usage_entry(const struct hid_item *);
struct upd_sensor *upd_lookup_sensor(struct upd_softc *, int, int);
+void upd_update_batdep_sensor(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+void upd_update_sensor_value(struct upd_softc *, struct upd_sensor *,
+ uint8_t *, int);
+
+/* dependency order only for developer sanity */
+static struct upd_usage_entry upd_usage_table[] = {
+ { HUP_POWER, HUP_SHUTDOWN_IMMINENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, "ShutdownImminent",
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_BATTERY_PRESENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, "BatteryPresent",
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_AC_PRESENT,
+ HUP_UNDEFINED, 0,
+ SENSOR_INDICATOR, "ACPresent",
+ upd_update_sensor_value },
+ { HUP_BATTERY, HUB_REL_STATEOF_CHARGE,
+ HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_PERCENT, "RelativeStateOfCharge",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_ABS_STATEOF_CHARGE,
+ HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_PERCENT, "AbsoluteStateOfCharge",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_REM_CAPACITY,
+ HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_PERCENT, "RemainingCapacity",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_FULLCHARGE_CAPACITY,
+ HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_PERCENT, "FullChargeCapacity",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_CHARGING,
+ HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_INDICATOR, "Charging",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_DISCHARGING,
+ HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_INDICATOR, "Discharging",
+ upd_update_batdep_sensor },
+ { HUP_BATTERY, HUB_ATRATE_TIMETOFULL,
+ HUP_BATTERY, HUB_BATTERY_PRESENT,
+ SENSOR_TIMEDELTA, "AtRateTimeToFull",
+ upd_update_batdep_sensor }
+};
struct cfdriver upd_cd = {
NULL, "upd", DV_DULL
@@ -183,20 +216,15 @@ upd_attach(struct device *parent, struct
sc->sc_num_sensors < sc->sc_max_sensors; ) {
DPRINTF(("upd: repid=%d\n", item.report_ID));
if (item.kind != hid_feature ||
- item.report_ID < 0)
+ item.report_ID < 0 ||
+ item.report_ID >= sc->sc_max_repid)
continue;
-
if ((entry = upd_lookup_usage_entry(&item)) == NULL)
continue;
-
- /* filter repeated usages, avoid duplicated sensors */
- sensor = upd_lookup_sensor(sc, entry->usage_pg,
- entry->usage_id);
- if (sensor && sensor->attached)
+ if (upd_lookup_sensor(sc, entry->usage_pg, entry->usage_id))
continue;
sensor = &sc->sc_sensors[sc->sc_num_sensors];
- /* keep our copy of hid_item */
memcpy(&sensor->hitem, &item, sizeof(struct hid_item));
strlcpy(sensor->ksensor.desc, entry->usage_name,
sizeof(sensor->ksensor.desc));
@@ -204,17 +232,14 @@ upd_attach(struct device *parent, struct
sensor->ksensor.flags |= SENSOR_FINVALID;
sensor->ksensor.status = SENSOR_S_UNKNOWN;
sensor->ksensor.value = 0;
+ sensor->entry = entry;
sensor_attach(&sc->sc_sensordev, &sensor->ksensor);
sensor->attached = 1;
sc->sc_num_sensors++;
- if (item.report_ID >= sc->sc_max_repid ||
- sc->sc_reports[item.report_ID].enabled)
- continue;
-
sc->sc_reports[item.report_ID].size = hid_report_size(desc,
size, item.kind, item.report_ID);
- sc->sc_reports[item.report_ID].enabled = 1;
+ sc->sc_reports[item.report_ID].pending = 0;
}
hid_end_parse(hdata);
DPRINTF(("upd: sc_num_sensors=%d\n", sc->sc_num_sensors));
@@ -262,29 +287,98 @@ void
upd_refresh(void *arg)
{
struct upd_softc *sc = (struct upd_softc *)arg;
+ struct upd_sensor *sensor;
struct upd_report *report;
- uint8_t buf[256];
- int repid, actlen;
+ int i, pending;
- for (repid = 0; repid < sc->sc_max_repid; repid++) {
- report = &sc->sc_reports[repid];
- if (!report->enabled)
+ pending = 0;
+ for (i = 0; i < sc->sc_num_sensors; i++) {
+ sensor = &sc->sc_sensors[i];
+ if (!sensor->attached)
continue;
+ report = &sc->sc_reports[sensor->hitem.report_ID];
+ if (report->pending) {
+ pending = 1;
+ sensor->ksensor.status = SENSOR_S_UNKNOWN;
+ sensor->ksensor.flags |= SENSOR_FINVALID;
+ DPRINTF(("%s: repid=%d still pending\n",
+ sc->sc_sensordev.xname, sensor->hitem.report_ID));
+ }
+ }
- memset(buf, 0x0, sizeof(buf));
- actlen = uhidev_get_report(sc->sc_hdev.sc_parent,
- UHID_FEATURE_REPORT, repid, buf, report->size);
+ if (pending)
+ printf("%s: async reports still pending\n",
+ sc->sc_sensordev.xname);
+ else
+ upd_refresh_sensor_tree(sc, HUP_UNDEFINED, 0, 1);
+}
- if (actlen == -1) {
- DPRINTF(("upd: failed to get report id=%02x\n", repid));
+void
+upd_refresh_sensor_tree(struct upd_softc *sc, uint8_t page, uint8_t usage,
+ int valid)
+{
+ struct upd_sensor *sensor;
+ struct upd_report *report;
+ int i, repid;
+
+ for (i = 0; i < sc->sc_num_sensors; i++) {
+ sensor = &sc->sc_sensors[i];
+ if (sensor->entry->parent_pg != page ||
+ sensor->entry->parent_id != usage)
continue;
+ repid = sensor->hitem.report_ID;
+ report = &sc->sc_reports[repid];
+ if (report->pending)
+ continue;
+
+ if (valid && sensor->attached &&
+ uhidev_get_report_async(sc->sc_hdev.sc_parent,
+ UHID_FEATURE_REPORT, repid, sc->sc_buf, report->size,
+ sc, upd_get_report_async_cb) == report->size) {
+ DPRINTF(("%s: repid=%d requested by %s\n",
+ sc->sc_sensordev.xname, repid,
+ sensor->ksensor.desc));
+ report->pending = 1;
+ } else {
+ DPRINTF(("%s: repid=%d invalidated by %s\n",
+ sc->sc_sensordev.xname, repid,
+ sensor->ksensor.desc));
+ upd_get_report_async_cb(sc, repid, NULL, -1);
}
+ }
+}
- /* Deal with buggy firmwares. */
- if (actlen < report->size)
- report->size = actlen;
+void
+upd_get_report_async_cb(void *priv, int repid, void *data, int len)
+{
+ struct upd_softc *sc = priv;
+ struct upd_report *report;
+ struct upd_sensor *sensor;
+ int i, valid;
- upd_update_sensors(sc, buf, report->size, repid);
+ report = &sc->sc_reports[repid];
+ report->pending = 0;
+ if (len > 0 && report->size != len) {
+ printf("%s: adjusting size of repid %d\n",
+ sc->sc_sensordev.xname, repid);
+ report->size = len;
+ }
+
+ for (i = 0; i < sc->sc_num_sensors; i++) {
+ sensor = &sc->sc_sensors[i];
+ if (sensor->hitem.report_ID == repid) {
+ valid = (len > 0 && sensor->attached);
+ if (valid)
+ sensor->entry->update_sensor(sc,
+ sensor, data, len);
+ else {
+ sensor->ksensor.status = SENSOR_S_UNKNOWN;
+ sensor->ksensor.flags |= SENSOR_FINVALID;
+ }
+ upd_refresh_sensor_tree(sc,
+ HID_GET_USAGE_PAGE(sensor->hitem.usage),
+ HID_GET_USAGE(sensor->hitem.usage), valid);
+ }
}
}
@@ -319,56 +413,49 @@ upd_lookup_sensor(struct upd_softc *sc,
}
void
-upd_update_sensors(struct upd_softc *sc, uint8_t *buf, unsigned int len,
- int repid)
+upd_update_batdep_sensor(struct upd_softc *sc, struct upd_sensor *sensor,
+ uint8_t *buf, int len)
{
- struct upd_sensor *sensor;
- ulong hdata, batpres;
- ulong adjust;
- int i;
-
- sensor = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
- batpres = sensor ? sensor->ksensor.value : -1;
-
- for (i = 0; i < sc->sc_num_sensors; i++) {
- sensor = &sc->sc_sensors[i];
- if (!(sensor->hitem.report_ID == repid && sensor->attached))
- continue;
-
- /* invalidate battery dependent sensors */
- if (HID_GET_USAGE_PAGE(sensor->hitem.usage) == HUP_BATTERY &&
- batpres <= 0) {
- /* exception to the battery sensor itself */
- if (HID_GET_USAGE(sensor->hitem.usage) !=
- HUB_BATTERY_PRESENT) {
- sensor->ksensor.status = SENSOR_S_UNKNOWN;
- sensor->ksensor.flags |= SENSOR_FINVALID;
- continue;
- }
- }
+ struct upd_sensor *bat;
+ int batpres = 0;
- switch (HID_GET_USAGE(sensor->hitem.usage)) {
- case HUB_REL_STATEOF_CHARGE:
- case HUB_ABS_STATEOF_CHARGE:
- case HUB_REM_CAPACITY:
- case HUB_FULLCHARGE_CAPACITY:
- adjust = 1000; /* scale adjust */
- break;
- default:
- adjust = 1; /* no scale adjust */
- break;
- }
+ bat = upd_lookup_sensor(sc, HUP_BATTERY, HUB_BATTERY_PRESENT);
+ if (bat && !(bat->ksensor.flags & SENSOR_FINVALID))
+ batpres = bat->ksensor.value;
+
+ if (batpres)
+ upd_update_sensor_value(sc, sensor, buf, len);
+ else {
+ sensor->ksensor.status = SENSOR_S_UNKNOWN;
+ sensor->ksensor.flags |= SENSOR_FINVALID;
+ }
+}
- hdata = hid_get_data(buf, len, &sensor->hitem.loc);
+void
+upd_update_sensor_value(struct upd_softc *sc, struct upd_sensor *sensor,
+ uint8_t *buf, int len)
+{
+ int64_t hdata, adjust;
- sensor->ksensor.value = hdata * adjust;
- sensor->ksensor.status = SENSOR_S_OK;
- sensor->ksensor.flags &= ~SENSOR_FINVALID;
- DPRINTF(("%s: hidget data: %lu\n",
- sc->sc_sensordev.xname, hdata));
+ switch (HID_GET_USAGE(sensor->hitem.usage)) {
+ case HUB_REL_STATEOF_CHARGE:
+ case HUB_ABS_STATEOF_CHARGE:
+ case HUB_REM_CAPACITY:
+ case HUB_FULLCHARGE_CAPACITY:
+ adjust = 1000; /* scale adjust */
+ break;
+ default:
+ adjust = 1; /* no scale adjust */
+ break;
}
-}
+ hdata = hid_get_data(buf, len, &sensor->hitem.loc);
+ sensor->ksensor.value = hdata * adjust;
+ sensor->ksensor.status = SENSOR_S_OK;
+ sensor->ksensor.flags &= ~SENSOR_FINVALID;
+ DPRINTF(("%s: %s hidget data: %lld\n",
+ sc->sc_sensordev.xname, sensor->ksensor.desc, hdata));
+}
void
upd_intr(struct uhidev *uh, void *p, uint len)
