drivers/ft5x06: fix initialization if callback function parameter is NULL #19880

gschorcht · 2023-08-12T10:27:29Z

Contribution description

This PR fixes the ft5x06 driver initialization if the callback function parameter cb is `NULL. This might be the case for example if the application uses the touch device in polling mode.

If the interrupt pin is initialized if the callback function parameter cb is NULL, the driver crashes the first time an interrupt is triggered. Therefore, the INT pin must be initialized only if also the callback function parameter cb is not NULL.

To be able to test the polling mode, this PR also includes a change of the tests/drivers/ft5x06 application which introduces the environment variables FT5X06_POLLING_MODE FT5X06_POLLING_PERIOD and in the makefile.

Testing procedure

Use any board with a FTXXXX touch device and test it in polling mode, for example:

FT5X06_POLLING_MODE=1 BOARD=stm32f746g-disco make -C tests/drivers/ft5x06 flash term

It should work as expected.

main(): This is RIOT! (Version: 2023.10-devel-119-g92a44a-drivers/ft5x06_fix_cb_null)
FT5x06 test application
+------------Initializing------------+
Initialization successful
1 touch detected
Touch 1 - X: 236, Y:111
Touch 1 - X: 236, Y:111
...
Touch 1 - X: 236, Y:111
Released!

Checkout master branch and cerry-pick commit 691a5e6. The test application tests/drivers/ft5x06 will crash once a touch event occur:
```
+------------Initializing------------+
Initialization successful
1 touch detected
Context before hardfault:
```

Issues/PRs references

riot-ci · 2023-08-12T10:43:04Z

Murdock results

✔️ PASSED

bf720c0 drivers/ft5x06: configure interrupt mode in any case

Success	Failures	Total	Runtime
7937	0	7937	14m:07s

Artifacts

Documentation preview

benpicco · 2023-08-23T22:43:14Z

bors merge

19880: drivers/ft5x06: fix initialization if callback function parameter is NULL r=benpicco a=gschorcht ### Contribution description This PR fixes the `ft5x06` driver initialization if the callback function parameter `cb` is `NULL. This might be the case for example if the application uses the touch device in polling mode. If the interrupt pin is initialized if the callback function parameter `cb` is `NULL`, the driver crashes the first time an interrupt is triggered. Therefore, the INT pin must be initialized only if also the callback function parameter `cb` is not `NULL`. To be able to test the polling mode, this PR also includes a change of the `tests/drivers/ft5x06` application which introduces the environment variables `FT5X06_POLLING_MODE` `FT5X06_POLLING_PERIOD` and in the makefile. ### Testing procedure 1. Use any board with a FTXXXX touch device and test it in polling mode, for example: ``` FT5X06_POLLING_MODE=1 BOARD=stm32f746g-disco make -C tests/drivers/ft5x06 flash term ``` It should work as expected. ``` main(): This is RIOT! (Version: 2023.10-devel-119-g92a44a-drivers/ft5x06_fix_cb_null) FT5x06 test application +------------Initializing------------+ Initialization successful 1 touch detected Touch 1 - X: 236, Y:111 Touch 1 - X: 236, Y:111 ... Touch 1 - X: 236, Y:111 Released! ``` 2. Checkout master branch and cerry-pick commit 691a5e6. The test application `tests/drivers/ft5x06` will crash once a touch event occur: ``` +------------Initializing------------+ Initialization successful 1 touch detected Context before hardfault: ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

bors · 2023-08-24T10:21:03Z

Build failed:

Murdock

benpicco · 2023-08-24T14:27:11Z

This needs a rebase

To be able to test the FT5x06 device driver in polling mode, variable `FT5X06_POLLING_MODE` is introduced. It is set to 0 by default and can be overriden by 1 to use the polling mode. The polling period can be controlled by the `FT5X06_POLLING_PERIOD` variable.

If the INT pin is initialized if the callback function parameter `cb` is `NULL`, the driver crashes the first time an interrupt is triggered. Therefore, the INT pin must be initialized only if also the callback function parameter `cb` is not `NULL`.

gschorcht · 2023-08-27T17:04:32Z

@benpicco I had to add a small change (commit 7acf801). The INT pin must not be initialized if cb is NULL. The interrupt trigger mode, however, should be configured independent on whether the INT pin is initialized by the ft5x06_init function or later, for example by the touch_dev implementation. Even if the INT pin isn't defined, it doesn't matter to configure the interrupt trigger mode.

It worked before without this change because the interrupt trigger mode is the default mode after reset. That is, we could also drop its configuration completely.

aabadie · 2023-08-28T07:09:48Z

Just tested this PR on stm32f723e-disco and I get a hard fault when enabling polling mode:

2023-08-28 09:07:15,847 # Initialization successful
2023-08-28 09:07:16,881 # Stack pointer corrupted, reset to top of stack
2023-08-28 09:07:16,881 # FSR/FAR:
2023-08-28 09:07:16,884 #  CFSR: 0x00020000
2023-08-28 09:07:16,885 #  HFSR: 0x40000000
2023-08-28 09:07:16,887 #  DFSR: 0x0000000b
2023-08-28 09:07:16,889 #  AFSR: 0x00000000
2023-08-28 09:07:16,889 # Misc
2023-08-28 09:07:16,891 # EXC_RET: 0xfffffff1
2023-08-28 09:07:16,892 # *** RIOT kernel panic:
2023-08-28 09:07:16,894 # HARD FAULT HANDLER
2023-08-28 09:07:16,894 # 
2023-08-28 09:07:16,895 # *** halted.
2023-08-28 09:07:16,896 # 
2023-08-28 09:07:16,897 # Inside isr -13
2023-08-28 09:07:18,009 # Exiting Pyterm

The interrupt mode still works though.

aabadie · 2023-08-28T07:11:35Z

Just tested this PR on stm32f723e-disco and I get a hard fault when enabling polling mode

If I revert 7acf801, polling mode works.

aabadie · 2023-08-28T07:14:29Z

I think the following diff makes more sense (instead of 7acf801):

diff --git a/drivers/ft5x06/ft5x06.c b/drivers/ft5x06/ft5x06.c
index d455e8b1b8..e13ee7dc4d 100644
--- a/drivers/ft5x06/ft5x06.c
+++ b/drivers/ft5x06/ft5x06.c
@@ -83,9 +83,10 @@ int ft5x06_init(ft5x06_t *dev, const ft5x06_params_t *params, ft5x06_event_cb_t
     if (gpio_is_valid(dev->params->int_pin) && cb) {
         DEBUG("[ft5x06] init: configuring touchscreen interrupt\n");
         gpio_init_int(dev->params->int_pin, GPIO_IN, GPIO_RISING, cb, arg);
-        i2c_write_reg(FT5X06_BUS, FT5X06_ADDR, FT5X06_G_MODE_REG, FT5X06_G_MODE_INTERRUPT_TRIGGER & 0x01, 0);
     }
 
+    i2c_write_reg(FT5X06_BUS, FT5X06_ADDR, FT5X06_G_MODE_REG, FT5X06_G_MODE_INTERRUPT_TRIGGER & 0x01, 0);
+
     i2c_release(FT5X06_BUS);
 
     return 0;

(keep the && cb in the if)
Because the interrupt pin might be defined by default in the board configuration, so skipping the interrupt pin initialization if it's not defined and cb is also not defined is better IMHO.

The interrupt mode has to be configured independent on whether the INT pin is initialized already by the `ft5x06_init` function if parameter `cb` is defined or later, for example by the `touch_dev` implementation.

gschorcht · 2023-08-28T16:51:10Z

(keep the && cb in the if) Because the interrupt pin might be defined by default in the board configuration, so skipping the interrupt pin initialization if it's not defined and cb is also not defined is better IMHO.

🙈 This was exactly what I intended by the last change. I shouldn't have changed the source on my mobile. Of course gpio_init must only be called if cb is not NULL. This was the initial intention of the fix in this PR.

I fixed it and squashed this small fix directly.

aabadie · 2023-08-28T19:31:09Z

bors merge

19880: drivers/ft5x06: fix initialization if callback function parameter is NULL r=aabadie a=gschorcht ### Contribution description This PR fixes the `ft5x06` driver initialization if the callback function parameter `cb` is `NULL. This might be the case for example if the application uses the touch device in polling mode. If the interrupt pin is initialized if the callback function parameter `cb` is `NULL`, the driver crashes the first time an interrupt is triggered. Therefore, the INT pin must be initialized only if also the callback function parameter `cb` is not `NULL`. To be able to test the polling mode, this PR also includes a change of the `tests/drivers/ft5x06` application which introduces the environment variables `FT5X06_POLLING_MODE` `FT5X06_POLLING_PERIOD` and in the makefile. ### Testing procedure 1. Use any board with a FTXXXX touch device and test it in polling mode, for example: ``` FT5X06_POLLING_MODE=1 BOARD=stm32f746g-disco make -C tests/drivers/ft5x06 flash term ``` It should work as expected. ``` main(): This is RIOT! (Version: 2023.10-devel-119-g92a44a-drivers/ft5x06_fix_cb_null) FT5x06 test application +------------Initializing------------+ Initialization successful 1 touch detected Touch 1 - X: 236, Y:111 Touch 1 - X: 236, Y:111 ... Touch 1 - X: 236, Y:111 Released! ``` 2. Checkout master branch and cerry-pick commit 691a5e6. The test application `tests/drivers/ft5x06` will crash once a touch event occur: ``` +------------Initializing------------+ Initialization successful 1 touch detected Context before hardfault: ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

bors · 2023-08-29T01:12:22Z

Build failed:

Murdock

aabadie · 2023-08-29T06:09:40Z

bors merge

19880: drivers/ft5x06: fix initialization if callback function parameter is NULL r=aabadie a=gschorcht ### Contribution description This PR fixes the `ft5x06` driver initialization if the callback function parameter `cb` is `NULL. This might be the case for example if the application uses the touch device in polling mode. If the interrupt pin is initialized if the callback function parameter `cb` is `NULL`, the driver crashes the first time an interrupt is triggered. Therefore, the INT pin must be initialized only if also the callback function parameter `cb` is not `NULL`. To be able to test the polling mode, this PR also includes a change of the `tests/drivers/ft5x06` application which introduces the environment variables `FT5X06_POLLING_MODE` `FT5X06_POLLING_PERIOD` and in the makefile. ### Testing procedure 1. Use any board with a FTXXXX touch device and test it in polling mode, for example: ``` FT5X06_POLLING_MODE=1 BOARD=stm32f746g-disco make -C tests/drivers/ft5x06 flash term ``` It should work as expected. ``` main(): This is RIOT! (Version: 2023.10-devel-119-g92a44a-drivers/ft5x06_fix_cb_null) FT5x06 test application +------------Initializing------------+ Initialization successful 1 touch detected Touch 1 - X: 236, Y:111 Touch 1 - X: 236, Y:111 ... Touch 1 - X: 236, Y:111 Released! ``` 2. Checkout master branch and cerry-pick commit 691a5e6. The test application `tests/drivers/ft5x06` will crash once a touch event occur: ``` +------------Initializing------------+ Initialization successful 1 touch detected Context before hardfault: ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

bors · 2023-08-29T08:14:17Z

Build failed:

Murdock

aabadie · 2023-08-29T08:19:36Z

bors merge

...

bors · 2023-08-29T11:51:26Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

gschorcht · 2023-08-29T15:36:06Z

Thanks for reviewing and merging 😄

19882: tests/drivers/touch_dev: allow to test a touch device in polling mode r=aabadie a=gschorcht ### Contribution description To be able to test the touch device in polling mode, variable `TOUCH_DEV_POLLING` is introduced. It is set to 0 by default and can be overriden by 1 to use the polling mode. The polling period can be controlled by the `TOUCH_DEV_POLLING_PERIOD` variable. To use it for `ft5x06` and `stmpe811` drivers, the polling mode has to be fixed for these touch device drivers (PR #19880 respective PR #19881). ### Testing procedure Use any board with touch device driver that uses the generic touch device API, for example (PR #19881 is required) ``` TOUCH_DEV_POLLING_MODE=1 TOUCH_DEV_POLLING_PERIOD=100 \ BOARD=stm32f429i-disc1 make -C tests/drivers/touch_dev flash term ``` or (PR #19880 is required) ``` TOUCH_DEV_POLLING_MODE=1 TOUCH_DEV_POLLING_PERIOD=100 \ BOARD=stm32f746g-disco make -C tests/drivers/touch_dev flash term ``` ### Issues/PRs references Depends partially on PR #19880 Depends partially on PR #19881 Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

gschorcht requested review from smlng, leandrolanzieri, aabadie, MichelRottleuthner and fjmolinas as code owners August 12, 2023 10:27

github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: drivers Area: Device drivers labels Aug 12, 2023

gschorcht added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 12, 2023

gschorcht mentioned this pull request Aug 12, 2023

tests/drivers/touch_dev: allow to test a touch device in polling mode #19882

Merged

gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 12, 2023

benpicco approved these changes Aug 23, 2023

View reviewed changes

gschorcht added 2 commits August 25, 2023 17:32

gschorcht force-pushed the drivers/ft5x06_fix_cb_null branch from 0b72051 to 7acf801 Compare August 27, 2023 16:55

drivers/ft5x06: configure interrupt mode in any case

bf720c0

The interrupt mode has to be configured independent on whether the INT pin is initialized already by the `ft5x06_init` function if parameter `cb` is defined or later, for example by the `touch_dev` implementation.

gschorcht force-pushed the drivers/ft5x06_fix_cb_null branch from e15615c to bf720c0 Compare August 28, 2023 16:50

aabadie removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 28, 2023

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 28, 2023

bors bot merged commit 5cf3200 into RIOT-OS:master Aug 29, 2023

gschorcht deleted the drivers/ft5x06_fix_cb_null branch August 29, 2023 15:37

MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023

drivers/ft5x06: fix initialization if callback function parameter is NULL #19880

drivers/ft5x06: fix initialization if callback function parameter is NULL #19880

Uh oh!

Conversation

gschorcht commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

riot-ci commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

benpicco commented Aug 23, 2023

Uh oh!

bors bot commented Aug 24, 2023

Uh oh!

benpicco commented Aug 24, 2023

Uh oh!

gschorcht commented Aug 27, 2023

Uh oh!

aabadie commented Aug 28, 2023

Uh oh!

aabadie commented Aug 28, 2023

Uh oh!

aabadie commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gschorcht commented Aug 28, 2023

Uh oh!

aabadie commented Aug 28, 2023

Uh oh!

bors bot commented Aug 29, 2023

Uh oh!

aabadie commented Aug 29, 2023

Uh oh!

bors bot commented Aug 29, 2023

Uh oh!

aabadie commented Aug 29, 2023

Uh oh!

bors bot commented Aug 29, 2023

Uh oh!

gschorcht commented Aug 29, 2023

Uh oh!

Uh oh!

gschorcht commented Aug 12, 2023 •

edited

Loading

riot-ci commented Aug 12, 2023 •

edited

Loading

aabadie commented Aug 28, 2023 •

edited

Loading