davidhi7 / ddcci-plasmoid

KDE Plasma widget to adjust the brightness of multiple external monitors
MIT License
390 stars 11 forks source link

Concurrently executed ddcutil getvcp lead to sporadic undetected monitor #47

Open ChristianAnke opened 9 months ago

ChristianAnke commented 9 months ago

I have two identical monitors and only one of them is detected. The issue seems to be the concurrently executed ddcutil getvcp which lead to an error which renders one monitor undetected.

Output from python3 -m ddcci_plasmoid_backend -d detect:

DEBUG __main__: backend version: 0.1.8
DEBUG __main__: ddcutil version: 1.4.1
DEBUG __main__: argv: /home/christian-anke/.local/lib/python3.11/site-packages/ddcci_plasmoid_backend/__main__.py -d detect
DEBUG ddcci_plasmoid_backend.ddcci: Execute command: `ddcutil detect`
DEBUG ddcci_plasmoid_backend.ddcci: [code]   ddcutil detect: 0
DEBUG ddcci_plasmoid_backend.ddcci: [stdout] ddcutil detect: Invalid display
   I2C bus:  /dev/i2c-12
   DRM connector:           card0-eDP-1
   EDID synopsis:
      Mfg id:               SHP - Sharp Corporation
      Model:                
      Product code:         5329  (0x14d1)
      Serial number:        
      Binary serial number: 0 (0x00000000)
      Manufacture year:     2020,  Week: 4
   DDC communication failed
   This is an eDP laptop display. Laptop displays do not support DDC/CI.

Phantom display
   I2C bus:  /dev/i2c-13
   DRM connector:           card0-DP-1
   EDID synopsis:
      Mfg id:               MSI - Microstep
      Model:                MSI G241
      Product code:         15268  (0x3ba4)
      Serial number:        
      Binary serial number: 803 (0x00000323)
      Manufacture year:     2021,  Week: 14
   DDC communication failed
   Use non-phantom device bus /dev/i2c-16

Display 1
   I2C bus:  /dev/i2c-16
   DRM connector:           card0-DP-4
   EDID synopsis:
      Mfg id:               MSI - Microstep
      Model:                MSI G241
      Product code:         15268  (0x3ba4)
      Serial number:        
      Binary serial number: 803 (0x00000323)
      Manufacture year:     2021,  Week: 14
   VCP version:         2.1

Display 2
   I2C bus:  /dev/i2c-17
   DRM connector:           card0-DP-5
   EDID synopsis:
      Mfg id:               MSI - Microstep
      Model:                MSI G241
      Product code:         15268  (0x3ba4)
      Serial number:        
      Binary serial number: 120 (0x00000078)
      Manufacture year:     2020,  Week: 44
   VCP version:         2.1
DEBUG ddcci_plasmoid_backend.ddcci: [stderr] ddcutil detect: 

DEBUG ddcci_plasmoid_backend.ddcci: Found 4 entries at root level
DEBUG ddcci_plasmoid_backend.ddcci: Key Invalid display does not match pattern for valid display, so skip it
DEBUG ddcci_plasmoid_backend.ddcci: Key Phantom display does not match pattern for valid display, so skip it
DEBUG ddcci_plasmoid_backend.ddcci: Execute command: `ddcutil getvcp --bus 16 --brief 10`
DEBUG ddcci_plasmoid_backend.ddcci: Execute command: `ddcutil getvcp --bus 17 --brief 10`
DEBUG ddcci_plasmoid_backend.ddcci: [code]   ddcutil getvcp --bus 16 --brief 10: 1
DEBUG ddcci_plasmoid_backend.ddcci: [stdout] ddcutil getvcp --bus 16 --brief 10: No monitor detected on bus /dev/i2c-16
DEBUG ddcci_plasmoid_backend.ddcci: [stderr] ddcutil getvcp --bus 16 --brief 10: 

DEBUG ddcci_plasmoid_backend.ddcci: [code]   ddcutil getvcp --bus 17 --brief 10: 1
DEBUG ddcci_plasmoid_backend.ddcci: [stdout] ddcutil getvcp --bus 17 --brief 10: VCP 10 C 100 100
DEBUG ddcci_plasmoid_backend.ddcci: [stderr] ddcutil getvcp --bus 17 --brief 10: 

DEBUG __main__: Command 'ddcutil getvcp --bus 16 --brief 10' returned non-zero exit status 1.
DEBUG __main__: Detected 1 working monitor bus, 1 non-working bus.
{"command": "detect", "value": [{"id": 2, "name": "MSI G241", "bus_id": 17, "brightness": 100}]}

And this can be reproduced with simple shell: ddcutil getvcp --bus 16 --brief 10 & ddcutil getvcp --bus 17 --brief 10

[1] 16898
No monitor detected on bus /dev/i2c-16
VCP 10 C 100 100
[1]+  Exit 1                  ddcutil getvcp --bus 16 --brief 10

Were as executing them in sequence makes the error safely disappear: ddcutil getvcp --bus 16 --brief 10; ddcutil getvcp --bus 17 --brief 10

VCP 10 C 100 100
VCP 10 C 100 100

It might be better to avoid the async execution here.

davidhi7 commented 8 months ago

I'll test how much this affects the execution time of the detect command. Gathering all monitor attributes does already take a lot of time and thus makes the widget pretty slow when it comes to detecting recently connected monitors. Avoiding asynchronous ddcutil calls would slow this down even further.

philippfriese commented 8 months ago

I can confirm the race-condition stated by OP. I was able to resolve this by making fetch_monitor_data use the non-async subprocess_wrappper and making that function itself non-async. With two monitors attached, I do not notice a significant slowdown. (Which even if wouldn't be that bad anyway as this is only performed on widget-bringup I think.)

output from git diff ```diff index f3a2674..38025f4 100644 --- a/backend/ddcci_plasmoid_backend/ddcci.py +++ b/backend/ddcci_plasmoid_backend/ddcci.py @@ -45,7 +45,7 @@ class MonitorID: async def detect(): - async def fetch_monitor_data(node: Node) -> MonitorData: + def fetch_monitor_data(node: Node) -> MonitorData: display_id = get_monitor_id(node) display_name = "" if "EDID synopsis" in node.child_by_key: @@ -56,7 +56,7 @@ async def detect(): bus_id = int(re.search(r"\d+$", node.child_by_key["I2C bus"].value).group()) - result = await async_subprocess_wrapper( + result = subprocess_wrapper( f"ddcutil getvcp --bus {bus_id} --brief {brightness_feature_code:x}" ) @@ -107,8 +107,7 @@ async def detect(): awaitables.append(fetch_monitor_data(child)) - return await asyncio.gather(*awaitables, return_exceptions=True) + return awaitables def set_brightness(bus_id: int, brightness: int) -> None: subprocess_wrapper( ```
davidhi7 commented 8 months ago

Considering the minor slowdowns I'll disable the concurrent ddcutil calls for now. The upcoming major release will feature an option to synchronize the brightness between all screens, which would benefit from faster execution. But until then, there no really benefit with calling ddcutil concurrently.

davidhi7 commented 8 months ago

Fixed in ddcci-plasmoid-backend 0.1.10 for now.