digitaltrails / vdu_controls

VDU controls - a control panel for monitor brightness/contrast/...
GNU General Public License v3.0
103 stars 4 forks source link

It doesn't support two-byte values #85

Closed denilsonsa closed 2 months ago

denilsonsa commented 3 months ago

So, I have an AOC Q27P1B monitor that gives some weird values for "input source". It seems to support writing to some of those values as a way to change the input source, and then it returns a different value upon reading. It's a mess.

BUT, this bug here is about vdu_controls not supporting two-byte values. Look:

$ ddcutil -v -d 1 getvcp 60
Getting data for non-table VCP code 0x60 - Input Source:
Raw value: opcode=0x60, mh=0x00, ml=0x04, sh=0x03, sl=0x05, max_val=4 (0x0004), cur_val=773 (0x0305)
VCP code 0x60 (Input Source                  ): Composite video 1 (sl=0x05)

Observe cur_val=773 (0x0305), but vdu_controls is reporting only 0x05. So, it's only looking that the low byte sl=0x05, while it should be looking at both high and low bytes (sh=0x03 and sl=0x05).

vdu_controls error message

Here's my custom capabilities-override: Q27P1B.conf. I've written my own list of values for feature 60, based on trial-and-error.


In summary:

Problem: vdu_controls is only reading the low byte, so it is incorrectly seeing the value 0x05.

Expectation: it should consider both bytes, as the correct full value is 0x305.

My system:

digitaltrails commented 3 months ago

Is this a change that has started occurring in the last couple of days?

I've released ddcutil-service 1.0.3 with a fix for ddcutil-service issue #84. The issue was that the high byte returned from some VDU's for some features was not zero for SNC (one byte Simple Non-Continuous features), which lead to values such as x0f being returned as xF0F. So ddcutil-service was changed to discard the high byte for SNC features.

So is this an SNC or CNC (two-byte Complex Non-Continuous) feature? We can find this out by doing:

% ddcutil --display 1 getvcp --brief 60

If it's CNC the output will be something like:

VCP 60 CNC x00 x00 x00 x00

If it's SNC the output will be something like:

VCP 60 SNC x00

Given 300 is a valid value I expect that this is a CNC feature. If the feature is CNC, then we should not be throwing away the high value, and this must be a bug.

We should see what the service is returning, we can do this by:

% busctl --user call com.ddcutil.DdcutilService /com/ddcutil/DdcutilObject com.ddcutil.DdcutilInterface GetVcp isyu "1" "" "0x60" "0"
qqsis 15 18 "DisplayPort-1 (sl=0x0f)" 0 "OK"

In this case, the result is from my HP monitor only shows sl because feature 0x60 is SNC, but maybe not for your AOC.

I expect there may be bugs here because most values are SNC, CNC is rare, it would be good to test an actual CNC feature.

digitaltrails commented 3 months ago

If the the feature is SNC, we could consider adding an optional CNC/two-byte override flag that can be added to the metadata that would be passed down to GetVcp in ddcutil-service. (Noting a possible solution in case we need one).

BTW - By setting a fake high byte value, I've checked and two byte CNC values are being passed from ddcutil-service back to vdu_controls, and vdu_controls is using them. So I'm not sure where the cause of this issues resides at this point.

denilsonsa commented 3 months ago

Indeed, feature 60 is supposed to be SNC:

$ ddcutil --display 2 getvcp --brief 60
VCP 60 SNC x05
$ busctl --user call com.ddcutil.DdcutilService /com/ddcutil/DdcutilObject com.ddcutil.DdcutilInterface GetVcp isyu "1" "" "0x60" "0"
qqsis 5 4 "Composite video 1 (sl=0x05)" 0 "OK"

But with these AOC displays, the high byte changes for DVI/HDMI inputs. Like mentioned in my own trial-and-error investigation:

Value Input source Readable Writable Notes
x01 VGA readable writable
x02 VGA no writable There is only one VGA port.
x03 DVI no writable
x04 DVI no writable There is only one DVI port.
x0f DisplayPort no writable
x10 Doesn't work no no
x11 HDMI no writable
x12 Doesn't work no no
x300 DVI/HDMI readable untested After finished switching the input source.
x301 DVI/HDMI readable untested While switching the input source.
x302 DVI/HDMI readable untested While switching the input source.
x305 DisplayPort readable untested

If we discard the high byte, then DVI/HDMI input gets shown as VGA, and switching to it will actually switch to VGA. Additionally, the DisplayPort input gets shown as Composite Video, which this display doesn't have.

Yeah, it's a mess. And I don't have a Windows machine to try to snoop i2c data from AOC software (and I don't know how to do it anyway).

digitaltrails commented 3 months ago

I will see about adding a keyword option that can be embedded in the metadata. The keyword will result in a flag value being passed to ddcutil-service that will restore the behaviour of returning both bytes.

I guess this should be generalised to passing a type-value so that the type can be overridden:

Or maybe I should just simplify that down to *UINT16* because I'm not actually sure what CC/SC means in terms of bytes. I'll give it some thought.

These would work much like the recently added *refresh* where adding that keyword to a feature causes the entire UI to refresh if that control is altered:

Feature: 15 (Picture Mode *refresh*)

So we might have:

Feature: 60 (Input Source *CNC*)

This will require changes in vdu_controls and ddcutil-service. I'm currently preparing a new release for vdu_controls, so I'll bundle into that. The ddcutil-service change is likely to be small, I can do a small release for that too (I expect to release that more often because it's new and evolving).

digitaltrails commented 3 months ago

In the meantime, if you want to build yourself a ddcutil-service that works, you could download the source and in the two functions get_vcp/get_multiple_vcp, you could change the two instances of lines that check for simple, which look like this:

const bool simple = (DDCA_SIMPLE_NC & metadata_ptr->feature_flags);

to always return false

const bool simple = false;

Then two byte values will always be returned (which is how it was in 1.0.2).

denilsonsa commented 3 months ago

In the meantime, if you want to build yourself a ddcutil-service that works

Thank you, but I'll pass. Right now I only have one input source, so I have no hurry. (In the future I'll likely get two inputs on the same display). My workaround was to add a dummy entry for the value x03.

digitaltrails commented 3 months ago

I'm going to discuss this with Sanford Rockowitz, perhaps I should always be passing both bytes and the special case should be one byte (even though one byte is the more standard compliant approach).

denilsonsa commented 3 months ago

So we might have:

Feature: 60 (Input Source *CNC*)

Or… If one of the values in the custom capabilities file is beyond a single byte, then the software can assume it should use two bytes. No extra keyword needed.

(Not sure if you're going towards that approach, but this is something I thought that can be simple enough and intuitive enough.)

digitaltrails commented 3 months ago

Or… If one of the values in the custom capabilities file is beyond a single byte, then the software can assume it should use two bytes. No extra keyword needed.

That sounds like a nice way to do it - I'll see if it's easy to implement.

(Not sure if you're going towards that approach, but this is something I thought that can be simple enough and intuitive enough.)

I'm waiting to see if Sanford has any insights. It's possible that libddcutil has a bug and it should be returning CNC. It's also possible that this is a common issue and I should reverse course and make two bytes the default with a qualifier to drop the high byte when necessary.

digitaltrails commented 3 months ago

What I may do is keep the service as a pure libddcutil wrapper. I may reverse the change that strips the high byte (issue #84). I would have the service always pass back both bytes. The client (vdu_controls) would have the responsibility of deciding what to do with each byte.

In this case, the client (vdu_controls) can circumvent the libddcutil/VDU retrieved feature-type metadata. It can do this my looking at the max-value in the list of possible-value metadata. It can promote a Non-Continuous to Complex (CNC) if the max is > 0xff, otherwise assume it's Simple (SNC) and discard the high byte. So, for the #84 the metadata max listed value is <= 0xff, so the high byte would be discarded, but for #85 the max is > 0xff, so vdu_controls would promote it to CNC and retain both bytes.

Or some variation on the above. Still pondering.

denilsonsa commented 3 months ago

Maybe… When probing the display for the value, if it receives a non-zero high byte, it could assume it's a two-byte value? But this idea breaks if the high byte is junk.

So far I've been thinking about reading those features. What about writing back to them? Can libddcutil/ddcutil-service write a two-byte value to a feature that reports itself as SNC?

digitaltrails commented 3 months ago

Maybe… When probing the display for the value, if it receives a non-zero high byte, it could assume it's a two-byte value? But this idea breaks if the high byte is junk.

Ddcutil-service Version 1.0.3 was rolled out to address issue #79 where an AW3225QF VDU was returning an SNC as 0x0Fnn instead of 0x00nn. The solution was to throw away the high-byte as an SNC is a one byte value.

But along comes the AOC Q27P1B, which appears to break the standard and returns something significant in the high-byte. The Q27P1B metadata should indicate this feature is CNC, not SNC.

To address both issues, ddcutil-service 1.0.4 now supports passing a flag on GetVcp/GetMultipleVcp that forces the return of the high-byte value. By default, ddcutil-service will obey the standard and assume the SNC high-byte must be junk, but it can return it if the client wants to make that decision itself. The flag value required is 2 (it's a 32 bit flag).

# Display 1, feature 0xcc, flags=2 (return all bytes)
busctl --user call com.ddcutil.DdcutilService /com/ddcutil/DdcutilObject com.ddcutil.DdcutilInterface GetVcp isyu "1" "" "0xcc" "2"

Vdu_controls 2.0.2 passes the new flag, so we're back to receiving all bytes. At VDU initialisation, vdu_controls inspect the value lists, if something in a list is greater than 0x00ff, then vdu_controls will internally promote the feature to CNC. If a feature is internally defined as CNC, two bytes are accepted. If a feature is internally defined as SNC, the high-byte is masked off.

So for vdu_controls, what is in the capabilities metadata determines whether an SNC is treated as SNC or CNC.

This should enable anyone to work with meaningful-high-bytes or junk-high-bytes, or even both.

So far I've been thinking about reading those features. What about writing back to them? Can libddcutil/ddcutil-service write a two-byte value to a feature that reports itself as SNC?

Vdu_controls passes a full 16 bit value to ddcutil-service. Ddcutil-service doesn't inspect the value, both bytes are passed to libddcutil. So unless libddcutil discards the high byte, this shouldn't be a problem.

digitaltrails commented 2 months ago

Both vdu_controls 2.0.2 and ddcutil-service 1.0.4 have been released.