Fix size and orientation issues #9

brianmay · 2022-03-20T01:37:16Z

Fixes #7
Fixes #8

brianmay · 2022-03-20T01:40:09Z

src/lib.rs

@@ -64,8 +64,6 @@ where
 pub enum Orientation {
    Portrait = 0b0000_0000,         // no inverting
    Landscape = 0b0110_0000,        // invert column and page/column order


I would also be setting this to just 0b0010_0000 as the 2nd bit can be inverted if required.

I'm wondering if it wouldn't be more straightforward to have a flat enum for all the possible combinations rather than the "invert x/y".

That is going to be a lot of combinations. Basically there are 3 bits, so 222 = 8 combinations. Think it might be more confusing, not less confusing.

Although personally, wondering if it would be better just to have:

pub enum Orientation { Portrait, Landscape, }

And have the set_orientation function map that to the correct value.

Also worth noting, yes, we really do need to support all of 8 combinations. Depending on how the display is mounted and if the hardware reverses the image or not.

Hmm the main issue I have with this is that splitting the orientation enum and the "revert" values seem confusing. We could do something like

pub enum Orientation { Portrait(bool, bool), // x/y invert Landscape(bool, bool), // x/y invert }

Or possibly introduce a new sub-enum for "inversion" and use that as the inner value.

Hmmm. Not sure I can see how this could be less confusing. Also most of the code only needs to know if it is Portrait/Landscape. If the display is inverted or not makes no difference. i.e. the invert parameters are only really needed to configure the MADCTL register and don't matter afterward. In fact the invert parameters have no real meaning outside the MADCTL because there is no real way to know what the hardware is doing with them. Maybe setting invert_x will invert the display, or maybe setting invert_x will restore the display.

Maybe that is the problem though. We use the set_orientation to set parameters that are not orientation. You wouldn't set the RGB/BGR value here. But the problem being it is interconnected. When setting orientation you typically will (always) have to adjust the invert values at the same time. Which is why we do it in the set_orientation call.

almindor · 2022-03-26T19:39:00Z

Sorry about the delay, I need to get my setup in working order again after losing an mcu to a short. I should be able to test these in a week or so.

brianmay · 2022-03-31T21:44:28Z

Updated to remove merge conflicts. Change to clear no longer as invasive. Will update again once #14 is merged.

brianmay · 2022-04-01T05:21:10Z

Rebased to latest master.

src/graphics.rs

almindor · 2022-04-01T16:52:37Z

src/lib.rs

@@ -64,8 +64,6 @@ where
 pub enum Orientation {
    Portrait = 0b0000_0000,         // no inverting
    Landscape = 0b0110_0000,        // invert column and page/column order


I'm wondering if it wouldn't be more straightforward to have a flat enum for all the possible combinations rather than the "invert x/y".

src/graphics.rs

almindor · 2022-04-01T16:53:42Z

src/graphics.rs

@@ -67,10 +67,10 @@ where
    }

    fn fill_solid(&mut self, area: &Rectangle, color: Self::Color) -> Result<(), Self::Error> {
-        let fb_size = self.model.framebuffer_size();


I'm still conflicted about this. Framebuffer_size is supposed to be "the whole pixel memory" which in case of clear is what we want AFAICS.

Clear doesn't use fill_solid. At least not with this version. So nothing here is going to affect the clear function.

However I do have an alternative approach. Will add another commit, see if you like that any better.

I added a framebuffer_size function and use that now, hopefully this will address your concern here.

Having said that, there is still the problem that both "whole pixel memory" and "displayable area" both need to start at 0,0. I don't think the concept is very usable as is.

Fixes almindor#7 Fixes almindor#8

brianmay · 2022-04-01T23:10:02Z

src/lib.rs

@@ -242,6 +251,19 @@ where
            .map_err(|_| Error::DisplayError)
    }

+    fn framebuffer_size(&self) -> (u16, u16)


Thought about making this return a Size object. That would clarify which one is width vs height. But Size objects use u32 dimensions. And pixel_count in graphics.rs uses usize dimensions. And usize::from works fine for u16, but fails for u32. So I stuck with the above for now.

u32 as usize should be safe in 32bit+ machines of course there are very special edge cases (there's a whole thing with ptrsize vs usize in rust). for our purposes and expected display sizes I think it's 100% safe to just case that way.

almindor · 2022-04-04T15:38:57Z

@brianmay what is instead of having "two world" (model vs display/lib) we unify the framebuffer_size in the model by either enforcing the orientation knowledge in the model, or by making it into fn frabebuffer_size(&self, orientation: Orientation) (provided Orientation has all the required info as I'd like).

This way we can keep a single source of size info. Same can be done for display_size then.

brianmay · 2022-04-04T21:59:54Z

@brianmay what is instead of having "two world" (model vs display/lib) we unify the framebuffer_size in the model by either enforcing the orientation knowledge in the model, or by making it into fn frabebuffer_size(&self, orientation: Orientation) (provided Orientation has all the required info as I'd like).

This way we can keep a single source of size info. Same can be done for display_size then.

I would be happy with both of these approaches. At present the model has no state information, the display is the authoritative source for all information. We probably should keep this unless we have a good reason to change it (e.g. if RGB/BGR config requires it). If so, then the later option would suffice. Also note that we only need to know the Portrait/Landscape stuff, we don't need to know bout the invert stuff here.

almindor

I'm going to merge this and then follow up with a PR to clarify with the orientation being pushed into Model::*_size() functions instead.

brianmay commented Mar 20, 2022

View reviewed changes

rfuest mentioned this pull request Mar 21, 2022

Orientation embedded-graphics/embedded-graphics#647

Open

brianmay force-pushed the fix_orientation_size branch from db1dea2 to 0a08c13 Compare March 31, 2022 21:41

almindor mentioned this pull request Mar 31, 2022

keep MADCTL value to use with orientation changes #14

Merged

brianmay force-pushed the fix_orientation_size branch from 0a08c13 to b6e2e0b Compare April 1, 2022 05:20

brianmay commented Apr 1, 2022

View reviewed changes

src/graphics.rs Show resolved Hide resolved

brianmay force-pushed the fix_orientation_size branch from b6e2e0b to 76cb310 Compare April 1, 2022 05:43

almindor reviewed Apr 1, 2022

View reviewed changes

brianmay added 2 commits April 2, 2022 09:51

Fix size and orientation issues

98e64a6

Fixes almindor#7 Fixes almindor#8

Fix screen dimensions for fill_solid

814429a

brianmay force-pushed the fix_orientation_size branch from 76cb310 to 7d21bbf Compare April 1, 2022 22:52

Implement framebuffer_size function

c7c0ee3

brianmay force-pushed the fix_orientation_size branch from 7d21bbf to c7c0ee3 Compare April 1, 2022 23:09

brianmay commented Apr 1, 2022

View reviewed changes

almindor approved these changes Apr 4, 2022

View reviewed changes

almindor merged commit da7d7f8 into almindor:master Apr 4, 2022

brianmay deleted the fix_orientation_size branch April 4, 2022 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix size and orientation issues #9

Fix size and orientation issues #9

brianmay commented Mar 20, 2022

brianmay Mar 20, 2022

almindor Apr 1, 2022

brianmay Apr 1, 2022 •

edited

Loading

brianmay Apr 1, 2022

almindor Apr 2, 2022

brianmay Apr 4, 2022

almindor commented Mar 26, 2022

brianmay commented Mar 31, 2022

brianmay commented Apr 1, 2022

almindor Apr 1, 2022

almindor Apr 1, 2022

brianmay Apr 1, 2022

brianmay Apr 1, 2022

brianmay Apr 1, 2022

brianmay Apr 1, 2022

almindor Apr 4, 2022

almindor commented Apr 4, 2022

brianmay commented Apr 4, 2022

almindor left a comment

Fix size and orientation issues #9

Fix size and orientation issues #9

Conversation

brianmay commented Mar 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmay Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almindor commented Mar 26, 2022

brianmay commented Mar 31, 2022

brianmay commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almindor commented Apr 4, 2022

brianmay commented Apr 4, 2022

almindor left a comment

Choose a reason for hiding this comment

brianmay Apr 1, 2022 •

edited

Loading