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

WIP: LCD display rotation #98

Closed

Conversation

rafacouto
Copy link
Contributor

@rafacouto rafacouto commented Oct 12, 2020

Implementing a new feature to support left/right-handed option.

WIP:

  • Use MADCTL register on ST7789 to use 0/180 degrees rotation.
  • Invert touchpad x-y coordinates when usage is rotated.
  • Review scrolling to change the extra display buffer offset.
  • RotationController to keep the state of face rotation.
  • Add the "Wear on right/left" option to the user interface.
  • Save the user option in non-volatile memory.

@JF002
Copy link
Collaborator

JF002 commented Oct 18, 2020

Hi @rafacouto and thanks for showing your progression through this WIP PR, it looks promising!

I was wondering if it would be possible to implement part of this functionality in the drivers (ST7789 and CST816S) so that DisplayApp does not have to worry about configuring the LCD each time it goes out of sleep, and about coordinates translations.

If the drivers are configured (via a parameter invert or similar) :

  • the driver for st7789 will be able to set the lcd orientation automatically in the WakeUp() method
  • the driver for cst816s will be able to convert the coordinates before returning them the the DisplayApp.

What is your opinion about this?

@rafacouto
Copy link
Contributor Author

Currently wakeup event on ST7789 is using MADCTL register to use the right orientation, no problem or overhead on this. CST816S events are received by interrupt and processed on DisplayApp; IMO it is better to transform coordinates and scrolling out of the interrupt handler (driver space), though it's possible to set there.

I think LCD orientation is a property/behaviour of DisplayApp because it also affects touchpad X-Y mapping, LVGL scrolling, menu settings to change orientation and, in short future, NVM to persist user option between reboots.

Anyway, I understand you are looking for a more readable code to manage this transversal feature. Maybe a refactoring of DisplayApp will be more convenient to deal with your question. For that time we could considere a DRE, Data Runtime Environment accesible by any subsystem, to address the orientation setting (or any variable involving several subsystems) and to simplify the object-methods calling.

@JF002
Copy link
Collaborator

JF002 commented Oct 18, 2020

You are right, I read the code too fast and mixed SetHandOrientation() with SetScreenCoordinates(), and the MADCTL is already encapsulated into the ST7789 driver!

And I think we could do the same for the touch panel driver : coordinate translation could be done in Cst816S::GetTouchInfo() (not in the IRQ) so that we don't have to remember to translate them using SetScreenCoordinates() in DisplayApp each time we read touch events from the driver. Cst816S::GetTouchInfo() would be able to convert X/Y coordinates and even invert swipe up/down/left/right gestures.
The touch panel driver could be configured in SetHandOrientation() like it's already done for the lcd.

I agree with you, configuring the hand orientation is the responsibility of DisplayApp (and it could be configured with a user setting stored in non volatile memory in the future).

I don't know the concept of DRE, but it looks like the component/controller concept I implemented in the project. The goal of these controllers is to provide a high level API between the application (DisplayTask) and the drivers. They are available to any class that have a reference to them.
BUT... it's true that the lcd and touchpanel drivers are directly integrated into DisplayApp, without that 'controller' layer...

@rafacouto
Copy link
Contributor Author

rafacouto commented Nov 11, 2020

I having problems to manage the scrolling routines code. At this moment, display rotation and touchpad references are working by disabling the scrolling FX.

There are other ST7789 interesting registers like PTLAR (Partial Area) that could help on scrolling so, I've planned to study the hardware scroll in deep and probably do some refactoring on LittleVgl::FlushDisplay.

You are right, a RotationController should also be implemented to keep the state of face rotation and communicate with the affected components (display, touchpad, ...). Extra checkpoint in this WIP was added to achieve it.

@redengin
Copy link

rather than making each implementer of "scrolling" behaviors have to comprehend the orientation, I promote that "scrolling" should be abstracted into the Display controller.

@Riksu9000
Copy link
Contributor

There's nothing stopping people from putting the watch on their right arm, but what this feature really is about is being able to choose which side the button is on regardless of which arm it is strapped to.

This would be neat if the PineTime was symmetrical. Unfortunately it isn't and it would probably look weird to wear it upside down.

@Riksu9000 Riksu9000 marked this pull request as draft October 31, 2021 09:24
@ck-telecom
Copy link

You are right, I read the code too fast and mixed SetHandOrientation() with SetScreenCoordinates(), and the MADCTL is already encapsulated into the ST7789 driver!

And I think we could do the same for the touch panel driver : coordinate translation could be done in Cst816S::GetTouchInfo() (not in the IRQ) so that we don't have to remember to translate them using SetScreenCoordinates() in DisplayApp each time we read touch events from the driver. Cst816S::GetTouchInfo() would be able to convert X/Y coordinates and even invert swipe up/down/left/right gestures. The touch panel driver could be configured in SetHandOrientation() like it's already done for the lcd.

I agree with you, configuring the hand orientation is the responsibility of DisplayApp (and it could be configured with a user setting stored in non volatile memory in the future).

I don't know the concept of DRE, but it looks like the component/controller concept I implemented in the project. The goal of these controllers is to provide a high level API between the application (DisplayTask) and the drivers. They are available to any class that have a reference to them. BUT... it's true that the lcd and touchpanel drivers are directly integrated into DisplayApp, without that 'controller' layer...

you could refer Zephyr LVGL handling with screen rotation in lib/gui/*

@Riksu9000
Copy link
Contributor

Please open a new PR if you intend to keep working on this.

@Riksu9000 Riksu9000 closed this Jan 21, 2022
@JF002 JF002 mentioned this pull request Apr 24, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants