| Summary: | Support for Logitech Unifying devices | ||
|---|---|---|---|
| Product: | upower | Reporter: | Julien Danjou <julien> |
| Component: | general | Assignee: | Richard Hughes <richard> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | medium | CC: | bugzilla |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Logitech Unifying devices support
0001-Add-lux-property.patch 0002-Add-support-for-Logitech-Unifying-devices.patch Add luminosity property Add support for Logitech Unifying devices 0001-Add-luminosity-property.patch 0002-Add-support-for-Logitech-Unifying-devices.patch |
||
|
Description
Julien Danjou
2012-10-30 12:56:38 UTC
Comment on attachment 69305 [details] [review] Logitech Unifying devices support Review of attachment 69305 [details] [review]: ----------------------------------------------------------------- I would split off the addition of the Lux property from the rest of the code. ::: libupower-glib/up-device.c @@ +1256,5 @@ > + * > + * Since: 0.9.19 > + **/ > + g_object_class_install_property (object_class, > + PROP_VOLTAGE, PROP_LUX ::: src/linux/Makefile.am @@ +30,4 @@ > up-device-supply.h \ > up-device-csr.c \ > up-device-csr.h \ > + up-device-unifying.c \ up-device-lg-unifying? ::: src/linux/up-backend.c @@ +167,3 @@ > /* check input device */ > input = up_input_new (); > ret = up_input_coldplug (input, backend->priv->daemon, native); The up_input_coldplug() code probably needs to be renamed to something clearer. ::: src/org.freedesktop.UPower.Device.xml @@ +485,5 @@ > + <property name="Lux" type="d" access="read"> > + <doc:doc> > + <doc:description> > + <doc:para> > + Lux (light) being recorded by the meter. Unit? Created attachment 69313 [details] [review] 0001-Add-lux-property.patch > I would split off the addition of the Lux property from the rest of the code. Done. > ::: libupower-glib/up-device.c > @@ +1256,5 @@ > > + * > > + * Since: 0.9.19 > > + **/ > > + g_object_class_install_property (object_class, > > + PROP_VOLTAGE, > PROP_LUX Thanks, fixed! > ::: src/linux/Makefile.am > @@ +30,4 @@ > > up-device-supply.h \ > > up-device-csr.c \ > > up-device-csr.h \ > > + up-device-unifying.c \ > > up-device-lg-unifying? Renamed. > ::: src/linux/up-backend.c > @@ +167,3 @@ > > /* check input device */ > > input = up_input_new (); > > ret = up_input_coldplug (input, backend->priv->daemon, native); > > The up_input_coldplug() code probably needs to be renamed to something clearer. Ok but this function does not come from my patch, it's tied to up-input.c, and I think the name makes sense and follow the logic. > ::: src/org.freedesktop.UPower.Device.xml > @@ +485,5 @@ > > + <property name="Lux" type="d" access="read"> > > + <doc:doc> > > + <doc:description> > > + <doc:para> > > + Lux (light) being recorded by the meter. > > Unit? Actually, lux is the unit. http://en.wikipedia.org/wiki/Lux Created attachment 69315 [details] [review] 0002-Add-support-for-Logitech-Unifying-devices.patch (In reply to comment #3) <snip> > > Unit? > > Actually, lux is the unit. > http://en.wikipedia.org/wiki/Lux I know the Lux unit, thought it was a name you used with reference to something else called Lux. It should be "Luminosity" instead. Sorry, misunderstood you indeed. :) I've modified both patches. Created attachment 69316 [details] [review] Add luminosity property Created attachment 69317 [details] [review] Add support for Logitech Unifying devices First and foremost, thanks for adding this, it's awesome. > +/* Arbitraty value used in ping */ Spelling? > +/* I wish i has the spec for this, but I don't so I invented the name */ > +#define HIDPP_FEATURE_K750_BATTERY 0x4301 > +#define HIDPP_FEATURE_K750_BATTERY_FUNCTION_STARTLUXANDBATTERY (0x00 << 4) > +#define HIDPP_FEATURE_K750_BATTERY_FUNCTION_LUXANDBATTERYEVENT (0x01 << 4) Heh, good enough for me. :) > + lux = (buf[5] << 8) | buf[6]; > + if (lux > 200) > + g_object_set (device, > + "state", UP_DEVICE_STATE_CHARGING, > + "power-supply", TRUE, > + NULL); > + else if (lux > 0) Multiline statements surrounded by {} please. > +#define foreach_unifying_response(fd, buf, size, timer) \ > + do { \ > + timer = g_get_monotonic_time (); \ > + } while (0); \ > + while (up_device_unifying_read_response (fd, buf, size, timer) > 0) > + > +#define foreach_unifying_response_len(fd, buf, size, timer, len) \ > + do { \ > + timer = g_get_monotonic_time (); \ > + } while (0); \ > + while ((len = up_device_unifying_read_response (fd, buf, size, timer)) > 0) I don't like these at all, can you expand them out in the code please. > +static gboolean > +up_device_unifying_set_device_type(UpDeviceUnifying *unifying) Space after the function name please. > + if(g_strcmp0(g_udev_device_get_property (native, "ID_VENDOR_ID"), > + USB_VENDOR_ID_LOGITECH) || > + (g_strcmp0(g_udev_device_get_property (native, "ID_MODEL_ID"), > + USB_DEVICE_ID_UNIFYING_RECEIVER) && > + g_strcmp0(g_udev_device_get_property (native, "ID_MODEL_ID"), > + USB_DEVICE_ID_UNIFYING_RECEIVER_2))) { > + g_debug ("Not an Unifying device, ignoring"); > + return FALSE; > + } I'm wondering if we should add a udev property here and just check for that, it makes adding new devices in the future much easier. > + len = strlen (bus_address); > + if (bus_address[len - 3] != ':' || !g_ascii_isdigit (bus_address[len - 2])) { > + g_debug ("Invalid Unifying device index, ignoring"); > + return FALSE; > + } Do we need to add a check for len here? > + unifying->priv->device_index = g_ascii_digit_value (bus_address[len - 2]); > + > + /* Find the hidraw device of the parent (the receiver) to > + * communicate with the devices */ > + gudev_client = g_udev_client_new (NULL); Is there no way of getting this without searching for the hidraw device? > /** > + * UpDevice:voltage: > + */ > + g_object_class_install_property (object_class, > + PROP_LUX, > + g_param_spec_double ("lux", NULL, NULL, > + 0.0, G_MAXDOUBLE, 0.0, > + G_PARAM_READWRITE)); You mean UpDevice:Lux :) Richard. (In reply to comment #9) > First and foremost, thanks for adding this, it's awesome. > > > +/* Arbitraty value used in ping */ > > Spelling? Fixed. > > + lux = (buf[5] << 8) | buf[6]; > > + if (lux > 200) > > + g_object_set (device, > > + "state", UP_DEVICE_STATE_CHARGING, > > + "power-supply", TRUE, > > + NULL); > > + else if (lux > 0) > > Multiline statements surrounded by {} please. Fixed. > > +#define foreach_unifying_response(fd, buf, size, timer) \ > > + do { \ > > + timer = g_get_monotonic_time (); \ > > + } while (0); \ > > + while (up_device_unifying_read_response (fd, buf, size, timer) > 0) > > + > > +#define foreach_unifying_response_len(fd, buf, size, timer, len) \ > > + do { \ > > + timer = g_get_monotonic_time (); \ > > + } while (0); \ > > + while ((len = up_device_unifying_read_response (fd, buf, size, timer)) > 0) > > I don't like these at all, can you expand them out in the code please. Fixed. > > +static gboolean > > +up_device_unifying_set_device_type(UpDeviceUnifying *unifying) > > Space after the function name please. Fixed. > > + if(g_strcmp0(g_udev_device_get_property (native, "ID_VENDOR_ID"), > > + USB_VENDOR_ID_LOGITECH) || > > + (g_strcmp0(g_udev_device_get_property (native, "ID_MODEL_ID"), > > + USB_DEVICE_ID_UNIFYING_RECEIVER) && > > + g_strcmp0(g_udev_device_get_property (native, "ID_MODEL_ID"), > > + USB_DEVICE_ID_UNIFYING_RECEIVER_2))) { > > + g_debug ("Not an Unifying device, ignoring"); > > + return FALSE; > > + } > > I'm wondering if we should add a udev property here and just check for that, > it makes adding new devices in the future much easier. As you please. I don't think it's likely to happen that often, but why not. > > + len = strlen (bus_address); > > + if (bus_address[len - 3] != ':' || !g_ascii_isdigit (bus_address[len - 2])) { > > + g_debug ("Invalid Unifying device index, ignoring"); > > + return FALSE; > > + } > > Do we need to add a check for len here? It's better, I've added it. Thanks! > > + unifying->priv->device_index = g_ascii_digit_value (bus_address[len - 2]); > > + > > + /* Find the hidraw device of the parent (the receiver) to > > + * communicate with the devices */ > > + gudev_client = g_udev_client_new (NULL); > > Is there no way of getting this without searching for the hidraw device? We need the hidraw device of the parent to talk to the device via the receiver itself, and that's the best way I found. :) > > /** > > + * UpDevice:voltage: > > + */ > > + g_object_class_install_property (object_class, > > + PROP_LUX, > > + g_param_spec_double ("lux", NULL, NULL, > > + 0.0, G_MAXDOUBLE, 0.0, > > + G_PARAM_READWRITE)); > > You mean UpDevice:Lux :) That was already fixed when changed to luminosity actually. :) Created attachment 69320 [details] [review] 0001-Add-luminosity-property.patch Created attachment 69321 [details] [review] 0002-Add-support-for-Logitech-Unifying-devices.patch Both pushed to master, thanks! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.