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

Automatic Resolution Detection #256

Open
wants to merge 5 commits into
base: 2.7.1-dev
Choose a base branch
from

Conversation

thebeline
Copy link
Contributor

Added support for OCTOSCREEN_RESOLUTION=AUTO which uses xrandr to detect the default display resolution automatically.

Additionally, added check and error reporting for minimum resolution requirements.

Kind of inspired by issues such as #246, but probably not exactly a resolution to those issues, as the screen it's self still needs to be configured correctly.

This has been compiled and is running on all of my systems and behaves as expected.

Added support for OCTOSCREEN_RESOLUTION=AUTO which uses `xrandr` to detect the default display resolution automatically.
@JeffB42
Copy link
Collaborator

JeffB42 commented Mar 23, 2021

@thebeline This looks interesting. I think it's too late for it to go into 2.7 (plus, it's labeled as experimental). Let's keep the PR open and I'll revisit this in during the development of 2.8.

@thebeline
Copy link
Contributor Author

@thebeline This looks interesting. I think it's too late for it to go into 2.7 (plus, it's labeled as experimental). Let's keep the PR open and I'll revisit this in during the development of 2.8.

I mean. To be fair, I was the one who tagged it experimental. 😏 I'm currently running it on a 3.5 (yes, one of the evil ones) and a 5 inch display. So far so good.

The only reason I tagged it experimental is because it would need to be tested by someone with a 7, 8, 10, 2.5, etc etc. Which I don't have. 🤔

That being said, I can't see a reason it would fail.

@thebeline
Copy link
Contributor Author

I spent some time considering failure states that would cause me concern, and I can only really think of 3:

  • xrandr is not installed or available
    This is installed by default, so unlikely, but I will add it to the dependencies to be sure.
  • The configured resolution of the display is less than the minimums
    This would exit the app with a miss-configured resolution error, as it would anyway. However, the check for this exits as a Fatal, so, on a fresh 3.5 with AUTO set and no drivers would loop. Which is less than ideal, so... That should probably be handled better.
  • The user has multiple screens installed and somehow they are not mirrored?
    I can't think of how this would occur, or even how to make if occur (for testing), but... maybe? Seems too edge-case to be a concern.

So, of those three concerns, the first is an easy fix, the third is so edge-case so as to be irrelevant, and the second... Well... Kind of relates to #261, and is ultimately outside the scope of this feature.

I will push a few changes shortly.

@JeffB42
Copy link
Collaborator

JeffB42 commented Mar 24, 2021

The user has multiple screens installed and somehow they are not mirrored?

Just a FYI, I've taken the position that only select hardware (RPi3 & RPi4 - no Pi-Zero) and configurations are supported (OctoPi only, no other distros). This also includes telling users that only a single display is supported, and OctoScreen needs to be the only GUI/X11 process running (so no Debian desktop).

@thebeline
Copy link
Contributor Author

The user has multiple screens installed and somehow they are not mirrored?

Just a FYI, I've taken the position that only select hardware (RPi3 & RPi4 - no Pi-Zero) and configurations are supported (OctoPi only, no other distros). This also includes telling users that only a single display is supported, and OctoScreen needs to be the only GUI/X11 process running (so no Debian desktop).

Well then it seems that edge-case is null, then. So, if the "supported" configurations are predictable, TBH, I would consider AUTO to be default. With manual setting for "unsupported edge-cases."

My thoughts

@thebeline
Copy link
Contributor Author

@JeffB42 - Merge conflict resolved. Thanks for the error handling bit, I was a little confused on how that worked in Go.

@thebeline thebeline changed the base branch from 2.7.0-dev to 2.7.1-dev March 29, 2021 14:02
@thebeline
Copy link
Contributor Author

@JeffB42 - I am rounding back to some of my work here, wanted to check before I merged master into my branches if the PRs and branches are even relevant any more. With regards to this branch, is this something I should test to be sure it behaves as described and re-consider? Or is this no longer relevant?

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.

2 participants