Created attachment 69305 [details] [review] Logitech Unifying devices support Here's my first shot for adding support for Logitech Unifying devices. This has been tested and works with at least Logitech K750 keyboards and M705 mouses. I'm planning to add more models supported as I get time and specifications. I've some contacts with Logitech to help me with that. But first I'd like to know if the current patch is fine enough to be merged. The following changes since commit fd32f0be0f16b9420bdecd57d4a0cdcfc3882960: Do not continue to poll if /proc/timer_stats is not readable (2012-10-03 16:20:20 +0100) are available in the git repository at: git://people.freedesktop.org/~jdanjou/upower jd/logitech-unifying for you to fetch changes up to f2993988290fbc9374217eeb83ff21ba04cf167c: Add support for Logitech Unifying devices (2012-10-30 11:57:44 +0100) ---------------------------------------------------------------- Julien Danjou (1): Add support for Logitech Unifying devices libupower-glib/up-device.c | 27 ++ src/linux/Makefile.am | 2 + src/linux/up-backend.c | 30 +- src/linux/up-device-unifying.c | 781 +++++++++++++++++++++++++++++++++ src/linux/up-device-unifying.h | 55 +++ src/org.freedesktop.UPower.Device.xml | 10 + src/up-device.c | 16 + 7 files changed, 910 insertions(+), 11 deletions(-) create mode 100644 src/linux/up-device-unifying.c create mode 100644 src/linux/up-device-unifying.h
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.