os/drivers/input/: Add callback functionality to notify touch interrupt to application for touch driver ist415, touch HAL layer #6539

abhinav-s235 · 2024-11-26T14:04:03Z

Touch Driver provides a mechanism to notify application whenever a touch event occur.
Register a callback function with touch input driver using TSIOC_SETAPPNOTIFY IOCTL Call. THis function will be automatically called whenever a touch event(Press ,Hold/Move, Release) is detected.
When a touch event occurs , the registered callback function is triggered by the touch interrupt handler function. The driver provides details such as touch coordinate and type directly to this callback. Whenever there is a touch interrupt a new task is created to get the touch data and it eventually call the callback function.

anjana348 · 2024-11-27T04:21:14Z

apps/examples/lcd_test/example_lcd.c

+	int ret = ioctl(touch_fd, TSIOC_SETAPPNOTIFY, &touch_set_callback);
+	if (ret != OK) {
+		printf("ERROR: Touch callback register from application to touch driver failed\n");
+		return;


close(touch_fd) before return

edwakuwaku · 2024-11-27T13:17:22Z

apps/examples/lcd_test/example_lcd.c

+
+#elif CONFIG_TOUCH_CALLBACK
+
+	touch_set_callback.touch_points = (struct touch_sample_s *)malloc(sizeof(struct touch_sample_s));


Should free it when task end or operation below failed

namanjain7 · 2024-11-28T10:35:11Z

os/include/tinyara/input/touchscreen.h

@@ -85,7 +85,9 @@
 #define TSIOC_GETFREQUENCY   _TSIOC(0x0004)  /* arg: Pointer to uint32_t frequency value */
 #define TSIOC_DISABLE        _TSIOC(0x0005)  /* Disable touch interrupt */
 #define TSIOC_ENABLE         _TSIOC(0x0006)  /* Enable touch interrupt */
-
+#ifdef CONFIG_TOUCH_CALLBACK
+#define TSIOC_SETAPPNOTIFY	 _TSIOC(0x0008)  /* arg: Pointer to struct touch_set_callback_s */


address will be 0x0007

namanjain7 · 2024-11-28T10:39:21Z

apps/examples/lcd_test/example_lcd.c

@@ -446,8 +510,15 @@ int lcd_test_main(int argc, char *argv[])
 	}
 	test_fps();
 	close(fd);
+errout:


it will not always errout case
this line could execute in OK case also.
change the name errout

namanjain7 · 2024-11-28T10:41:19Z

apps/examples/lcd_test/example_lcd.c

+		free(touch_set_callback.touch_points);
+		return;
+	}
+


remove all extra lines in # ifdefines

namanjain7 · 2024-11-28T10:41:46Z

apps/examples/lcd_test/example_lcd.c

@@ -370,6 +374,9 @@ static void test_fps(void)
 }

 #ifdef CONFIG_TOUCH
+


remove the extra line

namanjain7 · 2024-11-28T10:41:54Z

apps/examples/lcd_test/example_lcd.c

@@ -370,6 +374,9 @@ static void test_fps(void)
 }

 #ifdef CONFIG_TOUCH
+
+#ifdef CONFIG_TOUCH_POLL
+


remove the extra line

sunghan-chang · 2024-11-28T10:34:07Z

apps/examples/lcd_test/example_lcd.c

@@ -85,6 +85,10 @@ static int yres;
 #define EXAMPLE_LCD_FPS_TEST 5000
 #endif

+#if defined(CONFIG_TOUCH) && defined(CONFIG_TOUCH_CALLBACK) 


You can keep defined(CONFIG_TOUCH_CALLBACK) only. Because it can be enabled only when the CONFIG_TOUCH is enabled.

sunghan-chang · 2024-11-28T10:34:59Z

apps/examples/lcd_test/example_lcd.c

@@ -408,6 +415,37 @@ static void touch_test(void)
 	}
 	close(fd);
 }
+
+#elif CONFIG_TOUCH_CALLBACK


This should be #elif defined(CONFIG_...). This is wrong usage of the config.

sunghan-chang · 2024-11-28T10:36:14Z

apps/examples/lcd_test/example_lcd.c

+	touch_set_callback.touch_points = (struct touch_sample_s *)malloc(sizeof(struct touch_sample_s));
+	if (!touch_set_callback.touch_points) {
+		printf("Error: Touch point buffer memory allocation failed\n");
+		return;


This function return type is int, not void. So you should return -1 or something else.
Same as other returns in the function.

sunghan-chang · 2024-11-28T10:38:19Z

os/drivers/input/Kconfig

+	prompt "choose TOUCH DEVICE app notification type"
+	default TOUCH_POLL
+	---help---
+		Polling based notification to application


This help sentence is for TOUCH_POLL, not for the choice.

sunghan-chang · 2024-11-28T10:40:19Z

os/drivers/input/ist415.c

@@ -135,6 +147,12 @@ static int ist415_get_touch_data(struct ist415_dev_s *dev, FAR void *buf)
 	}
 	touchvdbg("touch_point %d\n", touch_point + 1);

+#ifdef CONFIG_TOUCH_CALLBACK


line 156 is not in this conditional. Do we need to add this conditional here only?
This new code looks not releated with callback config.

In case of TOUCH_POLL we have handle buffer NULL condition inside function touch read()
but in case of TOUCH_CALLBACK this function is getting directly called from here (

TizenRT/os/drivers/input/ist415.c

Line 272 in 54895ab

int ret = ist415_get_touch_data(priv, upper->app_touch_point_buffer);

). So we have to explicitly handle this condition.

The touch_read does not check whether the buffer is null or not.

What you linked is https://github.com/Samsung/TizenRT/blob/e5d7ef9b8629882448210d8615255e70fc591db4/os/drivers/input/ist415.c#L259C13-L259C27. This is for callback, not poll case.

Could you check it again?
And even it is really checking like that, I suggest to do the same thing at the same location.
For the same purpose, if we do different location for each, it's not good to maintain.

This is the platform code, it should be easy-maintain and common.

@anjana348 I left the same comment on @namanjain7 's PR. Let's concern this point for all.
Same purpose, same location regardless of the config difference as possible.

It has been fixed.
Now buffer NULL condition is getting checked at same location for both the cases.

sunghan-chang · 2024-11-28T10:47:47Z

os/drivers/input/ist415.c

+static void get_touch_data(struct ist415_dev_s *priv)
+{
+	FAR struct touchscreen_s *upper = priv->upper;
+
+	struct touch_sample_s touch_points;
+
+	if (!upper->app_touch_point_buffer) {
+		touchdbg("ERROR: application buffer touch data is NULL\n");
+		if (upper->is_touch_detected) {
+			upper->is_touch_detected(-EINVAL);
+			return;
+		}
+	}
+	int ret = ist415_get_touch_data(priv, upper->app_touch_point_buffer);
+
+	if (upper->is_touch_detected) {
+		upper->is_touch_detected(ret);
+	}
+}


Sorry I don't know how to give the data to app. Could you explain working flow?

We are registering callback function is_touch_detected and buffer to driver using IOCTL call TSIOC_SETAPPNOTIFY
When there is an touch interrupt a new thread will be created which will execute function get_touch_data() and this function will call function ist415_get_touch_data() by passing buffer as a parameter(this buffer was register to driver using IOCTL call).
Function ist415_get_touch_data() copy data related to touch inside the passed buffer and after completion of this function the callback function will be called from here
Now callback fuction use the buffer(which was passed to driver) to acces the touch data.

sunghan-chang · 2024-11-28T10:48:21Z

os/include/tinyara/input/touchscreen.h

@@ -85,7 +85,9 @@
 #define TSIOC_GETFREQUENCY   _TSIOC(0x0004)  /* arg: Pointer to uint32_t frequency value */
 #define TSIOC_DISABLE        _TSIOC(0x0005)  /* Disable touch interrupt */
 #define TSIOC_ENABLE         _TSIOC(0x0006)  /* Enable touch interrupt */
-
+#ifdef CONFIG_TOUCH_CALLBACK


Let's add this without conditional.

I didn't understood the query.
Could you please elaborate it more

Let's remove #ifdef CONFIG_TOUCH_CALLBACK and #endif.
Just add new definition without #ifdef.

…pt to application for touch driver ist415, touch HAL layer Register a callback function with touch input driver using TSIOC_SETAPPNOTIFY IOCTL Call. This function will be automatically called whenever a touch event(Press ,Hold/Move, Release) is detected. When a touch event occurs , the registered callback function is triggered by the touch interrupt handler function. The driver provides details such as touch coordinate and type directly to this callback. Whenever there is a touch interrupt a new task is created to get the touch data and it eventually call the callback function.

…ctionality Touch hold/move event Total touch points 3 TOUCH COORDINATES id: 0, x : 313 y : 162 touch type: 2 Touch hold/move event TOUCH COORDINATES id: 1, x : 398 y : 282 touch type: 2 Touch hold/move event TOUCH COORDINATES id: 2, x : 561 y : 321 touch type: 2 Touch hold/move event Total touch points 3 TOUCH COORDINATES id: 0, x : 313 y : 162 touch type: 2 Touch hold/move event

namanjain7 mentioned this pull request Nov 26, 2024

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

Closed

anjana348 reviewed Nov 27, 2024

View reviewed changes

abhinav-s235 force-pushed the callback_1 branch from 882d68b to 07a1610 Compare November 27, 2024 04:52

edwakuwaku reviewed Nov 27, 2024

View reviewed changes

abhinav-s235 marked this pull request as draft November 28, 2024 04:58

abhinav-s235 force-pushed the callback_1 branch from 07a1610 to 7813c5f Compare November 28, 2024 10:21

namanjain7 reviewed Nov 28, 2024

View reviewed changes

sunghan-chang reviewed Nov 28, 2024

View reviewed changes

abhinav-s235 force-pushed the callback_1 branch 3 times, most recently from e5d7ef9 to 9dacc47 Compare November 29, 2024 10:59

abhinav-s235 marked this pull request as ready for review December 4, 2024 09:18

abhinav-s235 added 2 commits December 4, 2024 15:18

abhinav-s235 force-pushed the callback_1 branch from 9dacc47 to e528e6c Compare December 4, 2024 09:49

sunghan-chang approved these changes Dec 5, 2024

View reviewed changes

sunghan-chang merged commit f40de0b into Samsung:master Dec 5, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os/drivers/input/: Add callback functionality to notify touch interrupt to application for touch driver ist415, touch HAL layer #6539

os/drivers/input/: Add callback functionality to notify touch interrupt to application for touch driver ist415, touch HAL layer #6539

abhinav-s235 commented Nov 26, 2024

anjana348 Nov 27, 2024

abhinav-s235 Nov 27, 2024

edwakuwaku Nov 27, 2024

namanjain7 Nov 28, 2024

abhinav-s235 Dec 4, 2024

namanjain7 Nov 28, 2024

abhinav-s235 Dec 4, 2024

namanjain7 Nov 28, 2024

abhinav-s235 Dec 4, 2024

namanjain7 Nov 28, 2024

namanjain7 Nov 28, 2024

abhinav-s235 Dec 4, 2024

sunghan-chang Nov 28, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 28, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 28, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 28, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 28, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 29, 2024

sunghan-chang Nov 29, 2024

sunghan-chang Nov 29, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 28, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 28, 2024

abhinav-s235 Nov 29, 2024

sunghan-chang Nov 29, 2024

abhinav-s235 Nov 29, 2024


		#elif CONFIG_TOUCH_CALLBACK

		touch_set_callback.touch_points = (struct touch_sample_s *)malloc(sizeof(struct touch_sample_s));

		@@ -370,6 +374,9 @@ static void test_fps(void)
		}

		#ifdef CONFIG_TOUCH

os/drivers/input/: Add callback functionality to notify touch interrupt to application for touch driver ist415, touch HAL layer #6539

os/drivers/input/: Add callback functionality to notify touch interrupt to application for touch driver ist415, touch HAL layer #6539

Conversation

abhinav-s235 commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment