On 17/02/14(Mon) 01:11, Andre de Oliveira wrote:
> On Fri, Feb 14, 2014 at 02:20:57PM +0100, Ingo Schwarze wrote:
> > Hi,
> >
> > a few comments regarding the manual:
>
> Ingo, thanks for your feedback.
> Here follows an updated version, just documentation changes.
>
> I also submitted it to mandoc-lint, now seems cleaner.
Some comments below.
> +++ b/sys/dev/usb/upd.c
> @@ -0,0 +1,413 @@
> +/* $OpenBSD$ */
> +
> +/*
> + * Copyright (c) 2014 Andre de Oliveira <deoliveira...@googlemail.com>
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +/* Driver for USB Power Devices sensors */
> +
> +#include <sys/param.h>
> +#include <sys/systm.h>
> +#include <sys/kernel.h>
> +#include <sys/malloc.h>
> +#include <sys/device.h>
> +#include <sys/sensors.h>
> +
> +#include <dev/usb/usb.h>
> +#include <dev/usb/usbdi.h>
> +#include <dev/usb/usbdevs.h>
> +#include <dev/usb/usbhid.h>
> +#include <dev/usb/hid.h>
> +
> +#ifdef USB_DEBUG
> +#define UPD_DEBUG
> +#endif
> +
> +#ifdef UPD_DEBUG
> +int upddebug = 9;
For such a simple driver I really recommend to use only one DPRINTF()
level, our USB drivers are not the best examples of simplicity debug
messages...
> +#define DPRINTF(x) do { if (upddebug) printf x; } while (0)
> +#define DPRINTFN(n, x) do { if (upddebug >= (n)) printf x; } while (0)
> +#else
> +#define DPRINTF(x)
> +#define DPRINTFN(n, x)
> +#endif
> +
> +#define UPD_USB_TMOUT 1000
Do you really need a non standard timeout?
> +
> +/* protocol */
> +#define UPD_CMD_GET_BATTNOMV 0x0a
> +#define UPD_CMD_GET_BATTCURV 0x0b
> +#define UPD_CMD_GET_BATTLOAD 0x0c
> +#define UPD_CMD_GET_UPSMISC 0x07 /* misc flags from ups */
I'm not sure to understand what are these magic number and how to you
map them to the Usage ID specified, for example in sec 4.2 Battery
System Page? Don't tell me you're hardcoding the report IDs of your
UPS, no? :)
> +
> +#define UPD_CMD_NUM 4
> +
> +enum upd_sensor_id {
> + UPD_SENSOR_UNKNOWN,
> + UPD_SENSOR_BATTNOMV,
> + UPD_SENSOR_BATTCURV,
> + UPD_SENSOR_BATTLOAD,
> + UPD_SENSOR_BATTCHARGING,
> + UPD_SENSOR_BATTDISCHARG,
> + UPD_SENSOR_BATTPRESENT,
> + UPD_SENSOR_SHUTIMMINENT,
> + UPD_SENSOR_ACPOWER,
> + UPD_SENSOR_ACOFFTIME,
> + UPD_SENSOR_NUM
> +};
> +
> +struct upd_report_param {
> + int enabled;
> + u_int8_t cmd; /* usage id (issue command) */
What about using uint8_t and the other related types, instead of
u_intXX_t, for new code since they are standardized?
> + char *type_s; /* sensor string */
> + struct hid_location loc; /* size, count, pos */
> +};
> +
> +static struct upd_report_param upd_report_params[UPD_SENSOR_NUM] = {
> + { 0, 0x0, "unknown", { 0, 0, 0 } },
> + { 1, UPD_CMD_GET_BATTNOMV, "battery nominal volt", { 0x10, 0x0, 0x0 } },
> + { 1, UPD_CMD_GET_BATTCURV, "battery current volt", { 0x10, 0x0, 0x0 } },
> + { 1, UPD_CMD_GET_BATTLOAD, "battery load", { 0x08, 0x0, 0x0 } },
> + { 1, UPD_CMD_GET_UPSMISC, "battery charging", { 0x01, 0x0, 0x0 } },
> + { 1, UPD_CMD_GET_UPSMISC, "battery discharging", { 0x01, 0x0, 0x01 }
> },
> + { 1, UPD_CMD_GET_UPSMISC, "battery present", { 0x01, 0x0, 0x03 }
> },
> + { 1, UPD_CMD_GET_UPSMISC, "shutdown imminent", { 0x01, 0x0, 0x06 }
> },
> + { 1, UPD_CMD_GET_UPSMISC, "acpower present", { 0x0, 0x0, 0x0 } },
> + { 0, UPD_CMD_GET_UPSMISC, "acpower off time", { 0x0, 0x0, 0x0 } }
> +};
> +
> +struct upd_udata {
> + u_int8_t cmd;
> + u_int16_t dlen;
> + enum hid_kind hkind;
> + u_int8_t data[8];
> + int rfail;
> +};
> +
> +struct upd_sensor {
> + struct ksensor sensor;
> + enum upd_sensor_id id;
> + int attached;
> +};
> +
> +struct upd_softc {
> + struct device sc_hdev;
> + struct usbd_device *sc_udev;
> + int sc_num_sensors;
> +
> + /* sensor framework */
> + struct upd_sensor sc_sensor[UPD_SENSOR_NUM];
> + struct upd_udata sc_usbdata[UPD_CMD_NUM];
> + struct ksensordev sc_sensordev;
> + struct sensor_task *sc_sensortask;
> +};
> +
> +struct upd_type {
> + struct usb_devno upd_dev;
> + u_int16_t upd_flags;
> +};
> +
> +static const struct upd_type upd_devs[] = {
> + { { USB_VENDOR_APC, USB_PRODUCT_APC_UPS }, 0 },
> + { { USB_VENDOR_APC, USB_PRODUCT_APC_UPS5G }, 0 },
> +};
> +#define upd_lookup(v, p) \
> + ((const struct upd_type *)usb_lookup(upd_devs, v, p))
> +
> +int upd_match(struct device *, void *, void *);
> +void upd_attach(struct device *, struct device *, void *);
> +int upd_detach(struct device *, int);
> +
> +void upd_setup_sensors(struct upd_softc *);
> +void upd_refresh(void *);
> +void upd_refresh_data(struct upd_softc *);
> +int upd_read_data(struct upd_softc *, struct upd_udata *, int);
> +struct upd_udata * upd_usbdata_lookup(struct upd_udata *, u_int8_t);
> +
> +struct cfdriver upd_cd = {
> + NULL, "upd", DV_DULL
> +};
> +
> +const struct cfattach upd_ca = {
> + sizeof(struct upd_softc),
> + upd_match,
> + upd_attach,
> + upd_detach
> +};
> +
> +int
> +upd_match(struct device *parent, void *match, void *aux)
> +{
> + struct usb_attach_arg *uaa = aux;
> +
> + if (upd_lookup(uaa->vendor, uaa->product) == NULL)
> + return (UMATCH_NONE);
> +
> + DPRINTFN(3, ("upd: vendor=0x%x, product=0x%x\n", uaa->vendor,
> + uaa->product));
I wouldn't leave such debug message they are very annoying when
debugging. Plus you can get vendor & product number with usbdevs(8).
> + return (UMATCH_VENDOR_PRODUCT);
> +}
> +
> +void
> +upd_attach(struct device *parent, struct device *self, void *aux)
> +{
> + struct upd_softc *sc = (struct upd_softc *)self;
> + struct usb_attach_arg *uaa = (struct usb_attach_arg *)aux;
> + struct usbd_device *udev;
> + int i;
> +
> + sc->sc_udev = udev = uaa->device;
> + sc->sc_num_sensors = 0;
> +
> + strlcpy(sc->sc_sensordev.xname, sc->sc_hdev.dv_xname,
> + sizeof(sc->sc_sensordev.xname));
> + upd_setup_sensors(sc);
> +
> + /* attach sensors */
> + for (i = 0; i < UPD_SENSOR_NUM; i++) {
> + if (sc->sc_sensor[i].id == UPD_SENSOR_UNKNOWN)
> + continue;
> + sc->sc_sensor[i].sensor.flags |= SENSOR_FINVALID;
> + sc->sc_sensor[i].sensor.value = 0;
> + sensor_attach(&sc->sc_sensordev, &sc->sc_sensor[i].sensor);
> + sc->sc_sensor[i].attached = 1;
> + sc->sc_num_sensors++;
> + }
> +
> + DPRINTFN(3, ("upd: sc_num_sensors=%d\n", sc->sc_num_sensors));
> +
> + if (sc->sc_num_sensors > 0) {
> + sc->sc_sensortask = sensor_task_register(sc, upd_refresh, 6);
> + if (sc->sc_sensortask == NULL) {
> + printf(", unable to register update task\n");
> + return;
> + }
> + sensordev_install(&sc->sc_sensordev);
> + }
> +
> + /*
> + * init usbdata
> + * those commands width were identified from observing apcupsd/ugen(4)
> + * data exchange
> + */
> + sc->sc_usbdata[0].cmd = UPD_CMD_GET_BATTNOMV;
> + sc->sc_usbdata[0].dlen = 0x03;
> + sc->sc_usbdata[0].hkind = hid_feature;
> + sc->sc_usbdata[1].cmd = UPD_CMD_GET_BATTCURV;
> + sc->sc_usbdata[1].dlen = 0x02;
> + sc->sc_usbdata[1].hkind = hid_feature;
> + sc->sc_usbdata[2].cmd = UPD_CMD_GET_BATTLOAD;
> + sc->sc_usbdata[2].dlen = 0x03;
> + sc->sc_usbdata[2].hkind = hid_feature;
> + sc->sc_usbdata[3].cmd = UPD_CMD_GET_UPSMISC;
> + sc->sc_usbdata[3].dlen = 0x03;
> + sc->sc_usbdata[3].hkind = hid_feature;
Even if for the moment you're only dealing with Feature report, be
prepared to support Input and Ouput too, see below.
But instead of hardcoding magic values, you should iterate over the
report IDs available and try to find the Usage ID your driver supports.
Have a look at the various HUP_* macro, HID_USAGE2() hid_is_collection()
and friend.
Maybe you'll find out that it's easier to split your driver into various
drivers, for example one dealing with the Power Device Page and another
one with the Battery System Page...
If your driver is generic enough you might even have various upd(4)
instances, one for each report ID, that just parse the available usages
and map them to sensors.
Have a look at hidms(4) it do some parsing based on a given report ID
provided by ums(4).
You'll need a lot of device descriptors to see how the mappings can
differ.
> +
> + DPRINTFN(3, ("upd_attach: complete\n"));
Same comment as before regarding debugging ;)
> +}
> +
> +int
> +upd_detach(struct device *self, int flags)
> +{
> + struct upd_softc *sc = (struct upd_softc *)self;
> + int i;
> +
> + if (sc->sc_num_sensors > 0) {
> + wakeup(&sc->sc_sensortask);
> + sensordev_deinstall(&sc->sc_sensordev);
> + for (i = 0; i < UPD_SENSOR_NUM; i++) {
> + if (sc->sc_sensor[i].attached) {
> + sensor_detach(&sc->sc_sensordev,
> + &sc->sc_sensor[i].sensor);
> + DPRINTFN(5, ("upd_detach: %s\n",
> + sc->sc_sensor[i].sensor.desc));
> + }
> + }
> + if (sc->sc_sensortask != NULL)
> + sensor_task_unregister(sc->sc_sensortask);
> + }
> +
> + DPRINTFN(3, ("upd_detach: complete\n"));
> + return (0);
> +}
> +
> +int
> +upd_read_data(struct upd_softc *sc, struct upd_udata *udata, int tmout)
> +{
> + usb_device_request_t req;
> + int ucr_actlen = 0;
> + int err = 0;
> + enum hid_kind hkind;
> + size_t buflen;
> + u_int16_t dlen;
> + u_int8_t cmd, *buf = NULL;
> + void *ptr = NULL;
> +
> + buf = udata->data;
> + cmd = udata->cmd;
> + dlen = udata->dlen;
> + hkind = udata->hkind;
> + buflen = sizeof(buf);
> +
> + if (buf == NULL || buflen == 0)
> + return (0);
> +
> + ptr = malloc(buflen, M_TEMP, M_WAITOK);
> +
> + req.bmRequestType = UT_READ_CLASS_INTERFACE;
> + req.bRequest = UR_GET_REPORT;
> +
> + USETW(req.wValue, ((hkind + 1) << 8) | cmd);
This won't work on all architecture, have a look at USETW2() if you
want to see why ;). But more importantly, you clearly want to use
uhidev_get_report(), here with UHID_FEATURE_REPORT, instead of rewriting
your own: upd_read_data().
I would even suggest to add a uhidev_get_report_async() so that you can
submit your request and don't wait for it to be processed. The sensor
will then be updated when the USB transfer is completed.
> + USETW(req.wIndex, 0);
> + USETW(req.wLength, dlen);
> +
> + err = usbd_do_request_flags(sc->sc_udev, &req, ptr, USBD_SHORT_XFER_OK,
> + &ucr_actlen, tmout);
> +
> + if (err)
> + goto ret;
> +
> + memcpy(buf, ptr, buflen);
> +
> + DPRINTFN(5, ("actlen=%02x ptr[0]=%02x ptr[1]=%02x ptr[2]=%02x\n",
> + ucr_actlen, ((u_int8_t *)ptr)[0], ((u_int8_t *)ptr)[1],
> + ((u_int8_t *)ptr)[2]));
> +
> +ret:
> + if (ptr)
> + free(ptr, M_TEMP);
> +
> + return (err);
> +}
> +
> +void
> +upd_setup_sensors(struct upd_softc *sc)
> +{
> + int i;
> +
> + for (i = 0; i < UPD_SENSOR_NUM; i++) {
> + if (upd_report_params[i].enabled != 1)
> + continue;
> +
> + sc->sc_sensor[i].id = i;
> + strlcpy(sc->sc_sensor[i].sensor.desc,
> + upd_report_params[i].type_s,
> + sizeof(sc->sc_sensor[i].sensor.desc));
> + sc->sc_sensor[i].attached = 0;
> +
> + switch (i) {
> + case UPD_SENSOR_BATTNOMV:
> + case UPD_SENSOR_BATTCURV:
> + sc->sc_sensor[i].sensor.type = SENSOR_VOLTS_DC;
> + break;
> + case UPD_SENSOR_BATTLOAD:
> + sc->sc_sensor[i].sensor.type = SENSOR_PERCENT;
> + break;
> + case UPD_SENSOR_SHUTIMMINENT:
> + case UPD_SENSOR_BATTCHARGING:
> + case UPD_SENSOR_BATTDISCHARG:
> + case UPD_SENSOR_BATTPRESENT:
> + case UPD_SENSOR_ACPOWER:
> + sc->sc_sensor[i].sensor.type = SENSOR_INDICATOR;
> + break;
> + }
> + }
> +}
> +
> +void
> +upd_refresh(void *arg)
> +{
> + struct upd_softc *sc = (struct upd_softc *)arg;
> + struct hid_location *loc;
> + struct upd_udata *udata;
> + u_long hdata;
> + u_int8_t cmd;
> + u_int8_t *buf;
> + int i;
> +
> + upd_refresh_data(sc);
Instead of sending USB commands and waiting for all of them to complete
before updating the sensors (that means in worst case, with your custom
timeout, you'll wait n seconds, where n is the number of sensors you
have), you should use a callback that'll update the data of your
sensors.
> +
> + for (i = 0; i < UPD_SENSOR_NUM; i++) {
> + if (sc->sc_sensor[i].attached != 1 ||
> + upd_report_params[i].enabled != 1)
> + continue;
> +
> + cmd = upd_report_params[i].cmd;
> + udata = upd_usbdata_lookup(sc->sc_usbdata, cmd);
By matching reportid you would avoid such lookup.
> +
> + /* skip on eventual read failures or unsupported features */
> + if (udata->rfail)
> + continue;
> +
> + buf = udata->data;
> + loc = &upd_report_params[i].loc;
> + hdata = hid_get_data(buf + 1, loc);
> +
> + switch (i) {
> + case UPD_SENSOR_BATTNOMV:
> + case UPD_SENSOR_BATTCURV:
> + case UPD_SENSOR_BATTLOAD:
> + if (sc->sc_sensor[UPD_SENSOR_BATTPRESENT].sensor.value)
> + hdata *= 1000; /* scale adjust */
If I'm not mistaken, units are properly specified by the HID Power
Devices specification. It would be nice to have a better comment a be
prepared to handle other conversions ;)
> + else
> + hdata = 0;
> + break;
> + case UPD_SENSOR_ACPOWER:
> + /* use discharging flag as acpower indicator */
> + if (sc->sc_sensor[UPD_SENSOR_BATTDISCHARG].sensor.value)
> + hdata = 0;
> + else
> + hdata = 1;
> + break;
> + }
> +
> + sc->sc_sensor[i].sensor.flags &= ~SENSOR_FINVALID;
> + sc->sc_sensor[i].sensor.value = hdata;
> + DPRINTFN(3, ("%s: %s: hidget data: %d\n",
> + sc->sc_sensordev.xname,
> + upd_report_params[i].type_s, hdata));
> + }
> +}
> +
> +void
> +upd_refresh_data(struct upd_softc *sc)
> +{
> + struct upd_udata *udata;
> + int i, ret;
> +
> + for (i = 0; i < UPD_CMD_NUM; i++) {
> + udata = &sc->sc_usbdata[i];
> + bzero(udata->data, sizeof(udata->data));
> + ret = upd_read_data(sc, udata, UPD_USB_TMOUT);
> + if (ret != USBD_NORMAL_COMPLETION) {
> + DPRINTFN(5, ("upd: data read fail\n"));
> + udata->rfail = 1;
> + }
> + udata->rfail = 0;
> + }
> +}
> +
> +struct upd_udata *
> +upd_usbdata_lookup(struct upd_udata *usbdata, u_int8_t cmd)
> +{
> + int i;
> +
> + for (i = 0; i < UPD_CMD_NUM; i++)
> + if (usbdata[i].cmd == cmd)
> + return (&usbdata[i]);
> +
> + return (NULL);
> +}
>
