You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/09/15 10:49:50 UTC
[GitHub] [incubator-nuttx] PeterBee97 opened a new pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
PeterBee97 opened a new pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550
## Summary
Fix PWM Driver bug
Ported ST7735, ST7789 and GC9A01 drivers to ESP32C3 platform
## Impact
PWM Timer0 6channel output working, but other timers still do not
## Testing
LCD driver works on 1.54" 240x240 ST7789 LCD module, 0.96" 80x160 ST7735S LCD module and 240x240 GC9A01 round display
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] PeterBee97 closed pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
PeterBee97 closed pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] PeterBee97 commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
PeterBee97 commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709175009
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
+
+if ESP32C3_SPI_LCD
+
+config SPI_SWAPBYTES
+ bool "Swap high and low bytes during 16bit SPI send"
Review comment:
Yes you're right, that is being used for correct display data. This config here is for swapping the bytes of command data, which is not really necessary if the command data was sent as 8-bit in the first place. So I'm a little confused here as whether to add swapping to the SPI driver or just change the drivers to 8bit SPI. (I believed someone use 16bit for a reason?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] PeterBee97 commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
PeterBee97 commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709194787
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
+
+if ESP32C3_SPI_LCD
+
+config SPI_SWAPBYTES
+ bool "Swap high and low bytes during 16bit SPI send"
Review comment:
Sure, it's the controller driver that needs fixing then. Thanks for the knowledge!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709120110
##########
File path: boards/risc-v/esp32c3/esp32c3-devkit/src/esp32c3_bringup.c
##########
@@ -189,6 +198,28 @@ int esp32c3_bringup(void)
}
#endif
+#ifdef CONFIG_VIDEO_FB
+ ret = fb_register(0, 0);
+ if (ret < 0)
+ {
+ _err("ERROR: Failed to initialize Frame Buffer Driver.\n");
Review comment:
```suggestion
syslog(LOG_ERR, "ERROR: Failed to initialize Frame Buffer Driver.\n");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709168037
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
Review comment:
One suggestion in case you remove created this config as a shortcut:
you can create a defconfig under `nuttx/boards/risc-v/esp32c3/esp32c3-devkit/configs`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] PeterBee97 commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
PeterBee97 commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709125999
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
+
+if ESP32C3_SPI_LCD
+
+config SPI_SWAPBYTES
+ bool
Review comment:
I encountered this issue when sending SPI commands with NuttX ST7789 drivers, which is sending 16 bits cmd code. Actually 8-bit sending is enough, but sending 16-bit cmd requires a byte swap on ESP32C3. So is this really necessary, or should I just change the drivers to send 8bit cmd instead? And the display data bytes are inverted as well, but that requires swapping from source in order to use DMA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] PeterBee97 commented on pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
PeterBee97 commented on pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#issuecomment-920009607
> This PR could be broken down into at least 3 smaller PRs:
>
> 1. New display drivers
> 2. ESP32-C3 support for the new display drivers (depends on **1**)
> 3. ESP32-C3 PWM fixes
>
> Just by reading the PR title and description, I would never know that **1** is also included in this PR.
> One **1** is, IMHO, the most relevant contribution to the project in this PR.
>
> I suggest that at least open a new PR with the new display drivers, so that this new contribution does not go unnoticed by the others.
Thanks for the suggestion, I will split it in separate PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#issuecomment-919997548
This PR could be broken down into at least 3 smaller PRs:
1) New display drivers
2) ESP32-C3 support for the new display drivers (depends on **1**)
3) ESP32-C3 PWM fixes
Just by reading the PR title and description, I would never know that **1** is also included in this PR.
One **1** is, IMHO, the most relevant contribution to the project in this PR.
I suggest that at least open a new PR with the new display drivers, so that this new contribution does not go unnoticed by the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709153294
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
+
+if ESP32C3_SPI_LCD
+
+config SPI_SWAPBYTES
+ bool "Swap high and low bytes during 16bit SPI send"
Review comment:
Shouldn't this swapping be performed at application-level?
See that LVGL has an option for this:
https://github.com/apache/incubator-nuttx-apps/blob/master/graphics/lvgl/Kconfig#L119-L122
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709116772
##########
File path: arch/risc-v/src/esp32c3/esp32c3_spi.c
##########
@@ -849,6 +849,9 @@ static uint32_t esp32c3_spi_poll_send(FAR struct esp32c3_spi_priv_s *priv,
{
uint32_t val;
+#ifdef CONFIG_SPI_SWAPBYTES
+ if (priv->nbits == 16) wd = __builtin_bswap16(wd);
Review comment:
It's against the NuttX Coding standard
Please see here the alternatives:
https://nuttx.apache.org/docs/latest/contributing/coding_style.html?highlight=coding%20style#if-then-else-statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709190890
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
+
+if ESP32C3_SPI_LCD
+
+config SPI_SWAPBYTES
+ bool "Swap high and low bytes during 16bit SPI send"
Review comment:
> Yes you're right, that is being used for correct display data. This config here is for swapping the bytes of command data, which is not really necessary if the command data was sent as 8-bit in the first place.
Agree. The SPI display drivers are responsible for configuring the SPI controller width before sending command and data.
> So I'm a little confused here as whether to add swapping to the SPI driver or just change the drivers to 8bit SPI. (I believed someone use 16bit for a reason?)
Sorry, I couldn't follow. But if there is any command being erroneously sent as 16-bit, it should be fixed at the peripheral driver. And the if the data needs to be swapped, the application should be the one responsible for it.
The SPI controller driver shouldn't have this kind of knowledge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#issuecomment-919997548
This PR could be broken down into at least 3 smaller and self-contained PRs:
1) New display drivers
2) ESP32-C3 support for the new display drivers
3) ESP32-C3 PWM fixes
Just by reading the PR title and description, I would never know that **1** is also included in this PR.
One **1** is, IMHO, the most relevant contribution to the project.
I suggest that at least open a new PR with the new display drivers, so that this new contribution does not go unnoticed by the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709157618
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
Review comment:
I believe there is no need to create this `Generic SPI LCD Module` entry in Kconfig.
I understand that this is just a `shortcut` to selecting each individual config.
And by the way, I think that defaulting to `ESP32C3_SPI_SWCS` breaks the "generic" qualifier for this `ESP32C3_SPI_LCD` config, since one may want to not use the CS via software.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709190890
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
+
+if ESP32C3_SPI_LCD
+
+config SPI_SWAPBYTES
+ bool "Swap high and low bytes during 16bit SPI send"
Review comment:
> Yes you're right, that is being used for correct display data. This config here is for swapping the bytes of command data, which is not really necessary if the command data was sent as 8-bit in the first place.
Agree. The SPI display drivers are responsible for configuring the SPI width before sending command and data.
> So I'm a little confused here as whether to add swapping to the SPI driver or just change the drivers to 8bit SPI. (I believed someone use 16bit for a reason?)
Sorry, I couldn't follow. But if there is any command being erroneously sent as 16-bit, it should be fixed at the peripheral driver. And the if the data needs to be swapped, the application should be the one responsible for it.
The SPI controller driver shouldn't have this kind of knowledge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709108411
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
+
+if ESP32C3_SPI_LCD
+
+config SPI_SWAPBYTES
+ bool
Review comment:
Please, add a prompt message to this config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#discussion_r709168037
##########
File path: arch/risc-v/src/esp32c3/Kconfig
##########
@@ -261,6 +262,29 @@ config ESP32C3_SPIFLASH
select MTD_BYTE_WRITE
select MTD_PARTITION
+config ESP32C3_SPI_LCD
+ bool "Generic SPI LCD Module"
+ default n
+ select ESP32C3_SPI2
+ select SPI_CMDDATA
+ select ESP32C3_SPI_SWCS
+ select SPI_SWAPBYTES
Review comment:
One suggestion in case you created this config as a shortcut:
you can create a defconfig under `nuttx/boards/risc-v/esp32c3/esp32c3-devkit/configs`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#issuecomment-919997548
This PR could be broken down into at least 3 smaller PRs:
1) New display drivers
2) ESP32-C3 support for the new display drivers
3) ESP32-C3 PWM fixes
Just by reading the PR title and description, I would never know that **1** is also included in this PR.
One **1** is, IMHO, the most relevant contribution to the project in this PR.
I suggest that at least open a new PR with the new display drivers, so that this new contribution does not go unnoticed by the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#issuecomment-919945500
@PeterBee97 please fix the style warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4550: ESP32C3 PWM Driver fix && LCD Drivers
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4550:
URL: https://github.com/apache/incubator-nuttx/pull/4550#issuecomment-919997548
This PR could be broken down into at least 3 smaller and self-contained PRs:
1) New display drivers
2) ESP32-C3 support for the new display drivers
3) ESP32-C3 PWM fixes
Just by reading the PR title and description, I would never know that **1** is also included in this PR.
One **1** is, IMHO, the most relevant contribution to the project in this PR.
I suggest that at least open a new PR with the new display drivers, so that this new contribution does not go unnoticed by the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org