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/09 16:30:31 UTC
[GitHub] [incubator-nuttx] michallenc opened a new pull request #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
michallenc opened a new pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506
## Summary
This PR presents the changes regarding ST7735 and ST7789 controllers discussed last month in NuttX mailing list. It basically merges those two controllers into one single file called ST77XX. Both files for ST7735 and ST7789 were mostly the same except
for different preset resolution and inversion color command in ST7789 driver. This can be helpful to avoid having diverging functionalities and bugfixes and also opens the doors for easier implementation of other similar controllers (like ST7796 etc.)
The configuration options stays separately for ST7735 and ST7789 to make it easier for user to configured those displays and controllers. This also allows to have preset resolutions for different controllers from ST77XX family.
## Impact
Boards using ST77XX controllers.
## Testing
Tested for ST7789 controller with Teensy 4.1 board, other configurations succesfully configured and compiled.
--
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] acassis commented on pull request #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#issuecomment-920870942
Yes, maybe the way to solve it is passing some parameter in the LCD initialize to indicate the type of LCD, this way we could have more than 1 initialization with the same or different type
--
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 #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#discussion_r705528998
##########
File path: drivers/lcd/Kconfig
##########
@@ -619,15 +625,15 @@ if LCD_ST7789
---help---
Specifies the Y offset of the LCD.
- config LCD_ST7789_BPP
+ config LCD_ST77XX_BPP
Review comment:
I'm concerned about having two `config` options with the same identifier.
Have you tested enabling both `LCD_ST7735` and `LCD_ST7789`?
This is a valid scenario and I believe will probably result in a crash in `Kconfig`.
--
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] acassis commented on a change in pull request #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#discussion_r708533978
##########
File path: drivers/lcd/Kconfig
##########
@@ -619,15 +625,15 @@ if LCD_ST7789
---help---
Specifies the Y offset of the LCD.
- config LCD_ST7789_BPP
+ config LCD_ST77XX_BPP
Review comment:
@michallenc maybe you can define the CONFIG_LCD_ST77XX as main option and after enabling it the user can select (using choice) his model as ST7735 or ST7789
--
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] michallenc commented on pull request #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
michallenc commented on pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#issuecomment-921850947
> Yes, maybe the way to solve it is passing some parameter in the LCD initialize to indicate the type of LCD, this way we could have more than 1 initialization with the same or different type
Yes, something like in [arch/.../imxrt_flexcan.c](https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/imxrt/imxrt_flexcan.c). I am not sure how this will turn out, It´s likely that I will put this on ice as my semester starts next week and I need to start working on [my semestral & bechalor project](https://fel.cvut.cz/en/education/semestral-projects.html?supervisor=pisa&project_detail=2247) (also with NuttX btw) and this should be done correctly and carefully otherwise we could end up with 3 or 4 not working lcd drivers. I will see how much time will I be able to put in this merge.
--
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] michallenc commented on a change in pull request #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
michallenc commented on a change in pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#discussion_r705970889
##########
File path: drivers/lcd/Kconfig
##########
@@ -619,15 +625,15 @@ if LCD_ST7789
---help---
Specifies the Y offset of the LCD.
- config LCD_ST7789_BPP
+ config LCD_ST77XX_BPP
Review comment:
Hi @gustavonihei I´ve just tried enabling both ST7735 and ST7789 and it configured and compiled NuttX without any problems. But when I run the fb example application it took the resolution of ST7735 instead of ST7789 because of those #ifdefs https://github.com/michallenc/incubator-nuttx/blob/st77xx/drivers/lcd/st77xx.c#L97.
I think the changes I did would not allow to use both 7735 and 7789 at the same time (because of resolution and also some other stuffs that are under ifdefs). It is a question whether we want to keep that option of using both controllers (I don´t know how much are two different displays used, haven´t seen much of those options myself) or to keep those controllers under one file.
--
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] michallenc commented on a change in pull request #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
michallenc commented on a change in pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#discussion_r709894470
##########
File path: drivers/lcd/Kconfig
##########
@@ -619,15 +625,15 @@ if LCD_ST7789
---help---
Specifies the Y offset of the LCD.
- config LCD_ST7789_BPP
+ config LCD_ST77XX_BPP
Review comment:
> @michallenc maybe you can define the CONFIG_LCD_ST77XX as main option and after enabling it the user can select (using choice) his model as ST7735 or ST7789
Hi @acassis, sorry for the late reply. I´ve already started with those changes you proposed, haven´t pushed them to GIT yet. It still does not solve the problem that it would not allow the user to use both ST7735 and ST7789 (or the driver mentiopned by @PeterBee97 below) drivers together (although I don´t think more than one display is usually used by the user, so it might not be such a problem).
--
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 #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
PeterBee97 commented on pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#issuecomment-920570670
Hi Michal, FYI I just opened a PR for adding GC9A01 driver, which is almost identical to ST7789 driver except for some init magic commands and different default orientation configs. You might want to take it into consideration when unifying the drivers :)
--
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 #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#discussion_r705528998
##########
File path: drivers/lcd/Kconfig
##########
@@ -619,15 +625,15 @@ if LCD_ST7789
---help---
Specifies the Y offset of the LCD.
- config LCD_ST7789_BPP
+ config LCD_ST77XX_BPP
Review comment:
I'm concerned about having two `config` options with the same identifier.
Have you tested enabling both `LCD_ST7735` and `LCD_ST7789`?
This is a valid scenario and I believe will probably result in a crash in `menuconfig`.
--
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 #4506: LCD: Merge files for ST7735 and ST7789 into ST77XX
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4506:
URL: https://github.com/apache/incubator-nuttx/pull/4506#discussion_r705528998
##########
File path: drivers/lcd/Kconfig
##########
@@ -619,15 +625,15 @@ if LCD_ST7789
---help---
Specifies the Y offset of the LCD.
- config LCD_ST7789_BPP
+ config LCD_ST77XX_BPP
Review comment:
I'm concerned about having two `config` options with the same identifier.
Have you tested enabling both `LCD_ST7735` and `LCD_ST7789`?
This is a valid scenario and I believe will probably result in error.
--
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