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

[Feature request]: Disable learning #49

Closed
danyspin97 opened this issue May 24, 2022 · 8 comments
Closed

[Feature request]: Disable learning #49

danyspin97 opened this issue May 24, 2022 · 8 comments

Comments

@danyspin97
Copy link

Please describe your feature request

While learning is an awesome feature, DDC/I2C is not reliable at all. This means that many readings are incorrect and makes the entire adjustings wrong. After reading the yaml output of the learning feature, I think it would be possible to manually set some fixed value and let wluma use those. Of course, this would still be an optional feature, learning should be activated by default, as it is now.

I'd be willing to contribute to this change, but I'd first want to know your opinion. Thanks for working on this great software :)

@maximbaz
Copy link
Owner

Hello! Thanks for trying it out, and for giving the feedback 🙏

I know and agree that DDC is quite annoying to use, it was on my todo list to implement support for https://gitlab.com/ddcci-driver-linux/ddcci-driver-linux, it is my hope that this driver would provide a much better experience, so that we don't need any hacks and tricks for DDC. From a quick first try I know wluma doesn't work with it out of the box, there were some issues, I can't recall what, maybe with detecting the right display... In any case if you want to experiment with it and submit some PR to make it work, it would be most welcome 😜

To come back to your specific idea, there are two aspects of it. If you want to remove ambient light from the equation, you could try to use hour based simulation of als or disable it altogether (see readme).

In a more general sense, disabling learning is actually not supported by design. Consider the following, you trained wluma to set 100% brightness during the day and 20% brightness during the night. Now it's night, you disable the training, and change brightness to 50%. With training disabled, this is not saved in the algorithm, and on the next "tick" wluma evaluates the surroundings (it's night) and resets your change back to 20%, completely ignoring your changes... Let me know if this makes sense?

@danyspin97
Copy link
Author

To come back to your specific idea, there are two aspects of it. If you want to remove ambient light from the equation, you could try to use hour based simulation of als or disable it altogether (see readme).

The ambient light is fine for me.

In a more general sense, disabling learning is actually not supported by design. Consider the following, you trained wluma to set 100% brightness during the day and 20% brightness during the night. Now it's night, you disable the training, and change brightness to 50%. With training disabled, this is not saved in the algorithm, and on the next "tick" wluma evaluates the surroundings (it's night) and resets your change back to 20%, completely ignoring your changes... Let me know if this makes sense?

This exactly what I want, reproducible results that cannot be changed by faulty readings. If I don't like the value, I can just go to the config, tweak it and restart wluma. I don't see why it's not supported by design, it requires just some minor changes after all.

After thinking about wluma training system, I have found out it's issue: there is no staling. After an incorrect entry make its way, it will always be used, except when a new entry with the same luma value takes its place. This means that an incorrect brightness reading can lead wluma to have faulty predictions about the screen brightness. I have never designed a system like this, so I might be wrong. My guess is that to improve the learning system, wluma should take in account how much time ago an entry has been added. This doesn't look easy either, the more reason to allow the training feature to be disabled.

@maximbaz
Copy link
Owner

This exactly what I want, reproducible results that cannot be changed by faulty readings. If I don't like the value, I can just go to the config, tweak it and restart wluma.

But you could do that today, no change required, right? Training only happens when you change brightness by hand while wluma is running. With your proposal (as I understand it now) you'd basically disable the functionality of your built-in buttons "increase & decrease brightness" (because with no training, your key press would be instantly reverted to the last trained value).

On the topic of incorrect readings, so ddc actually gives you wrong readings? Not just errors, but say when your actual brightness is 70%, wluma reads it as 43%? That I have not expected!

@maximbaz
Copy link
Owner

Or maybe I'm understanding something, while you don't touch brightness control buttons at all, you see wluma incorrectly detecting changes and thus retrained? I think this is what you meant, sorry, I finally understand maybe 🙂 But if it's happening, it sounds completely like a bug with reading, maybe we can fix it somehow?

@danyspin97
Copy link
Author

There are various bugs that I am encountering. None of those are related to wluma but to the DDC protocol and library, or rather to the faulty implementation of my monitors. I remember that wluma complained a lot about protocols errors and message length unspecified:

[2022-05-10T14:30:57Z ERROR wluma::brightness::controller] Unable to get brightness value: DDC/CI error: Expected DDC/CI length bit
[2022-05-10T14:31:10Z ERROR wluma::brightness::controller] Unable to get brightness value: DDC/CI error: invalid DDC/CI length
[2022-05-10T14:31:13Z ERROR wluma::brightness::controller] Unable to get brightness value: DDC/CI error: Unsupported VCP code
[2022-05-10T14:31:15Z ERROR wluma::brightness::controller] Unable to get brightness value: DDC/CI error: Expected DDC/CI length bit

It also happens that sometimes the reading is (slightly) incorrect, or setting the new brightness fail.

Or maybe I'm understanding something, while you don't touch brightness control buttons at all, you see wluma incorrectly detecting changes and thus retrained?

Luckily, this is not happening.

I can't try wluma again because it fails the assertion "Frames with multiple objects are not supported yet", so for now I just have the strace and logs I collected 2 weeks ago.

But you could do that today, no change required, right? Training only happens when you change brightness by hand while wluma is running.

Yea, I can just do that, but I wanted to make sure that these values cannot be set again and are considered as "configuration" rather than data.

With your proposal (as I understand it now) you'd basically disable the functionality of your built-in buttons "increase & decrease brightness" (because with no training, your key press would be instantly reverted to the last trained value).

That would also be fine for me. If I need manual brightness, I can temporarily stop wluma.

@maximbaz
Copy link
Owner

I can't try wluma again because it fails the assertion "Frames with multiple objects are not supported yet"

For info, you can try to set WLR_DRM_NO_MODIFIERS=1 before starting sway, until this is fixed in #8.

I understand the frustration you have with the lack of trust with DDC, overall I'd really love to get some stability into this, so that you can have trust in the fact that unless you change brightness by hand, the saved values would never disappear or get overwritten, the learning would not get triggered, etc.

I see two major ways forward, trying to get https://gitlab.com/ddcci-driver-linux/ddcci-driver-linux to work and evaluating its stability (and if it doesn't have the same issues, ditch the current ddcutil in favor of this driver entirely), or at least try to stabilize the DDC protocol (we can introduce some retries for more stable reading and writing, or averaging the read values to prevent slightly incorrect readings, just as a few ideas).

@danyspin97
Copy link
Author

danyspin97 commented Jun 4, 2022

I see two major ways forward, trying to get https://gitlab.com/ddcci-driver-linux/ddcci-driver-linux to work and evaluating its stability (and if it doesn't have the same issues, ditch the current ddcutil in favor of this driver entirely), or at least try to stabilize the DDC protocol (we can introduce some retries for more stable reading and writing, or averaging the read values to prevent slightly incorrect readings, just as a few ideas).

Those two sounds like good ideas, let's see if moving to ddcci-driver-linux improves it a bit.

Thank you for your support! :)

@cyrinux
Copy link
Contributor

cyrinux commented Oct 11, 2022

It is implem, you can use this config example:

...
[[output.backlight]]
name = "DP-3"
path = "/sys/class/backlight/ddcci12"
capturer = "wlroots"

[[output.backlight]]
name = "DP-4"
path = "/sys/class/backlight/ddcci13"
capturer = "wlroots"
...

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

No branches or pull requests

3 participants