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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ft5336 touch panel driver and integrate into lvgl #22055

Merged
merged 6 commits into from
Feb 1, 2020

Conversation

MaureenHelm
Copy link
Member

This is another attempt to enable the touch panel driver on i.mx rt boards and integrate into LittlevGL. Unlike #16119 which uses the experimental zio interface in the topic-sensors branch, this version uses the existing kscan interface already in master.

One small change was required in the kscan api, extending row and column callback arguments from 8-bits to 32-bits.

};

static K_MEM_SLAB_DEFINE(kscan_slab, sizeof(struct kscan_fifo_data), 10, 4);
static K_FIFO_DEFINE(kscan_fifo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using a k_msgq here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason, thanks for the suggestion. i've updated the pr to use a msgq.

Copy link
Member

@dleach02 dleach02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. The API change is fairly safe.

Extends the keyboard scan callback row and column arguments from 8-bits
to 32-bits to support a touch panel driver implementation.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
Adds device tree bindings for the focaltech ft5336 touch panel
controller, which will be used on several i.mx rt evk boards.

Moves address-cells and size-cells properties from the base kscan
bindings to the specific microchip,xec bindings since they are not
required for the ft5336.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
Introduces a new ft5336 touch panel driver for the keyboard scan (kscan)
interface. The driver currently uses a timer to periodically poll touch
data in the system work queue, but later it can be enhanced to use a
gpio interrupt instead of a timer.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
@MaureenHelm
Copy link
Member Author

rebased to pick up #22056

Copy link
Contributor

@franciscomunoz franciscomunoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@vanwinkeljan vanwinkeljan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good stop-gap solution until we have a real touch screen API, but we should make clear in the Kconfig that kscan is used for a pointer like device and not a keyboard.

help
Enable keyboard scan input

config LVGL_KSCAN_DEV_NAME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if LVGL_KSCAN_DEV_NAME should be renamed to LVGL_POINTER_KSCAN_DEV_NAME to make it clear that it is a pointer like input device and not a real keyboard.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


config LVGL_KSCAN_DEV_NAME
string "Keyboard scan input device name"
depends on LVGL_KSCAN
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put LVGL_KSCAN_DEV_NAME and LVGL_KSCAN_MSGQ_COUNT between if LVGL_KSCAN instead of using depends on LVGL_KSCAN

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

#ifdef CONFIG_LVGL_KSCAN
K_MSGQ_DEFINE(kscan_msgq, sizeof(lv_indev_data_t),
CONFIG_LVGL_KSCAN_MSGQ_COUNT, 4);
static lv_indev_drv_t indev_drv;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to make this global variable, it can just be a local variable in lvgl_kscan_init as lv_indev_drv_register makes a copy of the variable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Adds support for an optional lvgl touch input device using the zephyr
keyboard scan interface. This can be used with the ft5336 touch panel
driver, which returns single touch coordinates through the kscan
driver callback.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
# Copyright (c) 2020 NXP
# SPDX-License-Identifier: Apache-2.0

CONFIG_I2C=y
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This board config files are a maintenance overhead sometimes. Why not enable it in soc/arm/nxp_imx/rt/Kconfig.defconfig.series ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't implement it originally because I was worried about complex Kconfig dependencies, but given your feedback I went ahead and did it now. Please have a look at the updated version.

@@ -30,7 +31,10 @@ void main(void)
return;
}

hello_world_label = lv_label_create(lv_scr_act(), NULL);
hello_world_button = lv_btn_create(lv_scr_act(), NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me test it on different displays, probably button could be conditional.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The button does not look good on monochrome displays because of the background gradient, I suggest to use the button only if KSCAN is enabled, e.g.:

diff --git a/samples/display/lvgl/src/main.c b/samples/display/lvgl/src/main.c
index c40808258d..c566b52922 100644
--- a/samples/display/lvgl/src/main.c
+++ b/samples/display/lvgl/src/main.c
@@ -31,10 +31,14 @@ void main(void)
                return;
        }
 
-       hello_world_button = lv_btn_create(lv_scr_act(), NULL);
-       lv_obj_align(hello_world_button, NULL, LV_ALIGN_CENTER, 0, 0);
+       if (IS_ENABLED(CONFIG_KSCAN)) {
+               hello_world_button = lv_btn_create(lv_scr_act(), NULL);
+               lv_obj_align(hello_world_button, NULL, LV_ALIGN_CENTER, 0, 0);
+               hello_world_label = lv_label_create(hello_world_button, NULL);
+       } else {
+               hello_world_label = lv_label_create(lv_scr_act(), NULL);
+       }
 
-       hello_world_label = lv_label_create(hello_world_button, NULL);
        lv_label_set_text(hello_world_label, "Hello world!");
        lv_obj_align(hello_world_label, NULL, LV_ALIGN_CENTER, 0, 0);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit awkward to conditionalize the sample like this. What if we instead configured monochrome displays to use the mono theme by default?

Copy link
Collaborator

@jfischer-no jfischer-no Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit awkward to conditionalize the sample like this.

no preprocessor macros are awkward, if (IS_ENABLED(CONFIG_OPTION)) { is fine.

What if we instead configured monochrome displays to use the mono theme by default?

yes, an option. But it looks anyway ugly on small displays, the button size does not scale, probably lv_btn_set_fit would help.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lv_btn_set_fit(hello_world_button, LV_FIT_TIGHT); did not work well, the button is still too big for a 128x64 pixel display.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fond of making the button conditional in a simple "hello world" type of example, but I don't have a 128x64 pixel display to play with making the button fit. I updated the patch to your suggestion.

Adds device tree nodes and configures Kconfig defaults for the ft5336
touch panel driver on mimxrt10{50,60,64}_evk boards.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
Enhances the lvgl sample to support an optional touch panel input on
mimxrt10{50,60,64}_evk boards.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
@jfischer-no jfischer-no added this to the v2.2.0 milestone Jan 31, 2020
@nashif nashif merged commit 06d17aa into zephyrproject-rtos:master Feb 1, 2020
@MaureenHelm MaureenHelm deleted the kscan-touch branch February 3, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Boards area: Devicetree area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: NXP NXP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants