Skip to content

Conversation

@p-gentili
Copy link
Collaborator

@p-gentili p-gentili commented Jul 4, 2023

Description

Zapper-driven EDID cycling testcase has always failed in our lab. The reason is that on laptops (and in general extended monitors) the resolution retrieved using xdpyinfo is considering the whole setup, not the single outputs.

With this PR, tools like xrandr and gnome-randr are used instead: they display for each available outputs the possible resolutions, the preferred one (+) and the one in use (*).

Also, since I've experimented some noise moving directly from one EDID to another, I added an "unplug" step between every new EDID.

Resolved issues

Fixes ZAP-516

Documentation

The script was quite poor in terms of docstrings, I added a couple of them.

Tests

Successful runs:

$ XDG_SESSION_TYPE="wayland" DISPLAY=:0 ZAPPER_HOST=192.168.5.6 checkbox-cli run com.canonical.certification::monitor/zapper-edid
Using sideloaded provider: checkbox-provider-base, version 2.7 from /var/tmp/checkbox-providers/base
[...]
changing EDID to 2560x1440
checking resolution... PASS
changing EDID to 1920x1080
checking resolution... PASS
changing EDID to 1280x1024
checking resolution... PASS
[...]
 ☑ : Check if the system automatically changes the resolution based on EDID

@p-gentili p-gentili force-pushed the fix-edid-randr branch 3 times, most recently from 1bfe39d to 64fd944 Compare July 4, 2023 13:06
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

See below.

def change_edid(host, edid_file):
"""Clear EDID and then 'plug' back a new monitor."""
zapper_run(host, "change_edid", None)
time.sleep(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll be thinking loud here:
If there's a delay that we have to wait in order for EDID to land, it seems like the better place to do that would be on Zapper side. Cause right now it "maybe changes" :)
Second thing is why 1s? IIRC we can check whether they got propagated (v4l2 (something, something) query timings), so I guess a busy loop would be better, that after 5seconds returns an error if it failed to do appropriate operation.
What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, those manual delays are not nice :D
It could be done on both sides actually. On Zapper side there's a problem though: the EDID change is immediate but the timings in use are available (and credible) only if there's a monitor connected. The command is v4l2-ctl --query-dv-timings.
Since I don't want the Zapper service to hang and timeout only because there's no monitor connected I'd prefer to have the busy loop on the checkbox side. Done in the last commit.

from checkbox_support.scripts.zapper_proxy import ( # noqa: E402
zapper_run)

PATTERN = "^HDMI-1.*\n.* (\\d+x\\d+).*\\*"
Copy link
Contributor

Choose a reason for hiding this comment

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

use r-string literals, this way the whole string is taken as and you don't have to escape characters.
also the space before the parenthesis is redundant.
AKA:
`r"^HDMI-1.\n.(\d+x\d+).**'

Comment with explanation would be awesome, preferably with examples (ehem tests :P )

Copy link
Collaborator Author

@p-gentili p-gentili Jul 7, 2023

Choose a reason for hiding this comment

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

Thanks for the suggestion. The space is actually required because otherwise it matches just the last digit of the first number: like "0x1080" instead of 1920x1080. I modified it a little bit to make it clearer.
Also, added doctests.

@p-gentili p-gentili force-pushed the fix-edid-randr branch 2 times, most recently from c3bdaf3 to dc56dcf Compare July 7, 2023 16:10
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Nice!
+1

Wait until `expected` connection state is reached.
Times out after 5 seconds.
"""
iteration = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

one cool pythonic way of doing similar thing is:

for iteration in enumerate(5):
    if check_connected("HDMI-1") == expected:
        break
else:
    raise TimeoutError

But the one here is perfectly fine

@p-gentili p-gentili merged commit 45b1886 into main Jul 20, 2023
@p-gentili p-gentili deleted the fix-edid-randr branch July 20, 2023 14:22
diohe0311 pushed a commit that referenced this pull request Jul 31, 2023
* Fix: EDID cycling is asserting resolution with the wrong data

* Change: unplug and plug back the HDMI
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.

3 participants