Improvements in EDID cycling with Zapper (BugFix) by kissiel · Pull Request #1229 · canonical/checkbox

kissiel · 2024-05-06T10:15:59Z

Description

TL;DR: A more reliable way to do EDID cycling with Zapper.

When debugging the Zapperized jobs that tested the changes in resolutions of the connected monitor (as emulated with Zapper), I noticed that the regexes failed to detect the right port on which the Zapper was connected. I also discovered there's plenty of corner cases which would be very hard to cover with just a regex extraction. Thus, I wrote a fully fledged parser for xrandr's and gnome-randr's output. Both parsers are used in the new function I'm proposing here to land in checkbox-support where it would return the information about display modes available on given video output.

This in turn, when plugged into the edid_cycle test script started yielding true results from the job.

Resolved issues

Fixes: https://warthogs.atlassian.net/browse/ZAP-677
Fixes: https://warthogs.atlassian.net/browse/ZAP-678

Tests

I tested the code by running it on multiple devices running both X11 and Wayland.
I also propose unit tests with the code.

codecov · 2024-05-06T10:18:33Z

Codecov Report

Attention: Patch coverage is 98.11321% with 2 lines in your changes missing coverage. Please review.

Project coverage is 43.50%. Comparing base (646fe88) to head (89ec219).
Report is 171 commits behind head on main.

Files with missing lines	Patch %	Lines
...ox-support/checkbox_support/parsers/gnome_randr.py	96.96%	0 Missing and 1 partial ⚠️
...heckbox-support/checkbox_support/parsers/xrandr.py	97.14%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1229      +/-   ##
==========================================
+ Coverage   43.36%   43.50%   +0.13%     
==========================================
  Files         357      361       +4     
  Lines       38684    38782      +98     
  Branches     6560     6576      +16     
==========================================
+ Hits        16775    16871      +96     
  Misses      21245    21245              
- Partials      664      666       +2

Flag	Coverage Δ
checkbox-support	`53.12% <97.95%> (+0.90%)`	⬆️
provider-base	`16.71% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hook25

One major change to request, looks very nice, very useful utility!

Hook25 · 2024-05-07T12:52:31Z

            print(exc.args[0])
            failed = True
+    # leave the system with no edid set - monitor disconnected
+    _clear_edid(args.host)


This is my only major comment on this whole PR, this should be a contextmanager or atexit

Hook25 · 2024-05-07T12:57:19Z

beside the major comment below, what about _check_resolution? cant it also be greately simplified to just use the new get_display_modes api? if so, lets do it, its a negative lines change

Hook25 · 2024-05-07T13:00:50Z

+                Mode(resolution, refresh_rate, is_preferred, is_current)
+            )
+        return "mode", modes
+    return None


Minor: this is sligly different to what we do above, which is a bit counterintuitive, and I would throw a ValueError like int or float! Also, should we log what line triggers this condition? so that in the log we can debug easily if this causes an issue down the line

Hook25 · 2024-05-07T13:03:31Z

+            try:
+                refresh_rate = float(re.sub(r"[+*]", "", rate))
+            except ValueError:
+                # float couldn't be parsed


Same as below, should we log an error here? why wasn't this refresh rate logged?

p-gentili

This looks great and it's much more readable. It probably fixes some of the problems but not those reported in the linked cards unfortunately.

ZAP-677 still fails because laptops by default are set to mirror mode. Tested on one of the machine reported in the card.
ZAP-678 still fails because there are devices that are just not able to display high resolutions like 2560x1440 (HDMI < 1.4?). Tested on one of the machine reported in the card.
CHECKBOX-1436, even if not listed, is probably fixed with this PR but I found a regression while detecting the video port in use. It fails with Cannot detect the target video output device.

This can still be a good starting point but we may need to use another tool to get the job done, especially the mirror/extended mode. This was recommended by the Desktop team and could help us also with both setting and querying the display info, with a single parses for both wayland and x11. This is also nice but only querying is supported. We can discuss it offline but at least the regression should be fixed before landing this.

About the regression:

w/o Zapper EDID:

$ gnome-randr
supports-mirroring: true
layout-mode: physical
supports-changing-layout-mode: false
global-scale-required: false
legacy-ui-scaling-factor: 1
renderer: "native"

logical monitor 0:
x: 0, y: 0, scale: 1, rotation: normal, primary: yes
associated physical monitors:
	DP-5 TCT DP1080P60 0x00000000

DP-5 TCT DP1080P60 0x00000000
  3840x2160@17.000450134277344	3840x2160 	17.00     	[x1.00+, x2.00, x3.00, x4.00]
  2560x1600@29.985794067382812	2560x1600 	29.99     	[x1.00+, x2.00, x3.00]
  2560x1440@30.000099182128906	2560x1440 	30.00     	[x1.00+, x2.00, x3.00]
  1920x1200@59.950172424316406	1920x1200 	59.95     	[x1.00+, x2.00]
                  1920x1080@60	1920x1080 	60.00*+   	[x1.00+, x2.00]
  1920x1080@59.940200805664062	1920x1080 	59.94     	[x1.00+, x2.00]
  1680x1050@59.883251190185547	1680x1050 	59.88     	[x1.00+, x2.00]
  1600x1200@59.923629760742188	1600x1200 	59.92     	[x1.00+, x2.00]
                   1600x900@60	1600x900  	60.00     	[x1.00+]
  1440x1080@59.988838195800781	1440x1080 	59.99     	[x1.00+, x2.00]
  1440x1080@59.912242889404297	1440x1080 	59.91     	[x1.00+, x2.00]
   1440x900@59.901458740234375	1440x900  	59.90     	[x1.00+]
  1400x1050@59.978443145751953	1400x1050 	59.98     	[x1.00+]
  1400x1050@59.947769165039062	1400x1050 	59.95     	[x1.00+]
   1368x768@59.882049560546875	1368x768  	59.88     	[x1.00+]
   1368x768@59.853202819824219	1368x768  	59.85     	[x1.00+]
  1280x1024@75.024673461914062	1280x1024 	75.02     	[x1.00+]
  1280x1024@60.019741058349609	1280x1024 	60.02     	[x1.00+]
   1280x960@59.939048767089844	1280x960  	59.94     	[x1.00+]
   1280x960@59.920433044433594	1280x960  	59.92     	[x1.00+]
     1280x800@59.9095458984375	1280x800  	59.91     	[x1.00+]
                   1280x720@60	1280x720  	60.00     	[x1.00+]
   1280x720@59.940200805664062	1280x720  	59.94     	[x1.00+]
                   1152x864@75	1152x864  	75.00     	[x1.00+]
   1152x864@59.958633422851562	1152x864  	59.96     	[x1.00+]
   1152x864@59.801021575927734	1152x864  	59.80     	[x1.00+]
   1024x768@75.028579711914062	1024x768  	75.03     	[x1.00+]
   1024x768@70.069358825683594	1024x768  	70.07     	[x1.00+]
   1024x768@60.003841400146484	1024x768  	60.00     	[x1.00+]
                    800x600@75	800x600   	75.00     	[x1.00+]
    800x600@72.187568664550781	800x600   	72.19     	[x1.00+]
    800x600@60.316539764404297	800x600   	60.32     	[x1.00+]
                 800x600@56.25	800x600   	56.25     	[x1.00+]
is-builtin: false
is-underscanning: false
display-name: "Telecom Technology Centre Co. Ltd. 23\""

w/ Zapper EDID:

$ gnome-randr
supports-mirroring: true
layout-mode: physical
supports-changing-layout-mode: false
global-scale-required: false
legacy-ui-scaling-factor: 1
renderer: "native"

logical monitor 0:
x: 0, y: 0, scale: 1, rotation: normal, primary: yes
associated physical monitors:
	DP-5 TCT DP1080P60 0x00000000

logical monitor 1:
x: 1920, y: 0, scale: 1, rotation: normal, primary: no
associated physical monitors:
	HDMI-3 DEL DELL U2713HM 7JNY5488A9CS

DP-5 TCT DP1080P60 0x00000000
  3840x2160@17.000450134277344	3840x2160 	17.00     	[x1.00+, x2.00, x3.00, x4.00]
  2560x1600@29.985794067382812	2560x1600 	29.99     	[x1.00+, x2.00, x3.00]
  2560x1440@30.000099182128906	2560x1440 	30.00     	[x1.00+, x2.00, x3.00]
  1920x1200@59.950172424316406	1920x1200 	59.95     	[x1.00+, x2.00]
                  1920x1080@60	1920x1080 	60.00*+   	[x1.00+, x2.00]
  1920x1080@59.940200805664062	1920x1080 	59.94     	[x1.00+, x2.00]
  1680x1050@59.883251190185547	1680x1050 	59.88     	[x1.00+, x2.00]
  1600x1200@59.923629760742188	1600x1200 	59.92     	[x1.00+, x2.00]
                   1600x900@60	1600x900  	60.00     	[x1.00+]
  1440x1080@59.988838195800781	1440x1080 	59.99     	[x1.00+, x2.00]
  1440x1080@59.912242889404297	1440x1080 	59.91     	[x1.00+, x2.00]
   1440x900@59.901458740234375	1440x900  	59.90     	[x1.00+]
  1400x1050@59.978443145751953	1400x1050 	59.98     	[x1.00+]
  1400x1050@59.947769165039062	1400x1050 	59.95     	[x1.00+]
   1368x768@59.882049560546875	1368x768  	59.88     	[x1.00+]
   1368x768@59.853202819824219	1368x768  	59.85     	[x1.00+]
  1280x1024@75.024673461914062	1280x1024 	75.02     	[x1.00+]
  1280x1024@60.019741058349609	1280x1024 	60.02     	[x1.00+]
   1280x960@59.939048767089844	1280x960  	59.94     	[x1.00+]
   1280x960@59.920433044433594	1280x960  	59.92     	[x1.00+]
     1280x800@59.9095458984375	1280x800  	59.91     	[x1.00+]
                   1280x720@60	1280x720  	60.00     	[x1.00+]
   1280x720@59.940200805664062	1280x720  	59.94     	[x1.00+]
                   1152x864@75	1152x864  	75.00     	[x1.00+]
   1152x864@59.958633422851562	1152x864  	59.96     	[x1.00+]
   1152x864@59.801021575927734	1152x864  	59.80     	[x1.00+]
   1024x768@75.028579711914062	1024x768  	75.03     	[x1.00+]
   1024x768@70.069358825683594	1024x768  	70.07     	[x1.00+]
   1024x768@60.003841400146484	1024x768  	60.00     	[x1.00+]
                    800x600@75	800x600   	75.00     	[x1.00+]
    800x600@72.187568664550781	800x600   	72.19     	[x1.00+]
    800x600@60.316539764404297	800x600   	60.32     	[x1.00+]
                 800x600@56.25	800x600   	56.25     	[x1.00+]
is-underscanning: false
is-builtin: false
display-name: "Telecom Technology Centre Co. Ltd. 23\""

HDMI-3 DEL DELL U2713HM 7JNY5488A9CS
                  1920x1080@60	1920x1080 	60.00*+   	[x1.00+, x2.00]
  1920x1080@59.940200805664062	1920x1080 	59.94     	[x1.00+, x2.00]
                  1920x1080@50	1920x1080 	50.00     	[x1.00+, x2.00]
                  1920x1080@24	1920x1080 	24.00     	[x1.00+, x2.00]
  1920x1080@23.976079940795898	1920x1080 	23.98     	[x1.00+, x2.00]
  1680x1050@59.954250335693359	1680x1050 	59.95     	[x1.00+, x2.00]
                  1600x1200@60	1600x1200 	60.00     	[x1.00+, x2.00]
  1280x1024@75.024673461914062	1280x1024 	75.02     	[x1.00+]
  1280x1024@60.019741058349609	1280x1024 	60.02     	[x1.00+]
   1280x800@59.810325622558594	1280x800  	59.81     	[x1.00+]
                   1280x720@60	1280x720  	60.00     	[x1.00+]
   1280x720@59.940200805664062	1280x720  	59.94     	[x1.00+]
                   1280x720@50	1280x720  	50.00     	[x1.00+]
                   1152x864@75	1152x864  	75.00     	[x1.00+]
   1024x768@75.028579711914062	1024x768  	75.03     	[x1.00+]
   1024x768@60.003841400146484	1024x768  	60.00     	[x1.00+]
                    800x600@75	800x600   	75.00     	[x1.00+]
    800x600@60.316539764404297	800x600   	60.32     	[x1.00+]
                    720x576@50	720x576   	50.00     	[x1.00+]
is-builtin: false
display-name: "Dell Inc. 27\""

Hook25 · 2024-06-17T10:08:33Z

I'm closing this as it doesn't solve the issue we had and this was solved another way by @p-gentili

kissiel added 6 commits May 1, 2024 21:44

add parsers for xrandr and gnome-randr

644301e

add function listing display modes

4c05bc8

extract only the output name

29b93ea

use the new parser in edid_cycle

eadc6b3

fix check-output call

80f5828

clear EDID at the end

0560c94

kissiel added 3 commits May 6, 2024 12:23

make black happy

620e339

another black driven fixes

2d35b3b

mock clearing of edid

89ec219

Hook25 requested changes May 7, 2024

View reviewed changes

p-gentili requested changes May 21, 2024

View reviewed changes

Hook25 closed this Jun 17, 2024

p-gentili mentioned this pull request Jun 21, 2024

Set extend mode after plugging a new monitor (BugFix) #1286

Merged

Hook25 deleted the fix-edid-jobs branch March 11, 2026 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements in EDID cycling with Zapper (BugFix)#1229

Improvements in EDID cycling with Zapper (BugFix)#1229
kissiel wants to merge 9 commits into
mainfrom
fix-edid-jobs

kissiel commented May 6, 2024

Uh oh!

codecov Bot commented May 6, 2024 •

edited

Loading

Uh oh!

Hook25 left a comment

Uh oh!

Hook25 May 7, 2024

Uh oh!

Hook25 May 7, 2024

Uh oh!

Hook25 May 7, 2024

Uh oh!

Hook25 May 7, 2024

Uh oh!

p-gentili left a comment

Uh oh!

Hook25 commented Jun 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kissiel commented May 6, 2024

Description

Resolved issues

Tests

Uh oh!

codecov Bot commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hook25 left a comment

Choose a reason for hiding this comment

Uh oh!

Hook25 May 7, 2024

Choose a reason for hiding this comment

Uh oh!

Hook25 May 7, 2024

Choose a reason for hiding this comment

Uh oh!

Hook25 May 7, 2024

Choose a reason for hiding this comment

Uh oh!

Hook25 May 7, 2024

Choose a reason for hiding this comment

Uh oh!

p-gentili left a comment

Choose a reason for hiding this comment

Uh oh!

Hook25 commented Jun 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 6, 2024 •

edited

Loading