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

Severity of VCPIOError errors #5

Open
mciupak opened this issue Jul 23, 2020 · 22 comments
Open

Severity of VCPIOError errors #5

mciupak opened this issue Jul 23, 2020 · 22 comments
Assignees
Labels
bug Something isn't working

Comments

@mciupak
Copy link

mciupak commented Jul 23, 2020

Due to high level of errors severity it is impossible to use monitorcontrol (VCPIOError exception thrown) with monitors which do not follow the DDC/CI standard e.g. Philips 223V monitors returns corrupted data i.e. incorrect checksum and/or wrong length, despite that it is possible to control it.

Example output:
Reading 0x60...
Send: 51 82 01 60 dc | Q...
Recv: 6e | n
Invalid response, corrupted data - xor is 0x5e, length 0x08
6e 88 88 02 00 60 00 00 03 00 01 | n.........
Send: 51 82 01 60 dc | Q...
Recv: 6e | n
Invalid response, corrupted data - xor is 0x5e, length 0x08
6e 88 88 02 00 60 00 00 03 00 01 | n.........
Send: 51 82 01 60 dc | Q...
Recv: 6e | n
Invalid response, corrupted data - xor is 0x5e, length 0x08
6e 88 88 02 00 60 00 00 03 00 01 | n.........

If possible, it could be better to handle errors without throwing an exception and try to continue execution.

@newAM
Copy link
Owner

newAM commented Jul 25, 2020

I have seen this behavior once before on a different monitor, what helped for that was increasing the timeout beyond the DDC-CI minimum values.

Can you try increasing these constants to see if it changes the behavior?

monitorcontrol.vcp.vcp_linux.LinuxVCP.GET_VCP_TIMEOUT = 0.2  # default 0.04
monitorcontrol.vcp.vcp_linux.LinuxVCP.CMD_RATE = 0.2 # default 0.05

@mciupak
Copy link
Author

mciupak commented Jul 26, 2020

Unfortunately, it does not help:

File "/root/.local/lib/python3.7/site-packages/monitorcontrol/vcp/vcp_linux.py", line 178, in get_vcp_feature
raise VCPIOError(f"checksum does not match: {checksum_xor}")
monitorcontrol.vcp.vcp_abc.VCPIOError: checksum does not match: 80

@newAM
Copy link
Owner

newAM commented Jul 26, 2020

Huh, interesting! This is the first monitor that I have heard of that violates the spec then.

I would like to keep the exceptions as the default behavior since they usually mean that the rest of the command will fail.
These exceptions are buried rather deep as you noted. I would like to ideally add some way for the user to handle the exceptions better, but I don't see an elegant way forward with that.

I am thinking of a static class variable to disable exceptions; something along the lines of:

monitorcontrol.vcp.vcp_linux.LinuxVCP.BYPASS_CHECKSUM: bool = True
# and others for the various locations where VCPIOError can be raised

Would that work for your application?

@mciupak
Copy link
Author

mciupak commented Jul 28, 2020

Bypassing only checksum won't work as most probably remaining code will fail as well. I saw this already - not only checksum was incorrect but expected data was too short thus unpacking failed and it was not caught.

For my application it would be better to pass some argument during runtime down to vcp to bypass checksum and following operations and try to recover and/or use some predefined default values i.e. I have config file in which I can specify e.g. which bus number should have it bypassed and remaining one(s) should not.

As I have physical access to this monitor I can prototype and propose something. What do you think?

@newAM
Copy link
Owner

newAM commented Jul 30, 2020

As I have physical access to this monitor I can prototype and propose something. What do you think?

That would be great! 👍 It is hard to develop for something that does not meet the spec.

@jiri-one
Copy link

jiri-one commented Sep 29, 2020

I have latest Arch Linux, ddcutil works great, but I would like to use Python implementation, because I plan gui and small hw which will be set backlight value from ambient light (I finished HW, next part is SW in Python). But unfortunately latest monitorcontrol said:

monitorcontrol --get-luminance
Traceback (most recent call last):
  File "/usr/bin/monitorcontrol", line 8, in <module>
    sys.exit(main())
  File "/usr/lib/python3.8/site-packages/monitorcontrol/__main__.py", line 70, in main
    luminance = monitor.get_luminance()
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 180, in get_luminance
    return self._get_vcp_feature(code)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 157, in _get_vcp_feature
    current, maximum = self.vcp.get_vcp_feature(code.value)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/vcp/vcp_linux.py", line 178, in get_vcp_feature
    raise VCPIOError(f"checksum does not match: {checksum_xor}")
monitorcontrol.vcp.vcp_abc.VCPIOError: checksum does not match: 115

But the problem is not in monitor (it is HPE243i), because on Windows
monitorcontrol is working great :).
On Arch Linux I followed this https://monitorcontrol.readthedocs.io/en/latest/linux_setup.html and it is working for ddcutil too, but for monitorcontrol there is checksum bug.

@newAM newAM self-assigned this Sep 29, 2020
@newAM newAM added the bug Something isn't working label Sep 29, 2020
@newAM
Copy link
Owner

newAM commented Sep 29, 2020

...huh, if two people have this issue then this likely is a bug.

I'll add something this weekend to optionally disable the checksum.

@jiri-one does you monitor also work aside from the bad checksum?

@jiri-one
Copy link

On Win10 I tested only get and set luminance and it worked great. But on Archlinux everything ended with bad checksum.

@sebastianmaisel
Copy link

I also always get the VCPIOError which says: checksum does not match: 80, but I didn't get the display run on my Ubuntu properly with other tools while it works perfect on macOS with ddcutil.
There may also be a driver issue.

@newAM
Copy link
Owner

newAM commented Oct 4, 2020

I pushed a 2.2.0 release which disables the checksums by default.

https://monitorcontrol.readthedocs.io/en/latest/api.html#checksum-behaviour

This does not fully address this issue since there were also problems with the payload unpacking mentioned here.

Let me know if this solves any of the problems you guys are having (or what new problems you now have).

@jiri-one
Copy link

jiri-one commented Oct 5, 2020

I tested new version on Archlinux and it is not working.
https://pastebin.com/xNYnmCP1
And the last version is working nice on Windows .... with same monitor.

@sebastianmaisel
Copy link

sebastianmaisel commented Oct 5, 2020 via email

@jiri-one
Copy link

jiri-one commented Oct 5, 2020

@newAM Maybe you want to make some tests on my monitor? If you contact me directly on email jiri spot one in domain pm spot me (I hope I confused the spambots enough :)) then I can redirect some port on my router to my ArchLinux PC, and you can connect to it over SSH. Just if you want, it is just idea.

@newAM
Copy link
Owner

newAM commented Oct 8, 2020

Thanks for the offer @jiri-one , I may take you up on that pending the results of this next iteration.

I made another release, 2.3.0 with debug logging in the VCP class (and a --verbose argument to the CLI). This will not fix the exception, but it will give clues as to what is going on.

From the CLI can one of you repeat this with the new version (pip install -U monitorcontrol to upgrade)?

$ monitorcontrol -vvvv --get-luminance
DEBUG monitorcontrol.vcp.vcp_linux header=b'n\x88'
DEBUG monitorcontrol.vcp.vcp_linux payload=b'\x02\x00\x10\x00\x00d\x00d\xa4'
100

Yours should look similar but with the exception instead of 100.

The byte values of the header and the payload will help a lot to compare with the spec.

@jiri-one
Copy link

Hi, sorry for my delay, but covid shot me. Here is output:

monitorcontrol -vvvv --get-luminance
DEBUG monitorcontrol.vcp.vcp_linux header=b'\x08\x08'
DEBUG monitorcontrol.vcp.vcp_linux payload=b'\x08\x08\x08\x08\x08\x08\x08\x08\x08'
Traceback (most recent call last):
  File "/usr/bin/monitorcontrol", line 8, in <module>
    sys.exit(main())
  File "/usr/lib/python3.8/site-packages/monitorcontrol/__main__.py", line 101, in main
    luminance = monitor.get_luminance()
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 180, in get_luminance
    return self._get_vcp_feature(code)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 157, in _get_vcp_feature
    current, maximum = self.vcp.get_vcp_feature(code.value)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/vcp/vcp_linux.py", line 202, in get_vcp_feature
    raise VCPIOError(
monitorcontrol.vcp.vcp_abc.VCPIOError: received unexpected response code: 8

@newAM
Copy link
Owner

newAM commented Nov 1, 2020

Hi, sorry for my delay, but covid shot me.

No worries, hope you are doing well now!

monitorcontrol -vvvv --get-luminance
DEBUG monitorcontrol.vcp.vcp_linux header=b'\x08\x08'
DEBUG monitorcontrol.vcp.vcp_linux payload=b'\x08\x08\x08\x08\x08\x08\x08\x08\x08'

Well, I am not sure what I expected, but it definitely wasn't that.

I will take you up on your offer to SSH into your box if that is still open - this will require some trial and error to get to the bottom of, I sent you an email with my public SSH key.

@newAM
Copy link
Owner

newAM commented Nov 21, 2020

I got access to a failing monitor (thanks @jiri-one !) a while ago.

I think I found the root cause of this issue in the way the DDC packets were being constructed.

Using Jiri's monitor with monitorcontrol -vvvvv --get-luminance, to generate a payload of 51 82 01 10 it would only work if I hard coded the CRC to AC.

monitorcontrol calculated the CRC as C2 though, which checks out.

>>> hex(0x51 ^ 0x82 ^ 0x01 ^ 0x10)
'0xc2'

The difference between C2 and AC is 6E:

>>> hex(0xC2 ^ 0xAC)
'0x6e'

Now there's a familiar number from the DDC spec!

Sure enough, shift it by one, and you get the DDC-CI command address:

>>> hex(0x6E >> 1)
'0x37'

Now that's where the problems start occurring, the address is set in the VCP constructor with ioctl, and then the CRC is calculated like so:

        # transmission data
        data = bytearray()
        data.append(self.GET_VCP_CMD)
        data.append(code)

        # add headers and footers
        data.insert(0, (len(data) | self.PROTOCOL_FLAG))
        data.insert(0, self.HOST_ADDRESS)
        data.append(self.get_checksum(data))

get_checksum doesn't take into account the address in the CRC calculation.

The correct checksum is calculated if I just prepend the command address to the CRC.

        # transmission data
        data = bytearray()
        data.append(self.GET_VCP_CMD)
        data.append(code)

        # add headers and footers
        data.insert(0, (len(data) | self.PROTOCOL_FLAG))
        data.insert(0, self.HOST_ADDRESS)
        data.append(
            self.get_checksum(bytearray([self.DDCCI_ADDR << 1]) + data)
        )

Anyway, a pull-request is here to fix this: #20
Let me know if it fixes anything for you guys.

Also turns out my monitors are just ignoring invalid CRC, which explains why I couldn't reproduce this 👎

@jiri-one
Copy link

Not a problem newAM and note, your access to my PC is still on, so if you need some more tests, it is not a problem ... because:

This is version from git:

./monitorcontrol --get-luminance
Traceback (most recent call last):
  File "./monitorcontrol", line 8, in <module>
    sys.exit(main())
  File "/usr/lib/python3.8/site-packages/monitorcontrol/__main__.py", line 101, in main
    luminance = monitor.get_luminance()
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 180, in get_luminance
    return self._get_vcp_feature(code)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 157, in _get_vcp_feature
    current, maximum = self.vcp.get_vcp_feature(code.value)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/vcp/vcp_linux.py", line 186, in get_vcp_feature
    raise VCPIOError(
monitorcontrol.vcp.vcp_abc.VCPIOError: received unexpected response code: 8

And this is version from git with replaced file from commit 27235db

monitorcontrol --get-luminance
Traceback (most recent call last):
  File "./monitorcontrol", line 8, in <module>
    sys.exit(main())
  File "/usr/lib/python3.8/site-packages/monitorcontrol/__main__.py", line 101, in main
    luminance = monitor.get_luminance()
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 180, in get_luminance
    return self._get_vcp_feature(code)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/monitorcontrol.py", line 157, in _get_vcp_feature
    current, maximum = self.vcp.get_vcp_feature(code.value)
  File "/usr/lib/python3.8/site-packages/monitorcontrol/vcp/vcp_linux.py", line 186, in get_vcp_feature
    raise VCPIOError(
monitorcontrol.vcp.vcp_abc.VCPIOError: received unexpected response code: 8

@improbity39
Copy link

I have a problem, is there any way to send and get commands like 6e 88 88 02 00 60 00 00 03 00 01 in vcp_linux from vcp_windows

@newAM
Copy link
Owner

newAM commented Nov 22, 2022

I have a problem, is there any way to send and get commands like 6e 88 88 02 00 60 00 00 03 00 01 in vcp_linux from vcp_windows

I'm not sure I understand the "in vcp_linux from vcp_windows" part.

@improbity39
Copy link

I have a problem, is there any way to send and get commands like 6e 88 88 02 00 60 00 00 03 00 01 in vcp_linux from vcp_windows

I'm not sure I understand the "in vcp_linux from vcp_windows" part.

Hmm...I mean I want to use the command 03 10 00 100 on windows instead of using -set-luminance 100, and I will get the information returned by the monitor like 6F 6E 88 02...
I'm sorry that I am new to this field, some words may not be clear.

@newAM
Copy link
Owner

newAM commented Nov 22, 2022

It has been a while since I wrote this, but I don't think you can do (exactly) that with the APIs Microsoft provides for Windows, because it abstracts away some of the headers/footers: https://learn.microsoft.com/en-us/windows/win32/api/lowlevelmonitorconfigurationapi/#functions

If that's just a set/get VCP feature then you can use the set_vcp_feature and get_vcp_feature abstraction on the VCP which tries to make the Windows/Linux functions look the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants