Skip to content
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

Refactor modm:ui:display #604

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Conversation

TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Apr 2, 2021

Followup from #601

Color has nothing to do in any of the MonochromeGraphicDisplay but it inherits Color from GraphicDisplay. For MonochromeGraphicDisplay it shouldn't be color but pixel On / Off.

  • LCD pixels will be blackish on light background. Background color is a hardware property.
  • OLED pixels will be light on black background. Pixel color is a hardware property.

New inheritances

GraphicDisplay <-- ColoredGraphicDisplay (Color stuff in here) <-- ILI9341, ,,,
GraphicDisplay <-- MonochromeGraphicDisplay (Light / dark stuff in here) <-- SSD1306, KS0108, ...

  • New specialised interface GraphicDisplay <- ColoredGraphicDisplay; move color-stuff in there
    • Test Displays inheriting ColoredGraphicDisplay
  • New specialised interface GraphicDisplay <- MonochromeGraphicDisplay
    • Test Displays inheriting MonochromeGraphicDisplay
    • Implement optimized drawVerticalLine for MonochromeVerticalDisplay
  • Modernize code
  • Update docs

@TomSaw TomSaw force-pushed the refactor-GraphicDisplay branch 3 times, most recently from e78637f to 5db4730 Compare April 2, 2021 15:13
@TomSaw TomSaw force-pushed the refactor-GraphicDisplay branch 6 times, most recently from e4a48a7 to 496c3ab Compare April 7, 2021 06:09
@TomSaw TomSaw force-pushed the refactor-GraphicDisplay branch 2 times, most recently from 6790a1e to 7119514 Compare April 13, 2021 17:34
@TomSaw TomSaw changed the title Refactor GraphicDisplay Refactor GraphicDisplay and Color Apr 14, 2021
@TomSaw TomSaw force-pushed the refactor-GraphicDisplay branch 4 times, most recently from beacdcc to 64f9d4f Compare April 16, 2021 18:25
@TomSaw TomSaw changed the title Refactor GraphicDisplay and Color Refactor GraphicDisplay Apr 16, 2021
@TomSaw
Copy link
Contributor Author

TomSaw commented Apr 16, 2021

  • Due to the work on graphicDisplay i also consequently dug into refactoring color.hpp.
  • Since i have a ILI9341 around, i found it does not work properly and started touching it's driver too.

-> I'm going to create PRs for the two 🙄👆 but let's first get this PR shipped.

All interfaces remain the same, except that you have to replace pointers GraphicDisplay* -> ColorGraphicDisplay* in your source if you use any of setColor(...) / setForgeground / setBackground.

"Color"-control for monochrome Displays is a thing, i want to implement after my planned refactor-color PR is merged (or not ;P) so I struck it from here for now.

Would someone please review the commit !? @salkinium ? @rleh ? @chris-durand ?

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far so good!

src/modm/ui/display/graphic_display.hpp Outdated Show resolved Hide resolved
src/modm/ui/display/monochrome_graphic_display.hpp Outdated Show resolved Hide resolved
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ext/modm-devices submodule shouldn't be modified

@TomSaw TomSaw force-pushed the refactor-GraphicDisplay branch 4 times, most recently from e68a1ac to 69b2f42 Compare April 17, 2021 18:12
@TomSaw
Copy link
Contributor Author

TomSaw commented Apr 17, 2021

All done. Thanks @chris-durand and @salkinium for your always enriching comments. I did a few more addition improvements you propably want to check.

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it a lot!

src/modm/ui/display/color_graphic_display.hpp Outdated Show resolved Hide resolved
src/modm/ui/display/color_graphic_display.hpp Outdated Show resolved Hide resolved
{
const int16_t y = start.y / 8;

// TODO Implement draw / clear pixels for monochrome displays
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this?

Copy link
Contributor Author

@TomSaw TomSaw Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Postponed. I'm not clear about a good solution yet. nothing is lost, it hasn't worked before.

There's some stuff that interacts with foreground and background color, like writing characters. Also there's already setPixel(), clearPixel()
I fell like there could be a general solution that works with color and monochrome uniformly. but I want to cleanup color first #616 and afterwards spent some time on the ili9341 driver.
When i have nice color-class and a good to read ili9341 driver i want to dive into setting and clearing pixels ;)

Copy link
Contributor Author

@TomSaw TomSaw Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible solution: Define modm::color::Grayscale witch stores one byte with grayscale value.
For Monochrome, we only need a a boolean - storing Pixel On/Off - nut boolean will anyways consume a full byte.
So why not just using the Grayscale here too? If (value & 0x80) -> Monochrome pixel is On

All the color-types and diplays will be very compatible to each other. You will be able to use the same functions to write a colored image on a color, grayscale or monochrome display (very simplified) if you need to.

class modm::color::Grayscale{
  uint8_t value; // Grayscale value
};

But yea... that's just one idea

Copy link
Member

@salkinium salkinium Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 8-bit grayscale is fine, since it's only an intermediate value anyways and it's a simple solution.
You need have an optimized image format for binary images anyways.

You can convert RGB into grayscale using this formula, however that doesn't necessarily makes sense without dithering (which definitely needs to be done beforehand).
https://github.com/salkinium/upainter/blob/master/doc/surface/README.md#8bpp-grayscale

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I check this tomorrow :)

@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label Apr 18, 2021
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update docs? I'm happy with the rest. (and squash pls).

@TomSaw TomSaw force-pushed the refactor-GraphicDisplay branch 2 times, most recently from 04ac120 to f27abbd Compare April 20, 2021 13:45
@TomSaw TomSaw changed the title Refactor GraphicDisplay Refactor modm:ui:display Apr 21, 2021
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more touches:

src/modm/driver/display/ili9341.hpp Outdated Show resolved Hide resolved
src/modm/ui/display/color_graphic_display.hpp Outdated Show resolved Hide resolved
src/modm/ui/display/graphic_display.hpp Outdated Show resolved Hide resolved
src/modm/ui/display/graphic_display.hpp Show resolved Hide resolved
@TomSaw
Copy link
Contributor Author

TomSaw commented Apr 21, 2021

Merge?

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this good cleanup! Let's squash your commits into one or more with good messages, then we can merge this!

src/modm/ui/display/graphic_display.hpp Show resolved Hide resolved
@TomSaw TomSaw force-pushed the refactor-GraphicDisplay branch 3 times, most recently from e918ee1 to 8b022a3 Compare April 22, 2021 05:00
@salkinium salkinium added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Apr 22, 2021
@salkinium salkinium added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Apr 22, 2021
@salkinium salkinium merged commit 295dbc3 into modm-io:develop Apr 22, 2021
@TomSaw TomSaw deleted the refactor-GraphicDisplay branch April 22, 2021 17:50
@TomSaw
Copy link
Contributor Author

TomSaw commented Apr 23, 2021

That was a nice warmup. Thanks @salkinium 🌻 and @chris-durand 🌻 for taking me by the hand until these pleassent sessions.

By the time (and of course due to the cleanification) i gained more overview about the display API. I did not really wanted to warp all around but... now has taken me over. After some polishing of ssd1306 and ili9341, witch i use as reference hardware, i would love to present plans for and discus "refactor modm:ui:display part 2" ;).

Enjoy the days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advanced 🤯 ci:hal Triggers the exhaustive HAL compile CI jobs refactor 🔥
Development

Successfully merging this pull request may close these issues.

None yet

3 participants