-
Notifications
You must be signed in to change notification settings - Fork 2k
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
drivers/lcd: support MCU 8080 8-bit parallel mode #19915
drivers/lcd: support MCU 8080 8-bit parallel mode #19915
Conversation
To be clear that the functions are low-level functions, they are renamed to `lcd_ll_*`.
f5e86aa
to
60ae751
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty straightforward, just some small things
disp_dev_t *dev; /**< Pointer to the generic display device */ | ||
#endif | ||
const lcd_driver_t *driver; /**< LCD driver */ | ||
const lcd_params_t *params; /**< Device initialization parameters */ | ||
#if MODULE_LCD_PARALLEL || DOXYGEN | ||
mutex_t lock; /**< Mutex used to lock the device in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this?
I think if there are two threads trying to access the display, this would lead to chaos either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question 🤔 I have already discussed this with @aabadie and @bergzand and we basically agreed that mutual exclusion should be handled by the high-level driver, e.g. disp_dev
. However, since disp_dev
doesn't do this and the device driver implicitly uses mutual exclusion in SPI mode, I decided to implement mutual exclusion by the driver in parallel mode as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a use case where we would ever have multiple threads access the low-level driver, so I'd just avoid the added complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But please document explicitly, that it is not thread safe. (I do agree with delegation of thread safety to upper layers - if needed at all. But being upfront about that may prevent long debugging sessions.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not really sure. The execution of a function like lcd_fill
at low level looks like this:
lcd_ll_acquire(dev);
lcd_ll_set_area(dev, x1, x2, y1, y2);
lcd_ll_cmd_start(dev, LCD_CMD_RAMWR, true);
for (int i = 0; i < (num_pix - 1); i++) {
lcd_ll_write_bytes(dev, true, (uint8_t *)&color, sizeof(color));
}
lcd_ll_write_bytes(dev, false, (uint8_t *)&color, sizeof(color));
lcd_ll_release(dev);
That is, we always acquire the device, execute some low-level transfers and releases the device. This is necessary in SPI mode to avoid that each low-level transfer acquires and release the SPI device.
But what does lcd_ll_acquire
and lcd_ll_release
mean in parallel mode if we don't use a mutex? lcd_ll_acquire
and lcd_ll_release
are not internal function but part of the documented low-level API and suggest that the driver realizes mutual exclusion.
disp_dev_t *dev; /**< Pointer to the generic display device */ | ||
#endif | ||
const lcd_driver_t *driver; /**< LCD driver */ | ||
const lcd_params_t *params; /**< Device initialization parameters */ | ||
#if MODULE_LCD_PARALLEL || DOXYGEN | ||
mutex_t lock; /**< Mutex used to lock the device in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this?
I think if there are two threads trying to access the display, this would lead to chaos either way.
Without it, we wouldn't have to drop the const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without it, we wouldn't have to drop the
const
Exactly.
@@ -1,4 +1,8 @@ | |||
PSEUDOMODULES += lcd_multi_cntrl | |||
|
|||
PSEUDOMODULES += lcd_spi | |||
PSEUDOMODULES += lcd_parallel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lcd_parallel_gpio
?
I suppose if we have a native LCD interface we would need a different module name for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If have already defined a number of further pseudomodules for follow-up PRs that will control which data struct members and what code will be compiled in:
lcd_parallel
just enables the parallel mode support, no matter whether 8-bit or 16 bits are usedlcd_parallel_16bit
will enable 16-bit support in addition to the 8-bit-parallel supportlcd_parallel_ll_mcu
will enable the low-level MCU driven interface that has to be implemented by the respective MCU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lcd_parallel
uses the bit-banging implementation if the board doesn't enable the MCU driven implementation by lcd_parallel_ll_mcu
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be it would feel cleaner if lcd_parallel
were just a pseudo-module and each implementation had it's own name. Or is there a reason why you chose to do it that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lcd_parallel
is already a pseudomodule which is only intended to enable the parallel interface support, i.e. to declare the required additional members of the device data structure like the GPIOs which are needed independently of the parallel interface implementation and to define functions to use the parallel interface.
Since this PR is only a evolutionary step in a series of follow-on PRs for the final implementation, the GPIO-driven bit-banging implementation is the only implementation for now and is therefore an integral part of the driver. In the final implementation, the GPIO-driven implementation will be moved to a separate file and will be only the default or fallback implementation if no other implementation is enabled.
Instead of trying to explain it in detail, please have a look at the final implementation https://github.com/gschorcht/RIOT-Xtensa-ESP/tree/drivers/lcd_work. Due to its size, I have split it into several steps as separate PRs, so naming the module may not seem reasonable for now. There will be at least 4 follow-up PRs to reach the final implementation.
Please squash |
For the parallel interface support the following changes were made: 1. Additional `lcd_*` pseudomodules are defined to control whether SPI serial or MCU 8080 8-/16-bit parallel interface is used. 2. The low level function implementation was extended for parallel interfaces so that the now can use either the SPI serial interface or the MCU 8080 8-/16-bit parallel interface. Using the `lcc_*` modules, either the SPI serial interface or the MCU 8080 8-/16-bit interface or even both for multiple display can be used simultaneously.
8f90597
to
b0ec24b
Compare
bors merge |
19914: boards: complete SD Card MTD definition for several bords r=benpicco a=gschorcht ### Contribution description This PR completes the MTD definition for the following boards: - `seeedstudio-gd32` - `sipeed-longan-nano` including `sipeed-longan-nano-tft` - `waveshare-nrf52840-eval-kit` - ESP32x boards that have an SPI SD Card interface and use `mtd_sdcard_default` ### Testing procedure Green CI ### Issues/PRs references#19465 Prerequisite for PR #19465 19915: drivers/lcd: support MCU 8080 8-bit parallel mode r=benpicco a=gschorcht ### Contribution description LCD driver ICs usually support - SPI serial mode, - MCU 8080 8-bit parallel mode and - MCU 8080 16-bit parallel mode. This PR extends the LCD display driver API to support the MCU 8080 8-/16-bit parallel modes and implements a GPIO-driven MCU 8080 8-bit parallel mode. The following features are already working locally and will be provided as follow-on PRs for which this PR is a prerequisite. - GPIO-driven bit-banging implementation of the 16-bit mode of the MCU 8080 parallel interface - Enabling the display on `stm32f723e-disco` and `stm32l496g-disco` using the feature above - Definition of a low-level API for the parallel modes using the LCD controller of the MCU - Using FMC for the display on `stm32f723e-disco` and `stm32l496g-disco` - Using LCD controller for the display of `esp32-wt32-sc01-plus` (PR #19917) ### Testing procedure The PR can be tested with PR #19917 on top of this PR. ``` BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/st77xx flash ``` The following video shows the test. **Please note** The test is pretty slow because the display has 480 x 320 pixels and the MCU 8080 8-bit parallel interface is realized by a GPIO-driven bit-banging implementation where each GPIO of the data bus is set separately. A follow-up PR will use the ESP32-S3 LCD controller and DMA for this board. This PR just defines the extension of the driver by the parallel interface and provides the bit-banging implementation for MCUs that don't have a LCD controller on chip. https://github.com/RIOT-OS/RIOT/assets/31932013/c1e3e3d7-05d9-4ca5-8fff-9a5eaca50fba ### Issues/PRs references 19919: drivers/st77xx: introduce rotation defines r=benpicco a=gschorcht ### Contribution description The PR introduces counterclockwise rotations for the definition of parameter `ST77XX_PARAM_ROTATION`. It is more intuitive and universal to use `ST77XX_ROTATION_{0,90,180,270}` instead of `ST77XX_ROTATION_{ST77XX_ROTATION_{VERT,VERT_FLIP,HORZ,HORZ_FLIP}`, especially because the orientation of the display may vary with respect to the orientation of the board. ### Testing procedure `tests/drivers/st77xx` should still work, for example: ``` BOARD=adafruit-pybadge make -C tests/drivers/st77xx flash ``` ``` BOARD=esp32s3-usb-otg make -j8 -C tests/drivers/st77xx flash ``` ### Issues/PRs references 19931: boards: fix documentation for GD32V boards and doxygen 1.9.4 r=benpicco a=gschorcht ### Contribution description This PR fixes some small problems in documentation of `sipeed-longan-nano`, `sipeed-longan-nano-tft` and `seeedstudio-gd32` for doxygen 1.9.4 that is used on `doc.riot-os.org`. Doxygen version 1.9.4 doesn't like anymore - single double quotes as symbol for the inches unit in the text - line breaks in `[]()` to avoid exhausting the 100 characters per line. See https://doc.riot-os.org/group__boards__sipeed__longan__nano.html for example. Doxygen 1.9.1 which is part of `riot-docker` container didn't have theses problems 😟 ### Testing procedure Documentation should be fixed. ### Issues/PRs references 19935: boards/nucleo64: fix SPI Arduino mapping for most boards r=benpicco a=maribu ### Contribution description Before, the Arduino SPI mapping for all Nucleo-64 boards was incorrect. With this, the situation improves to the following: - [x] nucleo-f030r8 - [ ] nucleo-f070rb - No SPI buses provided. - [x] nucleo-f072rb - [x] nucleo-f091rc - [x] nucleo-f103rb - [ ] nucleo-f302r8 - No SPI bus at D11, D12, D13 provided - [x] nucleo-f303re - [x] nucleo-f334r8 - [x] nucleo-f401re - [x] nucleo-f410rb - [x] nucleo-f411re - [x] nucleo-f446re - [x] nucleo-g070rb - [x] nucleo-g071rb - [x] nucleo-g431rb - [x] nucleo-g474re - [x] nucleo-l053r8 - [x] nucleo-l073rz - [x] nucleo-l152re - No SPI bus at D11, D12, D13 provided - [x] nucleo-l452re - [x] nucleo-l476rg - [x] nucleo-wl55jc The remaining offenders still need to be fixed, but that is better done one PR at a time. ### Testing procedure - Check if the SPI device provided in the given `boards/<BOARD_NAME>/incude/periph_conf.h` is indeed `SPI_DEV(0)`, or in `periph_conf.h` the correct SPI dev is found - this should be fine for all boards above, except for the unchecked ones or: - run #19932: The SPI test should pass now ### Issues/PRs references Bug found in #19932 (comment) Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: Marian Buschsieweke <[email protected]>
Build failed (retrying...): |
19914: boards: complete SD Card MTD definition for several bords r=benpicco a=gschorcht ### Contribution description This PR completes the MTD definition for the following boards: - `seeedstudio-gd32` - `sipeed-longan-nano` including `sipeed-longan-nano-tft` - `waveshare-nrf52840-eval-kit` - ESP32x boards that have an SPI SD Card interface and use `mtd_sdcard_default` ### Testing procedure Green CI ### Issues/PRs references#19465 Prerequisite for PR #19465 19915: drivers/lcd: support MCU 8080 8-bit parallel mode r=benpicco a=gschorcht ### Contribution description LCD driver ICs usually support - SPI serial mode, - MCU 8080 8-bit parallel mode and - MCU 8080 16-bit parallel mode. This PR extends the LCD display driver API to support the MCU 8080 8-/16-bit parallel modes and implements a GPIO-driven MCU 8080 8-bit parallel mode. The following features are already working locally and will be provided as follow-on PRs for which this PR is a prerequisite. - GPIO-driven bit-banging implementation of the 16-bit mode of the MCU 8080 parallel interface - Enabling the display on `stm32f723e-disco` and `stm32l496g-disco` using the feature above - Definition of a low-level API for the parallel modes using the LCD controller of the MCU - Using FMC for the display on `stm32f723e-disco` and `stm32l496g-disco` - Using LCD controller for the display of `esp32-wt32-sc01-plus` (PR #19917) ### Testing procedure The PR can be tested with PR #19917 on top of this PR. ``` BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/st77xx flash ``` The following video shows the test. **Please note** The test is pretty slow because the display has 480 x 320 pixels and the MCU 8080 8-bit parallel interface is realized by a GPIO-driven bit-banging implementation where each GPIO of the data bus is set separately. A follow-up PR will use the ESP32-S3 LCD controller and DMA for this board. This PR just defines the extension of the driver by the parallel interface and provides the bit-banging implementation for MCUs that don't have a LCD controller on chip. https://github.com/RIOT-OS/RIOT/assets/31932013/c1e3e3d7-05d9-4ca5-8fff-9a5eaca50fba ### Issues/PRs references Co-authored-by: Gunar Schorcht <[email protected]>
This PR was included in a batch that was canceled, it will be automatically retried |
19915: drivers/lcd: support MCU 8080 8-bit parallel mode r=benpicco a=gschorcht ### Contribution description LCD driver ICs usually support - SPI serial mode, - MCU 8080 8-bit parallel mode and - MCU 8080 16-bit parallel mode. This PR extends the LCD display driver API to support the MCU 8080 8-/16-bit parallel modes and implements a GPIO-driven MCU 8080 8-bit parallel mode. The following features are already working locally and will be provided as follow-on PRs for which this PR is a prerequisite. - GPIO-driven bit-banging implementation of the 16-bit mode of the MCU 8080 parallel interface - Enabling the display on `stm32f723e-disco` and `stm32l496g-disco` using the feature above - Definition of a low-level API for the parallel modes using the LCD controller of the MCU - Using FMC for the display on `stm32f723e-disco` and `stm32l496g-disco` - Using LCD controller for the display of `esp32-wt32-sc01-plus` (PR #19917) ### Testing procedure The PR can be tested with PR #19917 on top of this PR. ``` BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/st77xx flash ``` The following video shows the test. **Please note** The test is pretty slow because the display has 480 x 320 pixels and the MCU 8080 8-bit parallel interface is realized by a GPIO-driven bit-banging implementation where each GPIO of the data bus is set separately. A follow-up PR will use the ESP32-S3 LCD controller and DMA for this board. This PR just defines the extension of the driver by the parallel interface and provides the bit-banging implementation for MCUs that don't have a LCD controller on chip. https://github.com/RIOT-OS/RIOT/assets/31932013/c1e3e3d7-05d9-4ca5-8fff-9a5eaca50fba ### Issues/PRs references Co-authored-by: Gunar Schorcht <[email protected]>
bors cancel |
Canceled. |
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. |
19917: boards: add support for ESP32-S3 WT32 SC01 Plus board r=benpicco a=gschorcht ### Contribution description This PR provides support for the [Wireless Tag WT32-SC01 Plus with ESP32-S3](http://en.wireless-tag.com/product-item-26.html) board. The board has the following main features: - ESP32-S3 SoC - 16 MB Flash - 2 MB QSPI RAM - 3.5" LCD Display 480 x 320 with ST7796UI - Capacitive Touch Panel with FT6336U - SD Card SPI mode - USB Type-C **Please note**: The ST7796UI display uses the MCU8080 8-bit parallel interface which require the changes in PR #19915. ### Testing procedure 1. `tests/drivers/st77xx` should work on top of PR #19915. ``` BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/st77xx flash ``` 2. `tests/drivers/ft5x06` should work with `stdio_uart` and the debugging tool. ``` USEMODULE=stdio_uart BOARD=esp32s3-wt32-sc01-plus make -j8 -C tests/drivers/ft5x06/ flash term ``` ``` main(): This is RIOT! (Version: 2023.10-devel-245-g55bd7-boards/esp32s3-wt32-sc01-plus) FT5x06 test application +------------Initializing------------+ Initialization successful 1 touch detected Touch 1 - X: 177, Y:96 Touch 1 - X: 177, Y:96 Touch 1 - X: 177, Y:96 Touch 1 - X: 177, Y:97 Touch 1 - X: 178, Y:99 Touch 1 - X: 180, Y:102 Touch 1 - X: 184, Y:106 Touch 1 - X: 190, Y:110 Touch 1 - X: 197, Y:116 Touch 1 - X: 204, Y:122 Released! ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <[email protected]>
Contribution description
LCD driver ICs usually support
This PR extends the LCD display driver API to support the MCU 8080 8-/16-bit parallel modes and implements a GPIO-driven MCU 8080 8-bit parallel mode.
The following features are already working locally and will be provided as follow-on PRs for which this PR is a prerequisite.
stm32f723e-disco
andstm32l496g-disco
using the feature abovestm32f723e-disco
andstm32l496g-disco
esp32-wt32-sc01-plus
(PR boards: add support for ESP32-S3 WT32 SC01 Plus board #19917)Testing procedure
The PR can be tested with PR #19917 on top of this PR.
The following video shows the test.
Please note The test is pretty slow because the display has 480 x 320 pixels and the MCU 8080 8-bit parallel interface is realized by a GPIO-driven bit-banging implementation where each GPIO of the data bus is set separately. A follow-up PR will use the ESP32-S3 LCD controller and DMA for this board. This PR just defines the extension of the driver by the parallel interface and provides the bit-banging implementation for MCUs that don't have a LCD controller on chip.
output.mp4
Issues/PRs references