After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 724955 - Full OLED support for Wacom Intuos4 Wireless
Full OLED support for Wacom Intuos4 Wireless
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: wacom
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Carlos Garnacho
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2014-02-22 14:43 UTC by Przemo Firszt
Modified: 2014-07-17 14:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to implement OLED handling over bluetooth for intuos4 wireless (8.19 KB, patch)
2014-02-22 14:43 UTC, Przemo Firszt
reviewed Details | Review
1 bit images on OLED screens of Intuos4 Wireless (131.11 KB, image/jpeg)
2014-02-22 14:45 UTC, Przemo Firszt
  Details
Patch to implement OLED handling over bluetooth for intuos4 wireless (8.84 KB, patch)
2014-03-11 22:24 UTC, Przemo Firszt
needs-work Details | Review
Patch to implement OLED handling over bluetooth for intuos4 wireless (9.81 KB, patch)
2014-03-13 20:45 UTC, Przemo Firszt
none Details | Review
wacom: add Bluetooth OLED handling for Intuos4 WL (13.75 KB, patch)
2014-05-07 15:46 UTC, Bastien Nocera
committed Details | Review

Description Przemo Firszt 2014-02-22 14:43: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.
Comment 1 Przemo Firszt 2014-02-22 14:45:27 UTC
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
Comment 2 Carlos Garnacho 2014-02-23 00:31:49 UTC
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;
}
Comment 3 Przemo Firszt 2014-03-11 22:24:29 UTC
Created attachment 271558 [details] [review]
Patch to implement OLED handling over bluetooth for intuos4 wireless

v2
Comment 4 Przemo Firszt 2014-03-11 22:32:43 UTC
(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?
Comment 5 Bastien Nocera 2014-03-13 10:45:27 UTC
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
Comment 6 Przemo Firszt 2014-03-13 20:44:45 UTC
(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
Comment 7 Przemo Firszt 2014-03-13 20:45:50 UTC
Created attachment 271795 [details] [review]
Patch to implement OLED handling over bluetooth for intuos4 wireless

v3
Comment 8 Przemo Firszt 2014-05-06 21:51:42 UTC
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
Comment 9 Bastien Nocera 2014-05-07 15:46:25 UTC
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 10 Bastien Nocera 2014-05-07 15:46:46 UTC
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
Comment 11 Bastien Nocera 2014-07-17 14:39:00 UTC
This was fixed in gnome-3-12 and master unless I'm mistaken. Please reopen if there are any more problems.