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

Fix allow_duplicates behavior in set_brightness() for relative string values #37

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

BingoWon
Copy link
Contributor

@BingoWon BingoWon commented Mar 4, 2024

This PR addresses a bug related to the allow_duplicates in the set_brightness() when the value is a relative string value (e.g., "+20" or "-10").

During the process of adding pytest cases for this scenario, I noticed a discrepancy in the definition of "duplicates". In the current implementation, it is supposed that duplicates could occur in two scenarios:

  1. The same monitor is connected through different interfaces at the same time.
  2. Different monitors share identical identifiers, including EDID, due to careless manufacturer.

In both scenarios, the list_monitors_info() function returns the same output: different indices but identical other fields. This behavior was observed on a Windows 11 system.

[{'name': 'None Generic Monitor',
'model': 'Generic Monitor',
'serial': '',
'manufacturer': None,
'manufacturer_id': 'IPS',
'edid': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc',
'method': screen_brightness_control.screen_brightness_control.windows.VCP,
'index': 0},
{'name': 'None Generic Monitor',
'model': 'Generic Monitor',
'serial': '',
'manufacturer': None,
'manufacturer_id': 'IPS',
'edid': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc',
'method': screen_brightness_control.screen_brightness_control.windows.VCP,
'index': 1}]

However, in the mock modules (helpers_mock.py and os_module_mock.py), the first and third displays share the same index but have many different other fields.

[{
'name': 'Brand Display1',
'model': 'Display1',
'manufacturer': 'Brand',
'manufacturer_id': 'BRD',
'serial': 'serial1',
'edid': '00ffffffffff00edid1',
'method': cls,
'index': 0
}]
[{
'name': 'Brand Display3',
'model': 'Display3',
'manufacturer': 'Brand',
'manufacturer_id': 'BRD',
'serial': 'serial3',
'edid': '00ffffffffff00edid3',
'method': cls,
'index': 0
}]

Do you suggest any modifications around allow_duplicates?

@Crozzers
Copy link
Owner

Crozzers commented Mar 4, 2024

the first and third displays share the same index

The index is not "global" but is scoped to the OS specific method that controls that monitor. The test displays are configured to represent different sets of monitors that can be controlled in different ways. In the real world, this might look like this:

# first laptop display detected
sbc.windows.WMI.get_brightness(display=0)
# first desktop monitor detected. Same index but definitely not the same as WMI[0]
sbc.windows.VCP.get_brightness(display=0)

This means that using the index as the identifier inside set_brightness will cause issues:

import screen_brightness_control as sbc
monitors = [
  {'method': sbc.windows.WMI, 'index': 0, 'name': 'Display A'},
  {'method': sbc.windows.VCP, 'index': 0, 'name': 'Display B'},
]
for monitor in sbc.filter_monitors(haystack=monitors):
  # will call with `display=0` twice, only setting the laptop display
  sbc.set_brightness(100, display=monitor['index'])

We could either use the Display class to get around this, or we could add the method kwarg to our set_brightness call.

for monitor in sbc.filter_monitors(haystack=monitors):
  # option 1:
  display_instance = Display.from_dict(monitor)
  # calls the OS method directly with the method-scoped index
  display.set_brightness(100)

  # option 2
  # includes all the error handling from __brightness
  set_brightness(100, display=monitor['index'], method=monitor['method'])

Option 1 is probably more efficient but I don't really mind which way it goes

@Crozzers
Copy link
Owner

Crozzers commented Mar 4, 2024

I'm also starting to wonder if, for your monitors, there may be more information in the EDID extension blocks. The following code should get the full edid for each of your monitors:

import screen_brightness_control as sbc
import struct
from pprint import pprint

wmi = sbc.windows._wmi_init()
edids = {}
for monitor in wmi.WmiMonitorDescriptorMethods():
    base = ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(0)[0])
    following_blocks = struct.unpack(sbc.helpers.EDID.EDID_FORMAT, bytes.fromhex(base))[-2]
    blocks = [base] + [
        ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(i)[0])
        for i in range(1, following_blocks + 1)
    ]
    edids[monitor.InstanceName] = ''.join(blocks)

pprint(edids)
print('All EDIDs identical:', len(set(edids.values())) == 1)

If they're not all identical, it may be worth figuring out how to add parsing for EDID extension blocks. I haven't up until now since most displays seem to store all the identifying info in the first edid block

@BingoWon
Copy link
Contributor Author

BingoWon commented Mar 7, 2024

I'm also starting to wonder if, for your monitors, there may be more information in the EDID extension blocks. The following code should get the full edid for each of your monitors:

import screen_brightness_control as sbc
import struct
from pprint import pprint

wmi = sbc.windows._wmi_init()
edids = {}
for monitor in wmi.WmiMonitorDescriptorMethods():
    base = ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(0)[0])
    following_blocks = struct.unpack(sbc.helpers.EDID.EDID_FORMAT, bytes.fromhex(base))[-2]
    blocks = [base] + [
        ''.join(f'{char:02x}' for char in monitor.WmiGetMonitorRawEEdidV1Block(i)[0])
        for i in range(1, following_blocks + 1)
    ]
    edids[monitor.InstanceName] = ''.join(blocks)

pprint(edids)
print('All EDIDs identical:', len(set(edids.values())) == 1)

If they're not all identical, it may be worth figuring out how to add parsing for EDID extension blocks. I haven't up until now since most displays seem to store all the identifying info in the first edid block

{'DISPLAY\\IPS2800\\5&21344eb6&0&UID4355_0': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc02032af24f610102030410131f292f3f4060101fe200c023097f0783010000e305c000e6060501626200023a801871382d40582c25006b5c2100001aab5e00a0a0a02d50302045006b5c2100001a56bd70a080a02d50302045006b5c2100001a34e300a0a0a02d50302045006b5c2100009a00000000000000000000000000d370123f000003003c11f90184ff0e030157002b006f081d0002000f004b7a0104ff0e2f02af0057006f08280002000f00cfa00104ff0ec7002f001f006f08280002000f00a50000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000090',
 'DISPLAY\\IPS2800\\5&21344eb6&0&UID4357_0': '00ffffffffffff00261300280000000020210104c53e23782987e5a456509e260d5054210800d1c0010181c0a9c0010101010101010150d000a0f0703e80302035006b5c2100001e000000fc005532383047410a202020202020000000fd003090ffff8c010a202020202020000000ff000a20202020202020202020202002bc02032af24f610102030410131f292f3f4060101fe200c023097f0783010000e305c000e6060501626200023a801871382d40582c25006b5c2100001aab5e00a0a0a02d50302045006b5c2100001a56bd70a080a02d50302045006b5c2100001a34e300a0a0a02d50302045006b5c2100009a00000000000000000000000000d370123f000003003c11f90184ff0e030157002b006f081d0002000f004b7a0104ff0e2f02af0057006f08280002000f00cfa00104ff0ec7002f001f006f08280002000f00a50000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000090'}
All EDIDs identical: True

@Crozzers
Copy link
Owner

Crozzers commented Mar 8, 2024

LGTM. Thanks for this.

@Crozzers Crozzers merged commit 4aa12f2 into Crozzers:dev Mar 8, 2024
9 checks passed
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