-
Notifications
You must be signed in to change notification settings - Fork 627
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
esp-idf: Add support for RGB888 displays #7301
base: master
Are you sure you want to change the base?
Conversation
... by making SlintPlatformConfiguration a template and using a deduction guide to make it source compatible (Olivier's magic :)
Use Olivier's great trick to rename color_swap_16 to color_swap via union.
/// Swap the 2 bytes of RGB 565 pixels before sending to the display. Use this | ||
/// if your CPU is little endian but the display expects big-endian. | ||
bool color_swap_16 = false; | ||
/// Swap the 2 bytes of RGB 565 pixels before sending to the display, or turn RGB into BGR. Use |
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.
/// Swap the 2 bytes of RGB 565 pixels before sending to the display, or turn RGB into BGR. Use | |
/// Swap the 2 bytes of RGB 565 pixels before sending to the display, or turn 24bit RGB into BGR. Use |
We don't generate documentation for this, right?
If we do, I wonder if this documentation shouldn't need to go to the new name.
But in the header it looks better like this, that's true.
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.
Right, we don't generate documentation for this at all yet. It's not integrated into any of your docs :(
@@ -92,4 +99,5 @@ void slint_esp_init(slint::PhysicalSize size, esp_lcd_panel_handle_t panel, | |||
* | |||
* This must be called before any other call to the Slint library. | |||
*/ | |||
void slint_esp_init(const SlintPlatformConfiguration &config); | |||
void slint_esp_init(const SlintPlatformConfiguration<slint::platform::Rgb565Pixel> &config); | |||
void slint_esp_init(const SlintPlatformConfiguration<slint::Rgb8Pixel> &config); |
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.
Should there be documentation for the new function?
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 mean, I can copy it, but it seems pointless for what's an overload. But when we generate docs in the future, we need to make sure that it shows up as one function or two with the same docs.
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.
Not sure if good idea, but maybe we could have
struct EspPlatformBase : public slint::platform::Platform {
// ... Most of the code and members
virtual void do_render() = 0;
};
So the part that is duplicated would be smaller.
That is usefull if the linker can't get rid of the duplication. Although we're not talking about so much code.
No description provided.