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

Added support for DELL U3818DW and PBP/PIP controls #71

Merged
merged 5 commits into from
Sep 2, 2018

Conversation

aryoda
Copy link
Contributor

@aryoda aryoda commented Sep 1, 2018

I have created a DB file for the monitor + extended the options.xml to support picture-by-picture (PBP/PIP) settings which split the screen into two parts an allow the visualisation of different input signals in each part (e. g. two computers).

See issue #70

For a better usability I have combined added the input source selection for every "split screen" into the same sub group.

The settings look like this:
screenshot for del u3818dw

Other monitors supporting PBP/PIB could reuse my control defintions in the options.xmlfile...

incl. vendor-specific VCP codes for split screen (PBP/PIP)
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.

There's a mistake in monitor's name. And, minor issues about source formatting. Please, keep formatting consistent with original formatting, for good readability.

@@ -375,6 +375,7 @@
<value id="component" name="Component"/>
<value id="s-video" name="S-Video"/>
<value id="composite" name="Composite"/>
<value id="usb-c" name = "USB-C" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, fix indentation, use tabs, as whole file uses tabs.

<value id="component" name="Component"/>
<value id="s-video" name="S-Video"/>
<value id="composite" name="Composite"/>
<value id="usb-c" name = "USB-C" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, fix indentation, use tabs only, and not mixture of tabs and spaces, as whole file uses tabs.

<value id="component" name="Component"/>
<value id="s-video" name="S-Video"/>
<value id="composite" name="Composite"/>
<value id="usb-c" name = "USB-C" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, fix indentation, use tabs only, and not mixture of tabs and spaces, as whole file uses tabs.

@@ -0,0 +1,31 @@
<?xml version="1.0"?>
<!-- DELL UltraSharp 38 Monitor (Model 2017) -->
<monitor name="DELL U8318DW" init="standard">
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, just mistake: DELL U8318DW

According to link, it's Dell U3818DW(DisplayPort), which matches the PR's name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is a mistake, I will fix it - sharp eyes ;-)

@kravemir
Copy link
Contributor

kravemir commented Sep 1, 2018

Thanks for creation of a PR. I did review it, and requested small changes. Otherwise, it seems fine.

@kravemir
Copy link
Contributor

kravemir commented Sep 1, 2018

@aryoda, can you please test, how this works via other inputs - HDMI 1.4, HDMI 2.0, ...? Taking another look at: https://www.driverscloud.com/en/drivers/manufacturercat/90-4-28/dell-monitors and, this monitor seems to change ID based on input source.

@aryoda
Copy link
Contributor Author

aryoda commented Sep 1, 2018

@kravemir Tabs are fixed now, thanks for keeping an eye on consistent code and configs!

can you please test, how this works via other inputs - HDMI 1.4, HDMI 2.0, ...?

How exactly can I test this? Since the HDMI inputs support older HDMI versions I guess I have to find an old computer with HDMI 1.4.

The OSD settings do not offer me a setting to fall back to an older HDMI version.

And: Which symptoms could happen? That the display is no longer recognized due to a changed Plug&Play-ID...?

@kravemir
Copy link
Contributor

kravemir commented Sep 1, 2018

How exactly can I test this? Since the HDMI inputs support older HDMI versions I guess I have to find an old computer with HDMI 1.4.

Didn't see this monitor settings,.. but I guess, there's some option like Enable HDMI 2.0 -> True/False, or Suported HDMI version -> 1.4 / 2.0.

The OSD settings do not offer me a setting to fall back to an older HDMI version.

Aha, there's no such setting,.. Then, just test what you can.

And: Which symptoms could happen? That the display is no longer recognized due to a changed Plug&Play-ID...?

Based on the ID, is select monitor descriptor, that's why you named it DELA0F3.xml.

Probably, you'll just create new DELA???.xml, which would include DELA0F3.xml.

<value id="PiP large" value="0x22"/>
<value id="PiP small" value="0x21"/>
</control>
<control id="maininputsource" type="list" address="0x60">
Copy link
Contributor

Choose a reason for hiding this comment

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

The address 0x60 is standard VESA, for: <control id="inputsource" ... . Why did you create another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this is for usability reasons within PBP/PIP to keep the sensible settings together. Interestingly the pure VESA config does not show the input setting by default. I could duplicate the control in the DELA0F3.xml setting as "Input Source" (standard VESA in "others") and in "PBP/PIP" ("main input source") to improve the usability even further (if gddccontrol does support this)...

<value id="PiP large" name="PiP Large"/>
<value id="PiP small" name="PiP Small"/>
</control>
<control id="maininputsource" type="list" name="Input Source Select (Main)">
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicates <control id="inputsource" ... . Is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem lies in the config.xml: I would like to capture the allowed values for input sources only once and reuse them for PBP/PIP settings but they are bound within a group tag (were I do not want to place them - the group name "others" is definitely the last group I would search for the input source). And for a user to understand that the sub input source is in the PBP/PIP (sub) group but to set the input source for one of the split screens is in the "others" group under "input settings" is difficult to understand.

I will follow your usability proposals anyhow, please just suggest what you want and I push a new commit.

@aryoda
Copy link
Contributor Author

aryoda commented Sep 1, 2018

@kravemir

can you please test, how this works via other inputs - HDMI 1.4, HDMI 2.0, ...?

Wow, good point, how did you know that?

I have used an HDMI-only notebook with Ubuntu 18.04 and ddccontrol.

According to the EDID readings of ddccontrol -p the U3818DW is saying DELA0F1 for HDMI 1.4 and DELA0F2 for HDMI 2.0 - I could not select the HDMI version manually but luckily the PBP mode seems to fall back to HDMI 1.4 (shown in the OSD info).

I will add two more config files including DELA0F3.xml (which is the ID for DisplayPort connections)
and send a new commit...

@aryoda
Copy link
Contributor Author

aryoda commented Sep 1, 2018

NB: I could not test the EDID ID for DisplayPort over USB-C since I have no computer supporting this...

via HDMI 1.4, HDMI 2.0 and DisplayPort
@aryoda
Copy link
Contributor Author

aryoda commented Sep 1, 2018

I have just seen in your link that DELL differentiates the EDID by the HDMI input port (HDMI1 or HDMI2).

I will push another commit to support all EDIDs later (tomorrow I guess)...

…ia manuall testing (except USB Type-c)

DisplayPort, HDMI 1 + 2 with version 1.4 and 2.0, USB Type-C.
USB Type-C could not be tested due to lack of test hardware
availability.
@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2018

OK, here a short summary of the status quo:

I have added support for all known EDIDs now and tested them successfully (DP, HDMI1 + HDMI2 with v1.4 and v2.0) - except USB Type-C since I have no hardware for that

The usability decision for the input source (VCP code 0x60) is still open:

Where to put the (main) input source select (VCD code 0x60)?

  • In case of monitors without support for PBP/PIP it is in the "others" group currently
  • In case of monitors with PCP/PIP support (only DELL currently) it is in the PCP/PIP group together with the input source selector for the sub input(s). This is for usability reasons since in case of split screens a user often changes two input sources at once and changing the group in the menu for doing this means more clicks and is less intuitive (you have to understand that one input source is set in the "others" group while the other(s) in the "PCP/PIP" group.

On the other side: If you work with PBP "off" (= single screen) you do not expect input source selector in the "PBP/PIP" menu.

Possible solutions:

  1. Move the PCP/PIP sub group to "others" and move add the sub input source selector into the same sub group as the current input source (in the "others" group)

  2. Rename the PCP/PIP group to "Input Settings", move the "inputsource" control from "others" into this group and add the sub input source selector(s) from the PCP/PIP settings sub gruop into the "input settings" sub group too.

I will upload some screen shots for both propsals here later so that my proposals become clearer...

@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2018

Visual mock for option 1:

option 1 - others - input settings

option 1 - others pip_pbp settings

Visual mock for option 2:

option 2 - inut settings - pip_pbp

option 2 - inut settings - input sources

IMHO

  • Option 1 is the least intrusive solution (no big changes) but "polutes" the "others" group.
  • Option 2 cleans-up the "others" group but also requires to refactor other controls in the input sub group of the "others" group (mainly "inputlevel" and "resolutionnotifier")

Other proposals welcome!

@kravemir
Copy link
Contributor

kravemir commented Sep 2, 2018

I have added support for all known EDIDs now and tested them successfully (DP, HDMI1 + HDMI2 with v1.4 and v2.0) - except USB Type-C since I have no hardware for that

Great!

Where to put the (main) input source select (VCD code 0x60)?

Thank you for provided examples. I'm for option 2, create Input Settings group, and move/refactor Others -> Input Settings sub-group there. Probably, it makes sense to put it as the last group, but before Others ?

To keep compatibility with existing monitors, keep the id of inputsource, you may rename it to Input Source. And, use that instead of maininputsource.

And, other one can be inputsource_secondary, or inputsource_sub1 (keeps space for inputsource_sub2...).

Extra idea. I thought of control name override for specific monitors, or monitor specific (OSD) name. Especially, dynamic contrast has got many different OSD names. But, that requires much more effort.

So, PR's status from my point of view. Seems good, just remove duplicated control, and refactor/move this input settings controls, as you want to have all input settings (input source and PIP) in one place.

@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2018

OK, I will push the proposed changes later today, thanks for your feed back :-)

…Adjusted config for DELL U3818DW accordingly.
@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2018

Refactored now, the "release candidate" looks like this:

screenshot from 2018-09-02 13-04-53

@kravemir
Copy link
Contributor

kravemir commented Sep 2, 2018

Refactored now, the "release candidate" looks like this:

Looks good 👍

@kravemir
Copy link
Contributor

kravemir commented Sep 2, 2018

I'll do (probably) final PR review after I'll arrive home, and merge it, if everything's fine.

@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2018

I'll do (probably) final PR review after I'll arrive home, and merge it, if everything's fine.

Perfect (no need to hurry, I have a local installation now).

Thanks for your support :-)

@kravemir kravemir merged commit d294da1 into ddccontrol:master Sep 2, 2018
@kravemir
Copy link
Contributor

kravemir commented Sep 2, 2018

I have merged the PR. I have had taken a look at the code. It seems fine, hope I didn't miss something.

The monitor seems tempting, though.. out of my price range. Do you use such "wow" monitor for work (on Linux), or just for personal stuff / hobby?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants