GNOME Bugzilla – Bug 724955
Full OLED support for Wacom Intuos4 Wireless
Last modified: 2014-07-17 14:39:00 UTC
Created attachment 269993 [details] [review] Patch to implement OLED handling over bluetooth for intuos4 wireless Attached patch implements OLED handling over bluetooth connection. It also make some changes to the way OLEDs are managed with USB connection, but those changes are limited to code refactoring and should not have any user experience impact. Due to the nature of the device which accepts 4 bit colour images over USB and only 1 bit colour images we have to handle those 2 situations differently. On top of that kernel side has been implemented in such way that the images for USB connection have to be scrambled in userland before they are send to the device. The decision to move scrambling to the OLED helper and to have 4-bit to 1-bit conversion in the same location is the cleanest solution that I found to the problem of inconsistent image formatting. It changes the previous approach to keep scrambling out of the helper which is executed with elevated privileges. 4-bit to 1-bit colour used in the patch simply uses the most significant bit. It's probably less than perfect, but gives quite good results. Please comment if you have an idea for a better solution.
Created attachment 269994 [details] 1 bit images on OLED screens of Intuos4 Wireless The result of 4 bit to 1 bit conversion on OLED screens of Intuos4 Wireless
Review of attachment 269993 [details] [review]: Thanks Przemo for the patch! I don't think it is a big problem to move scrambling to the helper executable, but I would recommend adding some check to bail out if the buffer received doesn't match the expected length, even though the loops write over restricted lengths, there's still the possibility of invalid memory writes if a smaller buffer is given. ::: plugins/wacom/gsd-wacom-oled-helper.c @@ -36,1 +36,5 @@ +#define OLED_WIDTH 64 +#define OLED_HEIGHT 32 +#define MAX_IMAGE_SIZE OLED_HEIGHT * OLED_WIDTH / 2 /*size of OLED screen * 4 bit per pixel - usb mode*/ +#define BT_BUF_LEN OLED_WIDTH * OLED_HEIGHT / 8 /*size of OLED screen * 1 bit per pixel - bluetooth mode*/ These two defines should probably share similar names, or maybe have #define USB_PIXEL_STRIDE 4 #define BT_PIXEL_STRIDE 1 #define IMAGE_SIZE(w,h,ps) (h * w / (8 / ps)) Although I do hope there's no more pixel formats to handle :) @@ -57,2 +88,7 @@ - /* write to device file */ - retval = write (fd, image, length); + if (usb) { + /* Image has to be scrambled for devices connected over USB ... */ + oled_scramble_icon (image); ... 4 more ... This could maybe move to a separate oled_image_to_mask() helper function, if both helpers wrote to a common buffer, and the written length was also returned by the helpers, the write() could also be moved out of the if/else. @@ +237,3 @@ } if (g_strcmp0 (g_udev_device_get_property (device, "ID_BUS"), "usb") != 0) { It would be preferable here to have: if (g_strcmp0 (id_bus, "usb") == 0) { /* Do usb */ } else if (check_this_is_bt) { /* Do bt */ } else { g_critical ("Not a expected device: %s", path); goto out; }
Created attachment 271558 [details] [review] Patch to implement OLED handling over bluetooth for intuos4 wireless v2
(In reply to comment #2) > Review of attachment 269993 [details] [review]: > > Thanks Przemo for the patch! I don't think it is a big problem to move > scrambling to the helper executable, but I would recommend adding some check to > bail out if the buffer received doesn't match the expected length, even though > the loops write over restricted lengths, there's still the possibility of > invalid memory writes if a smaller buffer is given. >Thanks for your comments! I added some checks. > ::: plugins/wacom/gsd-wacom-oled-helper.c > @@ -36,1 +36,5 @@ > > +#define OLED_WIDTH 64 > +#define OLED_HEIGHT 32 > +#define MAX_IMAGE_SIZE OLED_HEIGHT * OLED_WIDTH / 2 /*size of OLED screen * 4 > bit per pixel - usb mode*/ > +#define BT_BUF_LEN OLED_WIDTH * OLED_HEIGHT / 8 /*size of OLED screen * 1 bit > per pixel - bluetooth mode*/ > > These two defines should probably share similar names, or maybe have > #define USB_PIXEL_STRIDE 4 > #define BT_PIXEL_STRIDE 1 > #define IMAGE_SIZE(w,h,ps) (h * w / (8 / ps)) > > Although I do hope there's no more pixel formats to handle :) No, that's all! :-) I used slightly different approach - please comment Buffer sizes for USB and bluetooth OLEDs are (brutally) hardcoded in kernel, so what we have here in terms of flexibility is much more that will ever be needed unless wacom comes up with a new hardware. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-wacom.c?id=fa389e220254c69ffae0d403eac4146171062d08#n428 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/input/tablet/wacom_sys.c?id=fa389e220254c69ffae0d403eac4146171062d08#n878 > @@ -57,2 +88,7 @@ > - /* write to device file */ > - retval = write (fd, image, length); > + if (usb) { > + /* Image has to be scrambled for devices connected over USB ... */ > + oled_scramble_icon (image); > ... 4 more ... > > This could maybe move to a separate oled_image_to_mask() helper function, if > both helpers wrote to a common buffer, and the written length was also returned > by the helpers, the write() could also be moved out of the if/else. Done! > @@ +237,3 @@ > } > > if (g_strcmp0 (g_udev_device_get_property (device, "ID_BUS"), "usb") != 0) > { > > It would be preferable here to have: > > if (g_strcmp0 (id_bus, "usb") == 0) { > /* Do usb */ > } else if (check_this_is_bt) { > /* Do bt */ > } else { > g_critical ("Not a expected device: %s", path); > goto out; > } Done! There is one thing that bothers me: the way we're reporting errors. Both g_debug calls indicate situation where the OLED won't be set. In gsd_wacom_oled_helper_write we have 3 g_set_error calls (1 old + 2 new). Should we change the g_debug calls to g_set_error or maybe the other way? Is there a guideline that I could use for this?
Review of attachment 271558 [details] [review]: ::: plugins/wacom/gsd-wacom-oled-helper.c @@ +43,3 @@ + +static void +oled_scramble_icon (unsigned char* image) guchar. @@ +66,3 @@ + +static int +gsd_wacom_oled_prepare_buf (unsigned char *image, int usb) guchar. And it's not an int, it's a gboolean @@ +128,1 @@ + length = gsd_wacom_oled_prepare_buf (image, usb); You should check the retval here. @@ +263,3 @@ + parent = g_udev_device_get_parent_with_subsystem (device, "usb", "usb_interface"); + if (parent == NULL) { + g_debug ("Could not find parent USB device for '%s'", path); Errors should be g_critical(). @@ +274,3 @@ + parent = g_udev_device_get_parent (device); + if (parent == NULL) { + g_debug ("Could not find parent device for '%s'", path); Ditto. ::: plugins/wacom/gsd-wacom-oled.c @@ +210,3 @@ GsdWacomRotation rotation) { + unsigned char *image; guchar == unsigned char
(In reply to comment #5) Thanks for checking the pathc, Bastien! > Review of attachment 271558 [details] [review]: > > ::: plugins/wacom/gsd-wacom-oled-helper.c > @@ +43,3 @@ > + > +static void > +oled_scramble_icon (unsigned char* image) > > guchar. Done > @@ +66,3 @@ > + > +static int > +gsd_wacom_oled_prepare_buf (unsigned char *image, int usb) > > guchar. Done > And it's not an int, it's a gboolean > > @@ +128,1 @@ > + length = gsd_wacom_oled_prepare_buf (image, usb); > > You should check the retval here. I'm not really convinced if thats OK, because (if (usb) {set len = usb_len} else {set len = bt_len} should never leave len = 0, but I added the check anyway. > @@ +263,3 @@ > + parent = g_udev_device_get_parent_with_subsystem (device, "usb", > "usb_interface"); > + if (parent == NULL) { > + g_debug ("Could not find parent USB device for '%s'", path); > > Errors should be g_critical(). OK > @@ +274,3 @@ > + parent = g_udev_device_get_parent (device); > + if (parent == NULL) { > + g_debug ("Could not find parent device for '%s'", path); > > Ditto. OK > ::: plugins/wacom/gsd-wacom-oled.c > @@ +210,3 @@ > GsdWacomRotation rotation) > { > + unsigned char *image; > > guchar == unsigned char OK, I changed all unsigned char -> guchar
Created attachment 271795 [details] [review] Patch to implement OLED handling over bluetooth for intuos4 wireless v3
Is there anything that needs to be improved to get this patch into gsd? There is currently no support for OLED over BT as https://bugzilla.gnome.org/show_bug.cgi?id=704167 was for LED, not for OLED despite the description of commit 52c1ad8254035f282c854912d14263
Created attachment 276079 [details] [review] wacom: add Bluetooth OLED handling for Intuos4 WL That patch implements OLED handling over bluetooth for Intuos4 Wireless tablets. Due to the nature of the device which accepts 4 bit colour images over USB and only 1 bit colour images we have to handle those 2 situations differently. On top of that kernel side has been implemented in such way that the images for USB connection have to be scrambled in userland before they are send to the device. The decision to move scrambling to the OLED helper and to have 4-bit to 1-bit conversion in the same location is the cleanest solution that I found to the problem of inconsistent image formatting. It changes the previous approach to keep scrambling out of the helper which is executed with elevated privileges.
Comment on attachment 276079 [details] [review] wacom: add Bluetooth OLED handling for Intuos4 WL Attachment 276079 [details] pushed as 288a318 - wacom: add Bluetooth OLED handling for Intuos4 WL
This was fixed in gnome-3-12 and master unless I'm mistaken. Please reopen if there are any more problems.