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

display_control: Make display_name include monitor name, serial number, etc #56

Closed
wants to merge 1 commit into from

Conversation

pm215
Copy link
Contributor

@pm215 pm215 commented Feb 23, 2021

Hi; this pull request adds information to the display_name of each monitor, as I suggested in #54 .

Currently the display_name() function uses only the 'id' field of the ddc_hi::DisplayInfo struct, plus a possible numeric suffix to disambiguate otherwise-identical display names. At least on Linux, this leaves a lot of useful information out of the display name, because for the i2c-dev backend to ddc_hi the 'id' string is just the decimal representation of the (major,minor) of the i2c device node (e.g., '22789' == 0x5905 == device (59,05) == /dev/i2c-5).

There is usually more usefully identifying information in the DisplayInfo structure, including the 3-character manufacturer ID, the model name, and the monitor serial number. The pullreq adds this information to 'id' if it is present. The result is that instead of logging like

  Display '22790' is currently set to Custom(0x1111)

you get

  Display '22790 DEL DELL U2719D DT86VS2' is currently set to Custom(0x1111)

and you can also write monitor_id config file settings that match against the monitor model or serial number.

I guess in theory if users have config files with very short strings in their monitor_id settings then adding the extra info might cause them to match on monitors that previously didn't match; but this seems fairly unlikely in practice to me.

I'm not sure what Windows and OSX monitor ID strings look like, but adding the extra info doesn't seem like it could hurt.

NB: There are a lot of ways you could write the string-assembly in this function (eg heavy use of iterators); as somebody fairly new to rust I went for something that seemed to me fairly straightforward and easy to understand, and more-or-less in line with how the previous version of the function did things. Let me know if you'd like me to have a go at writing this in a different style.

…r, etc

Currently the display_name() function uses only the 'id' field of the
ddc_hi::DisplayInfo struct, plus a possible numeric suffix to disambiguate
otherwise-identical display names.  At least on Linux, this leaves a lot of
useful information out of the display name, because for the i2c-dev backend
to ddc_hi the 'id' string is just the decimal representation of the
(major,minor) of the i2c device node (e.g., '22789' == 0x5905 == device
(59,05) == /dev/i2c-5).

There is usually more usefully identifying information in the DisplayInfo
structure, including the 3-character manufacturer ID, the model name, and
the monitor serial number. Append this information to 'id' if it is
present. The result is that instead of logging like
  Display '22790' is currently set to Custom(0x1111)
you get
  Display '22790 DEL DELL U2719D DT86VS2' is currently set to Custom(0x1111)
and you can also write monitor_id config file settings that match
against the monitor model or serial number.
@haimgel
Copy link
Owner

haimgel commented Feb 28, 2021

Thanks for the PR! Yeah, Linux support is sub-par, comparing to MacOS and Windows, I personally use just these too and have no easy way to test on Linux.

So, on macOS and Windows display.info.id is already "good enough", for example for my displays it shows "LEN P27u-10 S/N 1144206897" -- as you see, this includes manufacturer, model and the serial number already. If we are going to append these again, it's going to be a bit too much... I also think this "string building" is a bit un-rusty.

I don't think this should be merged as-is, but this is a good start to make the display_name behave reasonably on all platforms.

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

2 participants