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

Add support for Acer XV273K (ACR06B1) #114

Merged
merged 3 commits into from Sep 4, 2020

Conversation

SimonAlling
Copy link
Contributor

No description provided.

Copy link
Contributor

@kravemir kravemir left a comment

Choose a reason for hiding this comment

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

Seems good.

Please, remove duplicate controls from VESA profile, and only include the VESA profile:

<include file="VESA"/>

db/monitor/ACR06B1.xml Outdated Show resolved Hide resolved
db/monitor/ACR06B1.xml Outdated Show resolved Hide resolved
@SimonAlling
Copy link
Contributor Author

Thanks for your reply! I'm as strong of a DRY advocate as they come, so I appreciate your feedback regarding the VESA profile. IIRC, there was actually a legit reason why I chose not to include it, but I'll have to get back to you once I get home. Hopefully I can find a more DRY solution. 🙂

@SimonAlling
Copy link
Contributor Author

Alright, @kravemir. The reason why I didn't include the VESA profile was because then I couldn't get rid of the Secondary Degauss control (which isn't supported by XV273K and makes no sense for an LCD monitor anyway). Surprisingly, while the presence or absence of 00 in the caps string corresponds to the presence or absence of the Degauss control, removing 02 from the caps string doesn't remove the Secondary Degauss control. No matter what I do, I can't get rid of it.

These statements are true of the output of ddccontrol -v -p:

  • > id=degauss, name=Degauss, address=0, delay=-1ms, type=1 appears if and only if 00 is in the vcp part of the caps string (as expected).
  • Control degauss has been discarded by the caps string. appears if and only if 00 is not in the vcp part of the caps string (as expected).
  • > id=secdegauss, name=Secondary Degauss, address=0x2, delay=-1ms, type=1 always appears.
  • Control secdegauss has been discarded by the caps string. never appears.

@SimonAlling
Copy link
Contributor Author

Thanks to Vojtěch Erben's PR (#125), I found a way to disable the unsupported Degauss control, so I could fix the issues you pointed out, @kravemir. 😃

@SimonAlling
Copy link
Contributor Author

I'd also like to suggest squashing PRs instead of creating merge commits, even though the latter happened to help me out this time.

@kravemir
Copy link
Contributor

kravemir commented Sep 4, 2020

@SimonAlling thank you for the effort you've spent preparing this PR, and applying the comment, good job!

I'm sorry for not responding before, and not directing you at the possibility to remove caps using remove="(vcp(02))". Alternatively, it's possible to just not list it within ...cmds(01 02 03 07 0C E3 F3)... (remove the 02 entry).

I'd also like to suggest squashing PRs instead of creating merge commits, even though the latter happened to help me out this time.

That's a good idea. Also, github complains "This branch is out-of-date with the base branch", so it would be nice, if you could merge the latest master into the PR's branch.

And, as I'm not a maintainer anymore, @larstobi could take over, and merge it.

Copy link
Member

@larstobi larstobi left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks!

@larstobi larstobi merged commit 3d29bf9 into ddccontrol:master Sep 4, 2020
@larstobi
Copy link
Member

larstobi commented Sep 4, 2020

I'd also like to suggest squashing PRs instead of creating merge commits, even though the latter happened to help me out this time.

Thanks for the suggestion!

Why would you like squashing PRs?

I think merge commits are useful in that they give a link to the PR, which often contains much information. I do realize that they create a non-linear history.

@SimonAlling
Copy link
Contributor Author

SimonAlling commented Sep 4, 2020

Why would you like squashing PRs?

Because in the Git log for master, we're usually interested in commits like "Add support for Acer XV273K" and not so much in ones like "Fix spelling mistake" or other commits that just correct something that was wrong in the first commit on the PR branch.

Also tends to make the log easier to read in general. Tip right now:

*   6c477a0 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #126 from odin-/dell_u3219q
|\  
| * 8c32473 Add DELL U3219Q support
* |   ac0096c Merge pull request #120 from SimonAlling/dependencies
|\ \  
| * | 9d8f727 Merge branch 'master' into dependencies
| |\| 
| * | 12b2fb8 Update build dependencies example
* | |   3520a9a Merge pull request #127 from SimonAlling/docs-disabling-controls
|\ \ \  
| * | | 888dd74 Document how to disable controls

I think merge commits are useful in that they give a link to the PR, which often contains much information.

So do squash commits. See for example this one.

@SimonAlling SimonAlling deleted the acer-xv273k branch September 4, 2020 21:13
@SimonAlling
Copy link
Contributor Author

I'm sorry for not responding before, and not directing you at the possibility to remove caps using remove="(vcp(02))".

No worries!

Alternatively, it's possible to just not list it within ...cmds(01 02 03 07 0C E3 F3)... (remove the 02 entry).

Surprisingly, removing 02 from cmds (and from vcp of course) doesn't disable the secdegauss control on my XV273K – I can't get it to work without the remove attribute.

@larstobi
Copy link
Member

@SimonAlling I agree. I've configured this repo to only allow Squash from now on. Thanks!

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.

None yet

3 participants