os/: IST415 touch driver with multitouch support and respective test case #6404

namanjain7 · 2024-09-18T15:35:26Z

Please review and merge.

os/drivers/input/ist415.c

os/drivers/input/touch.c

os/include/tinyara/input/ist415.h

apps/examples/lcd_test/example_lcd.c

os/board/rtl8730e/src/rtl8730e_ist415.c

os/drivers/input/touch.c

ewoodev · 2024-09-23T01:55:37Z

os/include/tinyara/input/ist415.h

@@ -0,0 +1,315 @@
+/****************************************************************************


I think, this file can be in os/drives/input.

ewoodev · 2024-09-23T01:56:29Z

os/include/tinyara/input/ist415.h

+
+	u32 CalResult;
+	u32 SCalResult;
+}stIST415xInfo_t;


let's add space.
And this struct is necessary?

we have taken this from tizen applied code. yes, It's not necessary, so I have removed it.

ewoodev · 2024-09-24T22:38:35Z

os/board/rtl8730e/src/rtl8730e_ist415.c

+	FAR struct i2c_dev_s *i2c;
+
+	rtl8730e_ist415_gpio_init();
+	rtl8730e_ist415_gpio_reset(false);


To properly perform a reset TSP here, you need to set the GPIO pin low and then set it high.

ewoodev · 2024-09-24T22:48:49Z

os/drivers/input/touch.c

+	DEBUGASSERT(buffer);
+	DEBUGASSERT(buflen > 0);


This is the wrong argument used, So we don't need to reboot the whole system.

How about change this to return minus errno number?

ewoodev · 2024-09-24T22:51:13Z

os/drivers/input/ist415.c

+ * Private Functions
+ ****************************************************************************/
+
+static int ist415_get_touch_data (struct ist415_dev_s *dev, FAR void *buf)


Please remove space

static int ist415_get_touch_data(struct ist415_dev_s *dev, FAR void *buf)

ewoodev · 2024-09-24T23:11:08Z

os/drivers/input/touch.c

+	}
+	sem_init(&g_dev->sem, 0, 1);
+	sem_init(&g_dev->pollsem, 0, 1);
+	g_dev->dev = dev;


How about following the to the upper and lower parts like timer?

FAR struct timer_upperhalf_s *upper;

upper->lower = lower;
lower->upper = upper; or lower->parent = upper;

upper is defined in drivers/.c and lower is defined in tinyara/.h, so how can lower use upper

@ewoodev Is this enough? If yes, please mark it as resolved conversation.

ewoodev · 2024-09-24T23:14:05Z

os/drivers/input/touch.c

+{
+	int itr;
+	for (itr = 0; itr < CONFIG_TOUCH_NPOLLWAITERS; itr++) {
+		struct pollfd *fds = g_dev->fds[itr];


you can access priv upper layer as below, let's remove global variable

int touch_register(struct touch_dev_s *dev) { dev->upper= upper; }

@namanjain7 @ewoodev How is this?

ewoodev · 2024-09-24T23:23:23Z

os/drivers/input/ist415.c

+	return OK;
+}
+
+static void touch_interrupt(int d)


do we need argument here?

I have removed the argument.

ewoodev · 2024-09-24T23:33:12Z

os/board/rtl8730e/src/rtl8730e_ist415.c

+	int ret = 0;
+	uint8_t reg[1];
+	reg[0] = 0x23;
+	ret = i2c_write(i2c, &(g_ist415info.touch_config.i2c_config), (uint8_t *)reg, 1);
+	if (ret < OK) {
+		touchdbg("ERROR: i2c_write failed\n");
+	}


This does not seem to be a board-specific code.
Please move to ist415.c

one write is necessary for working. without this, touch won't work.
I have moved other write and read to driver.

sunghan-chang

Let's split a commit for app.

add ist415 touch support
modify lcd test app for touch

Taejun-Kwon · 2024-10-02T10:58:35Z

os/drivers/input/touch.c

+
+	/* Read the touch data, only if screen has been touched or if we're waiting for touch up */
+	outlen = sizeof(struct touch_sample_s);
+	ret = priv->dev->touch_read(priv->dev, buffer);


You should use ioctl instead of read & write.
If another device should use some specific structure to bring data, then write & read will not bring data.
That's why all character device use ioctl instead of read / write

Taejun-Kwon · 2024-10-02T11:07:20Z

os/drivers/input/touch.c

+ * Name: touch_pollnotify
+ ****************************************************************************/
+
+void touch_pollnotify(FAR struct touch_dev_s *priv, pollevent_t eventset)


'priv' means private data of iNode. You register dev structure to private area of iNode. so dev is proper, instead of priv.

Taejun-Kwon · 2024-10-02T11:09:31Z

os/drivers/input/touch.c

+{
+	int itr;
+	for (itr = 0; itr < CONFIG_TOUCH_NPOLLWAITERS; itr++) {
+		struct pollfd *fds = g_dev->fds[itr];


g_dev allocated when driver registered. then I think you can add this to dev structure. INode will hold this structure too.

Taejun-Kwon · 2024-10-02T11:11:23Z

os/drivers/input/touch.c

+ * Name: touch_poll
+ ****************************************************************************/
+
+static int touch_poll(FAR struct file *filep, FAR struct pollfd *fds, bool setup)


ist415 doesn't support interrupt mode??

when there is interrupt in touch, it calls touch_notify.
Then apps calls poll in fd to get touch data.

ewoodev · 2024-10-04T06:24:16Z

os/drivers/input/touchscreen.c

+	if (sem_wait(sem) < 0) {
+		/* EINTR is the only error that we expect */
+		int errcode = get_errno();
+		DEBUGASSERT(errcode == EINTR);
+		if (errout) {
+			return errcode;
+		}
+	}


Suggested change

if (sem_wait(sem) < 0) {

/* EINTR is the only error that we expect */

int errcode = get_errno();

DEBUGASSERT(errcode == EINTR);

if (errout) {

return errcode;

}

}

while (sem_wait(sem) != OK) {

ASSERT(get_errno() == EINTR);

if (errout) {

return -EINTR;

}

}

sem_wait can be woken up by other signal. this case should be call sem_wait again

Taejun-Kwon · 2024-10-04T07:16:50Z

os/drivers/input/touchscreen.c

+ * Name: touch_read
+ ****************************************************************************/
+
+static ssize_t touch_read(FAR struct file *filep, FAR char *buffer, size_t buflen)


as per my previous reveiew, it should be moved to ioctl.
Not all device just return touched data, some other process before collect touch data can be required.
Please do not use read/write to collect or send data, if possible. It's not a good way.

I think touch_read and touch_write are fine because all devices require those.
We can add a separate ioctl which can be used to set configuration/setting before calling touch_read/touch_write in future devices.
@ewoodev should we add a separate ioctl or should we remove the touch_read/touch_write and only use ioctl?

No, some arguments or pre process can be added. That's why most of character device provide ioctl to send or receive data from application.
Also Tizen Lite follow posix, in POSIX, character device return private data on iNode when read requested.

namanjain7 · 2024-10-07T04:44:20Z

Hello Mr. Nam @ewoodev
Should I remove touch_read and touch_write and use ioctl only
OR
should I add a separate ioctl which can be used to set configuration/setting before calling touch_read/touch_write in future devices?

pls suggest.

sunghan-chang · 2024-10-10T09:07:57Z

os/board/rtl8730e/src/rtl8730e_ist415.c

+
+#define PIN_LOW		0
+#define PIN_HIGH	1
+#define TOUCH_DEV_PATH "/dev/touch0"


https://github.com/Samsung/TizenRT/pull/6404/files#diff-cfd76cbd748bd8386f6325ede497404efbb0c31ee5f70afffe199948445a80a6R90

Let's use the same definition.

sunghan-chang · 2024-10-10T09:08:22Z

os/include/tinyara/input/touchscreen.h

@@ -69,7 +69,8 @@
 #include <tinyara/config.h>


Let's keep 644 for file permission.

sunghan-chang · 2024-10-10T09:09:25Z

os/include/tinyara/input/touchscreen.h

@@ -121,7 +124,7 @@
 #define TOUCH_POS_VALID      (1 << 4) /* Hardware provided a valid X/Y position */
 #define TOUCH_PRESSURE_VALID (1 << 5) /* Hardware provided a valid pressure */
 #define TOUCH_SIZE_VALID     (1 << 6) /* Hardware provided a valid H/W contact size */
-
+#define TOUCH_MAX_POINTS 	15    /* Maximum number of simultaneous touch point supported */


Is this ok to configure in SW? In other words, isn't it a hw dependency configuration?
As you made, here is no hw dependency code.

It's main purpose is for buffer allocation where touch data will be written.
if there is a hardware which has support for less fingers say 10. so, it will detect at max 10 fingers and data will be written for at max 10 indexes. rest 5 index will be empty.
I have added the ifdef config CONFIG_TOUCH_IST415 similar to what we have done in mipi.
so, different touch hardware can define their own TOUCH_MAX_POINTS

sunghan-chang · 2024-10-10T09:13:10Z

os/include/tinyara/input/touchscreen.h

 #include <tinyara/fs/ioctl.h>

-#ifdef CONFIG_INPUT
+#ifdef CONFIG_TOUCH
+#include <tinyara/i2c.h>


This looks strange.

Why do you put i2c.h under CONFIG_TOUCH even you locate ioctl.h at outside of the conditional? I think both should be in the conditional.

Even this header is touchscreen.h, do we need to cover the code with the conditional?

I have added the ioctl inside the condition.

Taejun-Kwon · 2024-10-10T11:42:31Z

os/drivers/input/ist415.c

+			touchdbg("ERROR: I2C write failed\n");
+			return ret;
+		}
+		ret = i2c_read(i2c, &config, touch_event + EVENT_PACKET_SIZE, touch_point * EVENT_PACKET_SIZE);


it always give data when host request touch data to ist415? there is no some kind of state reg value?
As per my experience, there's some bits to check state of measure position.

Taejun-Kwon · 2024-10-10T12:12:08Z

os/board/rtl8730e/src/rtl8730e_ist415.c

+		dev =  &g_ist415_dev0;
+	}
+	struct rtl8730e_ist415_s *priv = dev->priv;
+	gpio_irq_disable((gpio_irq_t *)&priv->data_ready);


I am not sure but as I know, it should not disable entire of interrupt.

if (touh_down_mode()) { diable_interrupt; do_something; enable_interrupt; }

Because 'up' event happened only once, so it is required to prevent lose up event during process touch.

Taejun-Kwon · 2024-10-10T12:15:36Z

os/drivers/input/touchscreen.c

+
+static int touch_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
+{
+	return OK;


I think there should be suspend mode or something to handle sleep mode.
It is depend on requirement of product, but I think ioctl should provide some interface to handle suspend & wakeup to prevent touch interrupt during sleep mode.

Hi Mr. kwon,
I have added a ioctl support for sleep mode and wake up mode.
the interrupts will be disabled in case of sleep mode and interrupts will be enabled in case of wake up mode.

yes, i think so, it could be needed, but this PR is opening long ago. Let's seperate PR about this.

namanjain7 · 2024-10-14T11:03:45Z

Hi,
I have added the support for callback notification to UI.
Now UI can choose what notification they want to use.
They case use polling method as earlier, or they can use callback which they can register using ioctl.
Please refer example_lcd.c file to see usage.

Add Touch device driver in rtl8730e board and HAL touch common layer for application The structure of touch buffer is: struct touch_sample_s { int npoints; /* The number of touch points in point[] / struct touch_point_s point[1]; / Actual dimension is npoints / struct touch_point_s point[TOUCH_MAX_POINTS]; / Actual dimension is npoints */ }; struct touch_point_s { uint8_t id; /* Unique identifies contact; Same in all reports for the contact / uint8_t flags; / See TOUCH_* definitions above / int16_t x; / X coordinate of the touch point (uncalibrated) / int16_t y; / Y coordinate of the touch point (uncalibrated) / int16_t h; / Height of touch point (uncalibrated) / int16_t w; / Width of touch point (uncalibrated) / uint16_t pressure; / Touch pressure */ }; We have added the array for number of consecutive fingers touching the screen. you can also refer the example app for usage apps/examples/lcd_test/example_lcd.c function: static void touch_test(void) TASH>>lcd_test TASH>>=== LCD demo === xres : 480, yres:800 Total touch points 3 coordinates id: 0, x : 190 y : 135 touch type: 2 Touch hold/move event coordinates id: 1, x : 309 y : 532 touch type: 1 Touch press event coordinates id: 2, x : 161 y : 304 touch type: 1 Touch press event Total touch points 4 coordinates id: 0, x : 187 y : 135 touch type: 2 Touch hold/move event coordinates id: 1, x : 309 y : 532 touch type: 2 Touch hold/move event coordinates id: 2, x : 161 y : 304 touch type: 2 Touch hold/move event coordinates id: 3, x : 210 y : 431 touch type: 2 Touch hold/move event

Add Touch Device test for both single touch and multitouch. It shows the coordinates along with Touch id (Number of simultaneous touch)

namanjain7 · 2024-10-21T04:44:12Z

Hi,
Since we don't have the revision 6 or 7 board, we are not able to test this pr in loadable_ext_ddr defconfig although I have added the changes in this defconfig also.
I have tested in audio defconfig in rev 5 board by manually adding the LCD and Touch configs.

ewoodev · 2024-10-22T13:06:32Z

os/drivers/input/ist415.c

+{
+	FAR struct touchscreen_s *upper = priv->upper;
+	priv->ops->irq_enable();
+	work_queue(HPWORK, &ist415_work, get_touch_data, priv, 0);


This is kernel side work queue.
if app is dead, work_queue operation might corrupted.
Could you please find other way to notify to application?

ewoodev · 2024-11-19T06:24:04Z

Let's seperate below to another new PR

related to CONFIG_TOUCH_CALLBACK
related to TSIOC_SUSPEND and TSIOC_SUSPEND

sunghan-chang · 2024-11-26T10:30:25Z

@namanjain7 @anjana348 Could you check all changes in this PR were merged with other PRs? Can we close this?

sunghan-chang · 2024-11-26T10:30:50Z

@namanjain7 @anjana348 What about touch callback functionality?

namanjain7 · 2024-11-26T15:41:54Z

Touch callback PR: #6539

sunghan-chang · 2024-11-28T08:17:21Z

Touch callback PR: #6539

When we merge the PR #6539 , all are merged. Right? Then, how about closing this PR?

namanjain7 · 2024-11-28T08:19:43Z

Touch callback PR: #6539

When we merge the PR #6539 , all are merged. Right? Then, how about closing this PR?

yes, I will close this PR once #6539 is merged.