Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

8201568: zForce touchscreen input device fails when closed and immediately reopened #258

Open
wants to merge 2 commits into
base: master
from

Conversation

@jgneff
Copy link
Contributor

jgneff commented Jun 27, 2020

Fixes JDK-8201568.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8201568: zForce touchscreen input device fails when closed and immediately reopened

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/258/head:pull/258
$ git checkout pull/258

鈥tely reopened
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 27, 2020

馃憢 Welcome back jgneff! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Jun 27, 2020
@mlbridge
Copy link

mlbridge bot commented Jun 27, 2020

Webrevs

@jgneff
Copy link
Contributor Author

jgneff commented Jun 27, 2020

I thought I could avoid fixing this bug simply by waiting a few years, but the old devices from 2012 and 2013 where the problem occurs are still supported and still even being sold (refurbished) by the manufacturer. The Monocle EPD platform has a work-around for the bug in the method EPDInputDeviceRegistry.createDevice.

The problem does not occur on newer 2015 and 2018 devices that I tested.

Background

The Neonode zForce touchscreen input device almost always fails when it is opened immediately after being closed. Once it fails, no input events are received. The device driver logs the following messages to the kernel ring buffer:

[drivers/input/touchscreen/zforce_i2c.c-425] zforce_i2c_open()
[zForce_ir_touch_recv_data-145] command Activate (0) ...
[zForce_ir_touch_recv_data-154] command Resolution (0) ...
[zForce_ir_touch_recv_data-179] command Frequency (0) ...
[drivers/input/touchscreen/zforce_i2c.c-440] zforce_i2c_close()
[drivers/input/touchscreen/zforce_i2c.c-425] zforce_i2c_open()
[zForce_ir_touch_recv_data-142] command Deactivate ...
[zForce_ir_touch_recv_data-198] command overrun (8) ...

This pull request reorders the initialization sequence to be "open, open, close" instead of "open, close, open" so that the input device is never actually closed. With this change, the file reference count remains above zero so the call to close it is ignored, and the device driver logs the following messages to the ring buffer:

[drivers/input/touchscreen/zforce_i2c.c-425] zforce_i2c_open()
[zForce_ir_touch_recv_data-145] command Activate (0) ...
[zForce_ir_touch_recv_data-154] command Resolution (0) ...
[zForce_ir_touch_recv_data-179] command Frequency (0) ...

In both cases, when the program exits or is canceled with Ctrl-C, the input device is closed normally:

[drivers/input/touchscreen/zforce_i2c.c-440] zforce_i2c_close()
[zForce_ir_touch_recv_data-142] command Deactivate ...

Summary

I decided to go ahead and submit this pull request for the following reasons:

  1. we know of at least one input device where the code causes problems, so there could be others;
  2. this makes progress towards removing 48 lines of work-around code from EPDInputDeviceRegistry; and
  3. opening and closing an input device is expensive enough that we probably shouldn't close it when we know we're going to open it again only two statements later.
@kevinrushforth kevinrushforth requested a review from johanvos Jun 27, 2020
Copy link
Collaborator

johanvos left a comment

The rationale makes sense (open/open/close) instead of (open/close/open)

@openjdk
Copy link

openjdk bot commented Jun 29, 2020

@jgneff This change is no longer ready for integration - check the PR body for details.

@openjdk openjdk bot added the ready label Jun 29, 2020
@jgneff
Copy link
Contributor Author

jgneff commented Jun 29, 2020

Based on my notes below, I believe this change is safe for any Linux input device driver because the driver shouldn't receive the intermediate open and close calls at all.

Nonetheless, it would be reassuring if someone could try this change just once on a mainstream device, such as the Raspberry Pi with a touchscreen LCD panel. I only have six obscure ARM devices and a headless QEMU ARM virtual machine for testing. @johanvos or @dellgreen - Is that something you could test? If you think it's overkill and not worth it, that's fine, too.

Notes

The Linux kernel documentation about opening and closing input devices states:

Note that input core keeps track of number of users for the device and makes sure that dev->open() is called only when the first user connects to the device and that dev->close() is called when the very last user disconnects. Calls to both callbacks are serialized.

Also, the Linux Programmer's Manual for the close system call (man 2 close) states:

If fd is the last file descriptor referring to the underlying open file description (see open(2)), the resources associated with the open file description are freed.

Running a JavaFX program with strace -f -e trace=open,close -o strace.log shows the one open for the event0 keyboard, and the open, open, close for the event1 touchscreen:

5847  open("/dev/input/event0", O_RDONLY) = 11
...
5847  open("/dev/input/event1", O_RDONLY) = 12
5847  open("/dev/input/event1", O_RDONLY) = 13
5847  close(13)                         = 0

Both devices are actually closed by the kernel when the JavaFX program ends.

@johanvos
Copy link
Collaborator

johanvos commented Jul 1, 2020

I don't have access to a Pi right now, so I can't test this (I'll be able to test in about 10 days from now though)

@jgneff
Copy link
Contributor Author

jgneff commented Jul 9, 2020

I don't have access to a Pi right now, so I can't test this (I'll be able to test in about 10 days from now though)

Thank you, Johan. If you have the time, that would be great. Otherwise, merging this for early-access builds of JavaFX 16 should provide enough time to find any regression errors on other devices.

By the way, do you know whether I would be able to use a PinePhone instead of a Raspberry Pi for testing the Monocle Linux platform (monocle.platform=Linux)? The PinePhone has a quad-core ARM Cortex A53 processor instead of the quad-core ARM Cortex-A72 on the Raspberry Pi 4, but it also has the advantage of a built-in LCD touchscreen.

@Torbuntu
Copy link

Torbuntu commented Jul 27, 2020

By the way, do you know whether I would be able to use a PinePhone

I have been trying to run JavaFX on the PinePhone ever since I got it. The OS I'm using is Fedora and it is aarch64, which means I had to add that arch to build.gradle to compile it. As is the touchscreen doesn't respond correctly at all. Every touch reports as a mouse_entered, mouse_exited event.

Though this is due to using the Desktop JavaFX. Is there somewhere I can find an embedded aarch64 version? Since desktop javafx/OpenJFX only runs monocle in headless according to this: https://wiki.openjdk.java.net/display/OpenJFX/Monocle

@jgneff
Copy link
Contributor Author

jgneff commented Jul 27, 2020

Is there somewhere I can find an embedded aarch64 version?

If I had a PinePhone, I would try to get the 32-bit armhf build of JavaFX working. That's the one you build with gradle -PCOMPILE_TARGETS=armv6hf sdk jmod. You may need to switch operating systems. See the Ask Ubuntu question "Can I run an ARM32 bit App on an ARM64bit platform which is running Ubuntu 16.04?"

@Torbuntu
Copy link

Torbuntu commented Jul 27, 2020

I do not know of a currently supported 32-bit armhf OS for the PinePhone.
This issue affects Desktop JavaFX linux touch as well, so I suppose I'll need to dig into adding that in, which was mentioned on this older issue: javafxports/openjdk-jfx#329 (comment)

Thanks for the quick reply!

@jgneff
Copy link
Contributor Author

jgneff commented Sep 21, 2020

Nonetheless, it would be reassuring if someone could try this change just once on a mainstream device, such as the Raspberry Pi with a touchscreen LCD panel.

I just placed an order for a Raspberry Pi 2 Model B -- the older model with the 32-bit ARMv7-A architecture. It's the oldest, slowest, and coolest-running Raspberry Pi supported by Ubuntu, and it's also the closest match to the processors found in devices with e-paper displays. The Raspberry Pi should finally let me test changes like this to the Linux Monocle platform on which the EPD platform is based. I may eventually add a touchscreen, but I'm hoping to get by with just the mouse for now.

@openjdk openjdk bot removed the ready label Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.