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

Display inherits from base display #55

Merged
merged 4 commits into from
Oct 31, 2023
Merged

Conversation

Ptosiek
Copy link
Contributor

@Ptosiek Ptosiek commented Oct 30, 2023

Hey,

Small code reorg that I had in a branch. Kinda similar to GPS refactor, it simplifies a bit the code. but the 'config' object is still passed down (and modified)
To be fair, it can probably go a bit further, the 'blockers' are the reading G_QUIT, and G_AUTOBACKLIGHT (which is even modified).

  1. Is checking G_QUIT in update still necessary ? What is it needed ?
  2. I don't really get the code of change_brightness in MIP display (and can't test it). How come calling change_brightness can switch G_USE_AUTO_BACKLIGHT on /off?

Simplify code a bit
Move display related conf out of config object
Same order of execution as the official code.

>>> loop = QEventLoop(app)
>>> asyncio.set_event_loop(loop)
Copy link
Owner

@hishizuka hishizuka Oct 30, 2023

Choose a reason for hiding this comment

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

If in X environment, send is not necessary. No buffer calculation is required.

# default display (X window)
class Display:
    has_color = True
    has_touch = True
    send = False #not True

or

def init_display(config):
    # default dummy display

    display = Display(config)

    if not config.G_IS_RASPI:
        config.G_DISPLAY = "None"
        (+) display.send = False
        return display

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep my bad.
What about the change to asyncio you introduced?

Copy link
Owner

Choose a reason for hiding this comment

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

The corrected code seems to be the old correct order of execution.
In the latest version, the proper code should be reconsidered.

https://github.com/CabbageDevelopment/qasync/blob/master/qasync/__init__.py#L332

@hishizuka
Copy link
Owner

Hi @Ptosiek ,

Thank you for your PR.

  1. Ah, sure. mip_display.py’s update() checks for G_QUIT, but cython's update() does not. I think it's unnecessary.
  2. In the past I have intended the following.

change_brightness is like the OS brightness adjustment. For example, specify the brightness from 0~100% in 10% increments and fix the brightness. I used to assign this function to some button, but I turned it off. It should originally be in the menu. (I didn't make it)

Then, I created an automatic backlight on/off function with G_USE_AUTO_BACKLIGHT from sensor_core. This function is much more useful. I could have made an on/off toggle button for G_USE_AUTO_BACKLIGHT, but it was too much work, so I added an automatic backlight on/off state at the end of brightness_table.

[self.brightness_table(0, b1, b2, ..., bmax), self.display.G_USE_AUTO_BACKLIGHT(=True)]

So, the refactoring is either to properly create these two menu items, or to properly define the brightness_table with the addition of the state of automatic control of backlighting.

@hishizuka hishizuka merged commit 951867c into hishizuka:master Oct 31, 2023
@Ptosiek
Copy link
Contributor Author

Ptosiek commented Oct 31, 2023

Thanks for the explanation, now I got it.
Though some remark:
When auto_backlight is set: we set the brightness to 0 or 3 for (self.config.display.set_brightness(3))
But the values in table are 0/10/100.
I guess that we don't really use the values in table. But it would make sense that the default (auto) value is reachable manually too, no ?

@hishizuka
Copy link
Owner

hishizuka commented Oct 31, 2023

Yes, the value of 3 is then the value that experience has shown to be sufficient for this brightness.
For practical purposes for MIP display, the table would have the following values,
[0, 1, 2, 3, 5, 7, 10, 25, 50 ,100] (Example of 10 steps)
For the value of the auto setting, choose a value from here.

However, it is difficult to know if this table is appropriate for other displays that can use backlighting.

@Ptosiek Ptosiek deleted the display-conf branch November 2, 2023 08:34
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