drivers: rename st7735 to more generic st77xx#19825
drivers: rename st7735 to more generic st77xx#19825bors[bot] merged 13 commits intoRIOT-OS:masterfrom
Conversation
gschorcht
requested review from
MichelRottleuthner,
MrKevinWeiss,
aabadie,
fjmolinas,
jia200x,
leandrolanzieri and
smlng
as code owners
github-actions
bot
added
Area: doc
gschorcht
added
Type: bug
gschorcht
force-pushed
the
drivers/st77xx_generic
branch
2 times, most recently
from
bca322e to
0aac967
Compare
| ifneq (,$(filter disp_dev,$(USEMODULE))) | ||
| USEMODULE += st77xx | ||
| endif | ||
|
|
||
| ifneq (,$(filter st77xx,$(USEMODULE))) | ||
| # indicate that a ST7735 is used | ||
| USEMODULE += st7735 | ||
| endif |
There was a problem hiding this comment.
| ifneq (,$(filter disp_dev,$(USEMODULE))) | |
| USEMODULE += st77xx | |
| endif | |
| ifneq (,$(filter st77xx,$(USEMODULE))) | |
| # indicate that a ST7735 is used | |
| USEMODULE += st7735 | |
| endif | |
| ifneq (,$(filter disp_dev,$(USEMODULE))) | |
| USEMODULE += st7735 | |
| endif |
There was a problem hiding this comment.
I did it by intention in that way. Modeling consistent dependencies in Kconfig and the makefiles are really a pain 😟 It took me a number of iterations to get them working. First, I did it exactly in that way you suggested using such a DRIVER variable in the test and selecting directly the driver variant by the board, but I couldn't get it working in that way.
Selecting directly the variant if disp_dev is enabled and not if just the driver st77xx is enabled will cause inconsistencies in tests/drivers/st77xx. Kconfig pulls in the driver by CONFIG_MODULE_ST77XX=y in app.config.test and the HAVE_ST* symbols then enable the specific variant that a given board has. The Makefile dependencies however would always use st7735 by default because of the DRIVER variable. To compile it with the other variants in the CI would then require
-
either to separate
tests/driver/st77*test apps for each variant to be able to set theDRIVER ?= st77*variable andCONFIG_MODULE_ST77*=y(however the code duplication makes definitely no sense) -
or to set the
DRIVERvariable explicitly for all boards that use a certain variant other than the default, for example:# define the driver to be used for selected boards ifneq (,$(filter stm32l496g-disco esp32s3-usb-otg esp32s2-lilygo-ttgo-t8,$(BOARD))) DRIVER := st7789 endif ifneq (,$(filter esp32s3-wt32-sc01-plus,$(BOARD))) DRIVER := l3g4200d_ng endif
To be honest, isn't it more logical that if an app says "I select the display driver st77xx whatever variant is used" that the board says "Ok, I have this variant" or it uses the default. This corresponds to the approach as it is modeled in Kconfig.
| ifeq (,$(filter st7789 st7796,$(USEMODULE))) | ||
| # enable st7735 as default if no other module is enabled | ||
| USEMODULE += st7735 | ||
| endif |
There was a problem hiding this comment.
This block should go in drivers/Makefile.dep
There was a problem hiding this comment.
I don't like that the drivers/Makefile.dep is blown up more an more with driver specific dependencies. The goal should be to keep drivers/Makefile.dep as simple and small as possible and to resolve driver specific dependencies in the driver implementation. That was a further reason why I selected st77xx instead of st77* when disp_dev is enabled. In that case, the driver's Makefile.dep is used to resolve the driver specific dependencies.
|
Just for information, I have alread working here 😉
The parallel interface on STM32 is GPIO-driven at the moment which is as slow as SPI serial interface display. That's why I'm experimenting a bit to use the FMC instead. Since the signal Since the parallel interfaces are implemented in |
|
@aabadie How do we proceed with the required changes of the dependencies? I had tried to explain why I defined the dependencies exactly this way. For short
|
19824: boards/sipeed_longan_nano: separate board definition for Sipeed Longan Nano TFT r=benpicco a=gschorcht ### Contribution description This PR provides a minimal separate board definition for the Sipeed Longan Nano version with TFT display which is just an extension of `boards/sipeed-longan-nano` with enabled TFT display module. From the lessons we had to learn with the Kconfig modelling of optional hardware, the TFT version of the Sipeed Longan Nano board has been split off into its own board definition based on the existing Siepeed Longan Nano board. Commits ba29a5e, 237819e, 6d8b56d and c5faf34 are small cleanups of peripheral configurations and could be split from this PR as follow-up PR (changes +70 -36). ### Testing procedure Green CI ``` BOARD=sipeed-longan-nano-tft make -j8 -C tests/drivers/st77xx flash ``` should work ### Issues/PRs references Follow up to PR #19813 and PR #19814 Prerequisite for PR #19825 and PR #19827 19855: gnrc_static: fix static PID assignment r=benpicco a=benpicco Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: Benjamin Valentin <[email protected]>
19824: boards/sipeed_longan_nano: separate board definition for Sipeed Longan Nano TFT r=benpicco a=gschorcht ### Contribution description This PR provides a minimal separate board definition for the Sipeed Longan Nano version with TFT display which is just an extension of `boards/sipeed-longan-nano` with enabled TFT display module. From the lessons we had to learn with the Kconfig modelling of optional hardware, the TFT version of the Sipeed Longan Nano board has been split off into its own board definition based on the existing Siepeed Longan Nano board. Commits ba29a5e, 237819e, 6d8b56d and c5faf34 are small cleanups of peripheral configurations and could be split from this PR as follow-up PR (changes +70 -36). ### Testing procedure Green CI ``` BOARD=sipeed-longan-nano-tft make -j8 -C tests/drivers/st77xx flash ``` should work ### Issues/PRs references Follow up to PR #19813 and PR #19814 Prerequisite for PR #19825 and PR #19827 Co-authored-by: Gunar Schorcht <[email protected]>
Instead of using a wrong intialization command sequence for power and frame control, default values after reset are used.
gschorcht
force-pushed
the
drivers/st77xx_generic
branch
2 times, most recently
from
7da778c to
2c0c1ca
Compare
|
|
||
| config MODULE_ST7735 | ||
| bool "ST7735 display" | ||
| default y if !MODULE_ST7789 && !MODULE_ST7796 |
There was a problem hiding this comment.
Why not use the following?
| default y if !MODULE_ST7789 && !MODULE_ST7796 | |
| default y if HAVE_ST7735 |
like for other variants.
There was a problem hiding this comment.
Just to model the dependencies exactly as defined in the makefiles: https://github.com/RIOT-OS/RIOT/blob/2c0c1caa33158436b6906c441f34d2f279b33e94/drivers/st77xx/Makefile.dep#L9-L13 Do you think that your suggestion would also give the same results. If so, I'm fine with it.
There was a problem hiding this comment.
I don't think it would result in the same.
If I am understanding this modelling correctly the following statements would be made:
- Multiple variants of ST77xx can be explicitly selected
- Selecting ST77xx would default to ST7735 unless a board declares a variant with HAVE_
Using the suggestion would mean that boards would not have a default and require explicit selection or HAVE_ST7735.
If only one variant can be used (due to implementation constraints) then probably better to use a choice, this makes the default more scalable.
There was a problem hiding this comment.
If only one variant can be used (due to implementation constraints) then probably better to use a choice, this makes the default more scalable.
It is intentionally a selection. I don't know if it really makes sense or there is a use case where a board has two or more displays with different ST77xx variants. But to make this possible in principle, there can be multiple ST77xx displays and the user can select multiple variants.
There was a problem hiding this comment.
If I am understanding this modelling correctly the following statements would be made:
- Multiple variants of ST77xx can be explicitly selected
- Selecting ST77xx would default to ST7735 unless a board declares a variant with HAVE_
Exactly.
There was a problem hiding this comment.
@aabadie Are you fine now with the Kconfig dependency modelling? This was the last point open for this PR.
There was a problem hiding this comment.
@aabadie Are you fine now with the Kconfig dependency modelling? This was the last point open for this PR.
I'm fine with the changes. Let's get this in!
| # this file enables modules defined in Kconfig. Do not use this file for | ||
| # application configuration. This is only needed during migration. | ||
| CONFIG_MODULE_ST7735=y | ||
| CONFIG_MODULE_ST77XX=y |
There was a problem hiding this comment.
I think a Kconfig file is needed in the application directory to select the right module driver based on the type of board, like it is done in the Makefile.
config APPLICATION
select MODULE_ST7789 if BOARD_with_st7789
select MODULE_ST7796 if BOARD_with_st7796
select MODULE_ST7735 if !BOARD_with_st7789 && !BOARD_with_st7796
(untested)
There was a problem hiding this comment.
Do you mean a specific board by BOARD_with_st7789 and BOARD_with_st7796?
I'm not sure, but wouldn't it also work with the following?
config APPLICATION
select MODULE_ST7789 if HAVE_ST7789
select MODULE_ST7796 if HAVE_ST7796
select MODULE_ST7735 if !HAVE_ST7789 && !HAVE_ST7796
There was a problem hiding this comment.
I think a Kconfig file is needed in the application directory to select the right module driver based on the type of board, like it is done in the Makefile.
@aabadie I'm still a bit unsure about the right way to do Kconfig modeling. Do we agree that the following approach is the right one from the user's point of view?
(Top) → Drivers → Display Device Drivers → ST77xx display driver
[ ] ST7735 display
[*] ST7789 display
[ ] ST7796 display
That is, the user selects Drivers -> Display Device Drivers -> ST77xx display driver and the default variants is/are selected by the corresponding HAVE_ST77* features, e.g:
config MODULE_ST7789
bool "ST7789 display"
default y if HAVE_ST7789
depends on MODULE_ST77XX
There was a problem hiding this comment.
but wouldn't it also work with the following?
That seems better indeed. I don't know if that is good though. Maybe @MrKevinWeiss can tell
There was a problem hiding this comment.
The current state seems correct to me.
"I want a generic st77xx device", if a board has it declared than it will select it by default and if not, fallback to the default st7735.
19884: drivers/touch_dev_gestures: add gesture recognition for touch devices r=aabadie a=gschorcht ### Contribution description This PR adds simple gesture recognition for touch devices accessed via the generic Touch Device API. It can be used in conjunction with device drivers that use either interrupts or polling mode. It supports up to two touches and the following gestures: - Single and double tap at given position - Long press and release given position - Moving while pressed with current position - Swipe left, right, up and down - Zoom in (spread) and out (pinch) Gesture recognition has been tested with: - [x] `stm32f746g-disco` (works out of the box) - [x] `stm32f723e-disco` (works out of the box) - [x] `stm32f429i-disc1` (works on top of PR #19885) - [x] `stm32l496g-disco` (works with my local LCD display changes waiting for PR #19825, not yet provided) - [x] `esp32s3-wt32-sc01-plus` (new board, not yet provided) ### Testing procedure Flash `tests/drivers/touch_dev_gestures` to a board with touch pane, for example: ``` BOARD=stm32f746g-disco make -j8 -C tests/drivers/touch_dev_gestures/ flash ``` PR #19885 is required for the `stm32f429i-disc1` board. The output should look like this: ``` main(): This is RIOT! (Version: 2023.10-devel-121-g81c5c-drivers/touch_dev_gestures) Single Tap X: 255, Y:154 Single Tap X: 253, Y:153 Double Tap X: 253, Y:149 Swipe right Swipe down Swipe left Swipe up Pressed X: 257, Y:155 Moving X: 257, Y:155 Moving X: 257, Y:155 Moving X: 259, Y:156 Moving X: 262, Y:157 Moving X: 266, Y:158 Moving X: 269, Y:160 Moving X: 273, Y:162 Moving X: 276, Y:165 Moving X: 278, Y:167 Moving X: 278, Y:169 Moving X: 278, Y:169 Released X: 279, Y:172 ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <[email protected]>
19884: drivers/touch_dev_gestures: add gesture recognition for touch devices r=aabadie a=gschorcht ### Contribution description This PR adds simple gesture recognition for touch devices accessed via the generic Touch Device API. It can be used in conjunction with device drivers that use either interrupts or polling mode. It supports up to two touches and the following gestures: - Single and double tap at given position - Long press and release given position - Moving while pressed with current position - Swipe left, right, up and down - Zoom in (spread) and out (pinch) Gesture recognition has been tested with: - [x] `stm32f746g-disco` (works out of the box) - [x] `stm32f723e-disco` (works out of the box) - [x] `stm32f429i-disc1` (works on top of PR #19885) - [x] `stm32l496g-disco` (works with my local LCD display changes waiting for PR #19825, not yet provided) - [x] `esp32s3-wt32-sc01-plus` (new board, not yet provided) ### Testing procedure Flash `tests/drivers/touch_dev_gestures` to a board with touch pane, for example: ``` BOARD=stm32f746g-disco make -j8 -C tests/drivers/touch_dev_gestures/ flash ``` PR #19885 is required for the `stm32f429i-disc1` board. The output should look like this: ``` main(): This is RIOT! (Version: 2023.10-devel-121-g81c5c-drivers/touch_dev_gestures) Single Tap X: 255, Y:154 Single Tap X: 253, Y:153 Double Tap X: 253, Y:149 Swipe right Swipe down Swipe left Swipe up Pressed X: 257, Y:155 Moving X: 257, Y:155 Moving X: 257, Y:155 Moving X: 259, Y:156 Moving X: 262, Y:157 Moving X: 266, Y:158 Moving X: 269, Y:160 Moving X: 273, Y:162 Moving X: 276, Y:165 Moving X: 278, Y:167 Moving X: 278, Y:169 Moving X: 278, Y:169 Released X: 279, Y:172 ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <[email protected]>
19884: drivers/touch_dev_gestures: add gesture recognition for touch devices r=aabadie a=gschorcht ### Contribution description This PR adds simple gesture recognition for touch devices accessed via the generic Touch Device API. It can be used in conjunction with device drivers that use either interrupts or polling mode. It supports up to two touches and the following gestures: - Single and double tap at given position - Long press and release given position - Moving while pressed with current position - Swipe left, right, up and down - Zoom in (spread) and out (pinch) Gesture recognition has been tested with: - [x] `stm32f746g-disco` (works out of the box) - [x] `stm32f723e-disco` (works out of the box) - [x] `stm32f429i-disc1` (works on top of PR #19885) - [x] `stm32l496g-disco` (works with my local LCD display changes waiting for PR #19825, not yet provided) - [x] `esp32s3-wt32-sc01-plus` (new board, not yet provided) ### Testing procedure Flash `tests/drivers/touch_dev_gestures` to a board with touch pane, for example: ``` BOARD=stm32f746g-disco make -j8 -C tests/drivers/touch_dev_gestures/ flash ``` PR #19885 is required for the `stm32f429i-disc1` board. The output should look like this: ``` main(): This is RIOT! (Version: 2023.10-devel-121-g81c5c-drivers/touch_dev_gestures) Single Tap X: 255, Y:154 Single Tap X: 253, Y:153 Double Tap X: 253, Y:149 Swipe right Swipe down Swipe left Swipe up Pressed X: 257, Y:155 Moving X: 257, Y:155 Moving X: 257, Y:155 Moving X: 259, Y:156 Moving X: 262, Y:157 Moving X: 266, Y:158 Moving X: 269, Y:160 Moving X: 273, Y:162 Moving X: 276, Y:165 Moving X: 278, Y:167 Moving X: 278, Y:169 Moving X: 278, Y:169 Released X: 279, Y:172 ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <[email protected]>
If the controller-specific driver supports multiple controller variants, the variant has to be specified in the configuration parameter set.
If a board definition already used the ST7735 driver, `st7735*.h` header files and `ST7735_*` macros were used in the board definitions to define the default configuration parameter set. For backward compatibility these header files are kept and the `ST7735_*` macros are mapped to the `ST77XX_*` macros if they are defined.
gschorcht
force-pushed
the
drivers/st77xx_generic
branch
from
2c0c1ca to
020861b
Compare
|
@aabadie 👍👍👍 Great, thanks for the review. I have squashed the PR. |
|
bors merge |
|
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. If you want to switch to GitHub's built-in merge queue, visit their help page. |
19827: drivers/st77xx: implement initialization r=aabadie a=gschorcht ### Contribution description This PR implements correct initialization for ST7735, ST7789 and ST7796. A number of configuration parameters are exposed via Kconfig. The user can decide whether to use custom configuration parameters or reset defaults. ~To be compilable, the PR includes PR #19824 and PR #19825~ ### Testing procedure - Green CI - `tests/drivers/disp_dev` and `tests/drivers/st77xx` should still work for all boards using a ST77xx display. - The PR was already tested for these tests for: - [x] `adafruit-pybadge` - [x] `esp32s2-lilygo-ttgo-t8` - [x] `esp32s3-usb-otg` - [x] `sipeed-longan-nano` ### Issues/PRs references ~Depends on #19825~ Fixes #19818 Co-authored-by: Gunar Schorcht <[email protected]>
4 participants
Contribution description
This PR provides the following changes:
st7735tost77xxst7735tost77xxst7735,st7789andst7796ST7735_PARAM_*symbolsThe PR should solve the remaining dependency issues in KConfig we had by using
st7735module for different controller variants. The backward compatibility header files should work for boards that still useST7735_PARAM_*in their board definitions so that the board defintions at user's side use shouldn't be affected.To be compilable, the PR includes PR #19824.Testing procedure
tests/drivers/disp_devandtests/drivers/st77xxshould still work for all boards using a ST77xx display.adafruit-pybadgeesp32s2-lilygo-ttgo-t8esp32s3-usb-otgsipeed-longan-nanoIssues/PRs references
Workaround for issue #19818
Preqruisite for PR #19827