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

EDID test fails at maximum resolution (BugFix) #1393

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

p-gentili
Copy link
Collaborator

@p-gentili p-gentili commented Aug 6, 2024

Description

Another PR dedicated to the EDID job. It turns out that the new implementation (#1286) shed some light on the other related problem, where older machines could read the maximum allowed resolution.

In case where the HW doesn't support an available mode from the monitor EDID, no preferred mode is available (which makes sense). This PR takes care of such scenario, attempting to configure the monitor with the maximum available resolution.

  1. in case the maximum allowed resolution still matches what the test requested, the assertion over the actual configuration is performed as usual. Common example is with unsupported refresh rate.
  2. in case the maximum allowed resolution is lower than requested, the testcase can be skipped since we can assume the HW does not support it. Common example is 2K resolution with HDMI < 1.3

Resolved issues

Resolves ZAP-678

Documentation

N/A

Tests

Tested on some machines.

Previous behaviour:

Running from source:

Testing EDID cycling on HDMI-4
switching EDID to 1280x1024
PASS
switching EDID to 2560x1440
SKIP, max available was 2048x1152
switching EDID to 1920x1080
PASS

It's common to have a mismatch between the available modes
at EDID level and what the device actually supports. This commit
selects the maximum allowed resolution and returns the applied
configuration.
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 91.07143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 45.18%. Comparing base (7ab7c0f) to head (8c08ee4).
Report is 125 commits behind head on main.

Files with missing lines Patch % Lines
...box-support/checkbox_support/dbus/gnome_monitor.py 76.92% 2 Missing and 1 partial ⚠️
...heckbox-support/checkbox_support/monitor_config.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1393      +/-   ##
==========================================
+ Coverage   45.12%   45.18%   +0.05%     
==========================================
  Files         366      366              
  Lines       39058    39102      +44     
  Branches     6607     6609       +2     
==========================================
+ Hits        17626    17669      +43     
- Misses      20758    20760       +2     
+ Partials      674      673       -1     
Flag Coverage Δ
checkbox-support 59.69% <89.36%> (+0.25%) ⬆️
provider-base 18.65% <100.00%> (+0.03%) ⬆️

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.

@p-gentili p-gentili changed the title Extend to maximum resolution (BugFix) EDID test fails at maximum resolution (BugFix) Aug 6, 2024
@Hook25 Hook25 self-assigned this Aug 7, 2024
Hook25
Hook25 previously approved these changes Aug 7, 2024
Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

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

See if you like the change proposed below but it makes sense

checkbox-support/checkbox_support/monitor_config.py Outdated Show resolved Hide resolved
Co-authored-by: Massimiliano <massimiliano.girardi@canonical.com>
@p-gentili p-gentili force-pushed the extend-to-maximum-resolution branch from d067982 to 8c08ee4 Compare August 7, 2024 08:22
@Hook25 Hook25 merged commit 904692d into main Aug 7, 2024
44 checks passed
@Hook25 Hook25 deleted the extend-to-maximum-resolution branch August 7, 2024 08:32
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.

2 participants