Orientation

[rkmiec]

I'm working on some risc-v based board with a st7789 display.
The display driver provides the possibility to change the orientation in one of 4 modes: Landscape, LandscapeSwapped, Portrait, PortraitSwapped.
It has the possibility to change it on the fly. I have seen some other libraries do this as well.
Now my questions are: why each library provides this on his own? why there is no common Trait for handling it in the core library?
Should then the display size dimension be swapped?

In addition, the st7789 requires some offset between RAM memory and the display size (if display size is not 320x240). I implemented it on my own for this specific driver as the interface for the user to provide his own values or get the numbers calculated automatically. Is there any way to get such interfaces in the embedded-graphics-core as well?

[rfuest]

Now my questions are: why each library provides this on his own? why there is no common Trait for handling it in the core library?

Most display driver crates can also be used without embedded-graphics support by disabling a cargo feature. This means that they need to implement the orientation support without using any e-g types.

But providing a more consistent driver interface would is a good idea IMO. Adding a flush method has also come up multiple times. Perhaps these features could be implemented implemented in a separate lower level crate which is shared by the display driver crates, similar to what display-driver has done. I'm not too involved with the driver side of e-g and maybe @jamwaffles or @almindor can offer their opinion on that matter.

Should then the display size dimension be swapped?

Yes, the display size should always reflect the drawable area.

In addition, the st7789 requires some offset between RAM memory and the display size (if display size is not 320x240). I implemented it on my own for this specific driver as the interface for the user to provide his own values or get the numbers calculated automatically. Is there any way to get such interfaces in the embedded-graphics-core as well?

This is even lower level than the orientation support and doesn't belong in e-g IMO. But it might be a candidate for inclusion in the lower level crate I mentioned above.

[almindor]

I always found the orientation confusing myself. I sort of inherited the concept from the original ST7789 driver writer when I took over and kept it for backwards compat. Having some sort of standard might make sense but we need to be careful, there might be some crazy edge cases we could lock out. Perhaps including a Custom enum variant to cover anything like that.

[brianmay]

In this PR I attempted to come up with a better, more general, set_orientation function call:

pub fn set_orientation(&mut self, orientation: Orientation, invertx: bool, inverty: bool) -> Result<(), Error<RST::Error>> {

Where Orientation is an enum can either be Landscape or Portrait.

This fixes the problem with the previous implementation where the image can be reversed on the screen but the API didn't allow inverting it.

I would like to see a general flush method added to the DrawTarget interface. Dirvers that don't need/support it can implement it is a NOP.

While I am on the topic, another one I would like to see is a trait with a method to turn on/off the display. Where this is possible. As I like to be able to turn off my screens at night based on common code that should be able to work with any disable with this functionality.

[rfuest]

In this PR I attempted to come up with a better, more general, set_orientation function call:

I guess you are talking about this PR? almindor/mipidsi#9

[brianmay]

Yes, sorry, I fail at copy and paste.

[VersBinarii]

I also think that having a common Orientation enum in e-g would be useful.
I have a display driver and a touch controller driver that both have to define their own Orientation enum. It would be nice to be able to unify them.

[rfuest]

Does anyone have a good idea for the names of an generic Orientation enum? Portrait and Landscape are good for rectangular display, but don't make sense for square or circular ones.

We could define one orientation as the base orientation, which would need to defined in the display driver docs, and use degrees to describe the other orientations:

enum Orientation {
    Deg0,
    Deg90,
    Deg180,
    Deg270,
}
// or
enum Orientation {
    Deg0,
    Deg90Cw,
    Deg90Ccw,
    Deg180,
}

[brianmay]

Have a look at https://github.com/almindor/mipidsi/blob/9b06028fe5169c7068b65b2d886e9456d360f85f/src/lib.rs#L61-L74

Here we decided that we also needed a boolean mirror flag, because some devices are wired up in such a way that the image is displayed mirrored, and some devices are not. Not sure how applicable this is for other chipsets.

[rkmiec]

If you define your screen with orientation Orientation::Deg0 and size 320x240 then is it the same as Orientation::Deg90 with size 240x320?

I had some issues implementing the rotation, if the user can swap the X, Y dimensions when creating an instance.
Adding a fact, that odd size, like 135 may have to be divided in the memory to screen mapping. Because of that rotation::Deg90Cw and rotation::Deg90Ccw positions were off by one pixel.

[rfuest]

Here we decided that we also needed a boolean mirror flag, because some devices are wired up in such a way that the image is displayed mirrored, and some devices are not. Not sure how applicable this is for other chipsets.

To be honest I don't like that at all, especially with a bool parameter. The difference between Orientation::Portrait(true) and Orientation::Portrait(false) isn't apparent without reading the documentation.

[rfuest]

If you define your screen with orientation Orientation::Deg0 and size 320x240 then is it the same as Orientation::Deg90 with size 240x320?

Yes, if one display is 320x240 at Deg0 and the other is 240x320 as Deg0. Changing the orientation of the first display to Deg90 would present the display as a draw target with Size::new(240, 320) to the user. Which is the same as the second displays draw target size.

[brianmay]

To be honest I don't like that at all, especially with a bool parameter.

A previous approach of mine was to pass another parameter to the set_orientation() method for for the mirror flag. What was implemented was the preferred solution by the maintainer.

An important question I think: Is mirroring in scope for a common API to set orientation? I tend to think it might be a good idea. But we might need to have some allowances in case the hardware does not support it (e.g. by ignoring the invalid request or producing a defined error for this situation).

[almindor]

To be honest I don't like that at all, especially with a bool parameter.

A previous approach of mine was to pass another parameter to the set_orientation() method for for the mirror flag. What was implemented was the preferred solution by the maintainer.

An important question I think: Is mirroring in scope for a common API to set orientation? I tend to think it might be a good idea. But we might need to have some allowances in case the hardware does not support it (e.g. by ignoring the invalid request or producing a defined error for this situation).

The reason for this was that decoupling the mirror info from the Orientation resulted in two sources of truth between orientations set on init vs the set_orientation. And having the extra bool outside just felt wrong.

I'm not happy with the current state either tho but don't know of how to make it "elegant yet complete".

[rfuest]

The reason for this was that decoupling the mirror info from the Orientation resulted in two sources of truth between orientations set on init vs the set_orientation. And having the extra bool outside just felt wrong.

I'm not happy with the current state either tho but don't know of how to make it "elegant yet complete".

I agree that an extra bool argument for set_orientation feels wrong. What do you mean by "two sources of truth" in this context? How is setting the orientation in init different from set_orientation? I'm not familiar with the implementation in the mipidsi crate.

Or do you mean that there would be multiple representations of the same orientation? Like Landscape is the same as LandscapeInverted + both axis mirrored. But this wouldn't be a problem if there would only be one additional set_mirrored(bool) setting and not one per axis.

[almindor]

I agree that an extra bool argument for set_orientation feels wrong. What do you mean by "two sources of truth" in this context? How is setting the orientation in init different from set_orientation? I'm not familiar with the implementation in the mipidsi crate.

Or do you mean that there would be multiple representations of the same orientation? Like Landscape is the same as LandscapeInverted + both axis mirrored. But this wouldn't be a problem if there would only be one additional set_mirrored(bool) setting and not one per axis.

What I mean is that if the mirrored information is not part of Orientation but an external flag it becomes unwieldy. E.g. init is given config options with Orientation in it but then how do you tell it about the mirroring? set_orientation ends up having additional bools or enums etc. It just ends up being disjointed.

Regardless of how we want the Orientation enum to be tho I think it should live in a general abstraction crate rather than in individual drivers.

[rfuest]

What I mean is that if the mirrored information is not part of Orientation but an external flag it becomes unwieldy. E.g. init is given config options with Orientation in it but then how do you tell it about the mirroring?

DisplayOptions could use orientation: Orientation and mirrored: bool fields and Display would have set_orientation and set_mirrored setters. IMO mirroring is a far less commonly used option and splitting it into a separate bool makes it easier to ignore it if you don't need that feature, e.g. if you initialize DisplayOptions with the struct update syntax.

[almindor]

What I mean is that if the mirrored information is not part of Orientation but an external flag it becomes unwieldy. E.g. init is given config options with Orientation in it but then how do you tell it about the mirroring?

DisplayOptions could use orientation: Orientation and mirrored: bool fields and Display would have set_orientation and set_mirrored setters. IMO mirroring is a far less commonly used option and splitting it into a separate bool makes it easier to ignore it if you don't need that feature, e.g. if you initialize DisplayOptions with the struct update syntax.

That'd work. I don't like it but it'd work :) I'm not going to block either solution as long as we can unify the Orientation (in any form that is complete) into embedded-graphics somewhere so it can be unified accross drivers.

[rfuest]

That'd work. I don't like it but it'd work :)

I'm always open to better suggestions 😉.

We could also use an Orientation struct to combine the rotation and mirroring settings:

pub enum Rotation { Deg0, Deg90, Deg180, Deg270 }

pub struct Orientation {
    pub rotation: Rotation,
    pub mirrored: bool,
}

impl Orientation {
    /// Creates a non mirrored display orientation.
    pub const fn new(rotation: Rotation) -> Self {
        Self { rotation, mirrored: false }
    }

    /// Creates a display orientation with mirrored X axis.
    pub const fn new_mirrored(rotation: Rotation) -> Self {
        Self { rotation, mirrored: true }
    }
}

// Usage:

display.set_orientation(Orientation::new(Rotation::Deg0));
display.set_orientation(Orientation::new_mirrored(Rotation::Deg0));

// Or if you prefer struct literals:

display.set_orientation(Orientation { rotation: Rotation::Deg0, mirrored: false }));

[almindor]

I'm always open to better suggestions wink.

We could also use an Orientation struct to combine the rotation and mirroring settings:

pub enum Rotation { Deg0, Deg90, Deg180, Deg270 }

pub struct Orientation {
    pub rotation: Rotation,
    pub mirrored: bool,
}

impl Orientation {
    /// Creates a non mirrored display orientation.
    pub const fn new(rotation: Rotation) -> Self {
        Self { rotation, mirrored: false }
    }

    /// Creates a display orientation with mirrored X axis.
    pub const fn new_mirrored(rotation: Rotation) -> Self {
        Self { rotation, mirrored: true }
    }
}

// Usage:

display.set_orientation(Orientation::new(Rotation::Deg0));
display.set_orientation(Orientation::new_mirrored(Rotation::Deg0));

// Or if you prefer struct literals:

display.set_orientation(Orientation { rotation: Rotation::Deg0, mirrored: false }));

That seems pretty nice. Do we put it into e-g or e-g core ?

[rfuest]

That seems pretty nice. Do we put it into e-g or e-g core ?

I'm not sure yet, maybe neither.

A Rotation enum for rotation angles, that are a multiple of 90°, will be added to e-g, because this enum is also necessary to implement rotation for draw targets and primitives (#661, #321 (comment)).

But I'm not sure if we need the Orientation struct in e-g itself. I was thinking about creating a display-driver-hal crate to bundle some common display related traits/enums which are slightly outside e-g's scope.

The main trait in that crate would contain common functions for displays, which aren't part of the DrawTarget trait:

trait HalDisplay {
    // For drivers that draw directly to the framebuffer inside the display controller `flush` is a noop.
    // By using another error type for `flush` it can be set to `Infallible` in that case.
    type FlushError;

    // Error type for all other operations that communicate with the display.
    type Error;

    // Updates the display.
    // The default noop impl can be overridden if the display needs to be flushed.
    fn flush(&mut self) -> Result<(), Self::FlushError> {
        Ok(())
    }
    
    // Sets the orientation.
    // The error is wrapped in a `NotSupportedError` enum to notify the user if setting the orientation isn't supported.
    fn set_orientation(&mut self, orientation: Orientation) -> Result<(), NotSupportedError<Self::Error>> {
        Err(NotSupportedError::NotSupported);
    }

    // Returns the current orientation.
    // The method does directly return an `Orientation` instead of a `Result` because if the orientation can't
    // be changed it will always stay in the default orientation. But this also makes the assumption that
    // the driver knows the current orientation without communication with the display.
    fn orientation(&self) -> Orientations {
        Orientation::default()
    }

    // TODO: other methods?
}

enum NotSupportedError<E> {
    NotSupported,
    Display(E)
}

Some questions remain:

• Are there other methods this trait should contain?
  • A display on/off command is supported by many controllers.
  • Backlight control could be useful, but is often handled by a GPIO pin and not the display controller
    We could provide a wrapper around a HalDisplay to add backlight functionality to HalDisplays without backlight functions:

    struct GpioBacklight<D, P> {
        display: D,
        backlight_pin: P,
        backlight_current_state: BacklightState,
        backlight_on_state: PinState,
    }
    
    impl<D: HalDisplay, P: OutputPin> HalDisplay for GpioBacklight<D, P>{
       // ...
       // The impl forwards all methods, except the backlight methods to `self.display`.
       // ...
    
       fn set_backlight(&mut self, state: BacklightState) -> Result<(), Self::BacklightError> {
           match state {
               BacklightState::Off => self.backlight_pin.set_state(!self.backlight_on_state)?,
               BacklightState::On => self.backlight_pin.set_state(self.backlight_on_state)?,
           }
           self.backlight_current_state = state;
    
           Ok(())
       }
    
       fn backlight(&self) -> BacklightState {
           self.backlight_current_state
       }
    }
    
    enum BacklightState { On, Off }

    But the backlight could also be controlled by a PWM output.
• As @brianmay noticed we need to consider that some controllers might only support rotation or mirroring and not both. We would either need to add finer grained error reporting to set_orientation or use set_rotation and set_mirrored.

[brianmay]

Supporting back-lighting would definitely be a good thing for me. Although not 100% certain I can visualize exactly how the wrapper would work. Would D need to require HalDisplay?

Taking a step back, checking my understanding, the HalDisplay trait would have methods for:

• flush
• set_mirror
• set_orientation
• set_backlight
• Functions to retrieve current mirror, orientation, backlight state.

[rfuest]

Supporting back-lighting would definitely be a good thing for me. Although not 100% certain I can visualize exactly how the wrapper would work. Would D need to require HalDisplay?

Yes, in my example the impl requires D: HalDisplay. From a user perspective it would work like this:

let display = Display::st7789(di, rst);
// The returned `display` would implement `HalDisplay`,
// but `set_backlight` isn't supported and returns `NotSupportedError::NotSupported`

display.init(...)?;

// Add active high GPIO backlight.
let display_with_backlight = GpioBacklight::new(display, gpio_pin, PinState::High);
// `display_with_backlight` supports `set_backlight` by setting the GPIO pin.
// All other method calls are passed though to `display`.

// There would need to be some way of accessing the original `display` for methods which aren't part of `HalDisplay`.
// For example:
display_with_backlight.as_inner_mut().set_tearing_effect(TearingEffect::Vertical)?;

Taking a step back, checking my understanding, the HalDisplay trait would have methods for:
* flush
* set_mirror
* set_orientation
* set_backlight
* Functions to retrieve current mirror, orientation, backlight state.

IMO the trait would either have set_rotation and set_mirror(ed) or set_orientation. And there are some other common controller features we could add, like display on/off or display inversion.

[DrTobe]

Hi, I got here because I was confused that the DrawTarget trait does not contain a flush method which seems essential for many display drivers, especially the ones that use an internal framebuffer. The documentation shows that these cases have been considered but the method is missing nonetheless.

Now I am happy to see these efforts here to change that but I am not sure if the trait suggested above is the right approach: In our projects, we try to keep hardware-specific details in the board-support layer and add a hardware-independent application on top. From my understanding, setting orientation or a mirrored flag are hardware-specific details but flushing or controlling the backlight are controlled by the hardware-independent application logic. I consider flushing to be part of the hardware-independent part because the application logic knows when a new frame has been completely written which is when flush has to be called again.

One could argue that rotation and mirroring could be controlled by the application logic, too, but this only holds true if the application wants to display something upside-down or mirrored. In the case you have been discussed above (e.g. display is assembled upside-down), this is something hardware-specific which should be compensated without the application logic knowing about that detail.

[rfuest]

That's a valid point. I'm still not totally convinced that flush belongs into DrawTarget, but perhaps it's the easiest solution. If DrawTarget would only be used for displays I wouldn't hesitate to add a flush method, but for DrawTarget adapters, like Clipped (https://docs.rs/embedded-graphics/latest/embedded_graphics/draw_target/trait.DrawTargetExt.html#tymethod.clipped) the way flush needs to be implemented isn't clearly defined. All other methods of a clipped draw target only affect the clipping region, but if flush simply calls the parent's flush method other areas of the display could also be affected. This might be a contrived example and flush is normally only called for the display driver draw target, but this is something that needs to be considered IMO.

[mchodzikiewicz]

I am not a fan of having flush() as a DrawTarget method even if it was only for displays, from the perspective of some crate that draws a lot of primitives and want to "publish" them only after finishing drawing a set, I would like to ensure that target properly supports flushing by constraining display type like this:

fn draw<D>(&self, display: &mut D) -> Result<...>
where
    D: DrawTarget<Color = C> + FlushTarget,
{
    ...
}

having the flush() embedded in a DrawTarget trait would likely lead to some stub implementations in existing drivers - it would not be detectable from a crate that implements new drawables

[mchodzikiewicz]

Also, another place where flush() would be usable is when you implement an inter-thread facade and messaging each primitive on its own gives a lot of overhead - generating a message on flush() solves this problem.

Related comment: oxidecomputer/hubris#484 (reply in thread)