On 20.02.2014, at 12:23, Martin Pieuchot <mpieuc...@nolizard.org> wrote:
> 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?
>
Haha, I think we also had this discussion before.
You’re really insisting on this one, don't you?
OK, for the sake of "standardization, correctness”, I see the point
of using the (ugly) uintX_t types for new code.
>> + 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.
>
I don't like this approach. Because in the end I have “one” UPS with
several sensors. Splitting upd(4) into several devices makes it impossible
to associate them to a specific UPS. It gets worse when attaching two or
more UPSes to the system (which might happen). Having upd0, upd1 etc.
per UPS makes much more sense.
That’s why I proposed the upd(4) sensor super-driver that collects
information from the attached HIDs and presents them with a single sensor.
With your HID simplification diff, this becomes even easier and a bit cleaner.
Reyk
> 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);
>> +}
