From 695b7fd43d372b07c3edc1209b5f14d21f1dc217 Mon Sep 17 00:00:00 2001 From: knowlen Date: Wed, 30 Sep 2026 18:29:56 -0700 Subject: [PATCH] Address monitors by I2C bus and serialize writes per display to cut brightness latency --- monitorsettings/base.py | 103 +++++++++++------- .../controllers/backlight/blessed.py | 1 - .../controllers/backlight/curses.py | 1 - tests/unit/test_base.py | 62 ++++++++++- 4 files changed, 127 insertions(+), 40 deletions(-) diff --git a/monitorsettings/base.py b/monitorsettings/base.py index 15b22ea..84a7da9 100644 --- a/monitorsettings/base.py +++ b/monitorsettings/base.py @@ -8,6 +8,14 @@ import time from typing import Dict, List, Optional, Tuple +# ddcutil flags for the per-change path. Addressing a monitor by display +# number (-d N) makes every ddcutil run re-detect all monitors first (~1 s on a +# machine with many I2C buses); --bus N skips that. --noverify drops the +# read-back after each write and --skip-ddc-checks the per-run capability +# probe. Measured on 2 x LG 4K over HDMI: -d 1.0-1.2 s, --bus + flags 0.10 s. +SET_FLAGS = ["--noverify", "--skip-ddc-checks"] +GET_FLAGS = ["--skip-ddc-checks"] + class DDCInterface: """ @@ -17,9 +25,12 @@ class DDCInterface: def __init__(self) -> None: self.displays: List[int] = [] + self.buses: Dict[int, int] = {} # display number -> I2C bus, from detect self._lock = threading.Lock() self._last_command_time = 0.0 - self._command_interval = 1.0 # inimum time between DDC commands + # Minimum time between synchronous DDC commands. ddcutil already + # enforces the DDC/CI protocol delays itself, so no extra gap is needed. + self._command_interval = 0.0 def check_ddcutil(self) -> bool: """Check if ddcutil is available on the system""" @@ -43,11 +54,23 @@ def detect_displays(self) -> List[int]: displays = re.findall(r"Display (\d+)", result.stdout) self.displays = [int(d) for d in displays] + # Remember each display's I2C bus so later commands can skip detection. + self.buses = {} + for block in re.split(r"(?m)^(?=Display \d+)", result.stdout): + d = re.match(r"Display (\d+)", block) + b = re.search(r"I2C bus:\s*/dev/i2c-(\d+)", block) + if d and b: + self.buses[int(d.group(1))] = int(b.group(1)) return self.displays except (subprocess.TimeoutExpired, subprocess.CalledProcessError, FileNotFoundError): return [] + def _target(self, display: int) -> List[str]: + """ddcutil arguments selecting a display: its bus if known, else its number.""" + bus = self.buses.get(display) + return ["--bus", str(bus)] if bus is not None else ["-d", str(display)] + def get_vcp_value(self, display: int, vcp_code: str) -> Tuple[Optional[int], Optional[int]]: """ Get a VCP (Virtual Control Panel) value from a display @@ -64,7 +87,7 @@ def get_vcp_value(self, display: int, vcp_code: str) -> Tuple[Optional[int], Opt self._wait_for_command_interval() result = subprocess.run( - ["ddcutil", "getvcp", vcp_code, "-d", str(display)], + ["ddcutil", "getvcp", vcp_code, *self._target(display), *GET_FLAGS], capture_output=True, text=True, timeout=2, @@ -98,7 +121,7 @@ def set_vcp_value(self, display: int, vcp_code: str, value: int) -> bool: self._wait_for_command_interval() subprocess.run( - ["ddcutil", "setvcp", vcp_code, str(value), "-d", str(display)], + ["ddcutil", "setvcp", vcp_code, str(value), *self._target(display), *SET_FLAGS], capture_output=True, timeout=3, check=True, @@ -121,7 +144,7 @@ def set_vcp_value_async(self, display: int, vcp_code: str, value: int) -> subpro Popen process object """ return subprocess.Popen( - ["ddcutil", "setvcp", vcp_code, str(value), "-d", str(display)], + ["ddcutil", "setvcp", vcp_code, str(value), *self._target(display), *SET_FLAGS], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) @@ -139,18 +162,24 @@ def _wait_for_command_interval(self) -> None: class AsyncDDCWorker: """ - Background worker for sending DDC commands asynchronously with debouncing + Background worker that sends DDC commands asynchronously. + + At most one ddcutil process is in flight per display (they share one I2C + bus), and when it finishes the newest pending value is sent next, so + intermediate values are skipped and writes can never land out of order. """ - def __init__(self, ddc_interface: DDCInterface, update_interval: float = 0.5): + def __init__(self, ddc_interface: DDCInterface, update_interval: float = 0.0): self.ddc = ddc_interface + # Optional extra gap between writes to the same display (seconds). self.update_interval = update_interval self.running = False self._thread: Optional[threading.Thread] = None self._lock = threading.Lock() self._pending_updates: Dict[Tuple[int, str], int] = {} self._last_sent: Dict[Tuple[int, str], int] = {} - self._last_update_time = 0.0 + self._in_flight: Dict[int, subprocess.Popen] = {} + self._last_send_time: Dict[int, float] = {} def start(self) -> None: """Start the background worker thread""" @@ -177,36 +206,36 @@ def queue_update(self, display: int, vcp_code: str, value: int) -> None: with self._lock: self._pending_updates[(display, vcp_code)] = value + def _busy(self, display: int) -> bool: + """True while a write to this display is still running.""" + p = self._in_flight.get(display) + if p is None: + return False + if p.poll() is None: + return True + del self._in_flight[display] + return False + def _worker_loop(self) -> None: - """Main worker loop for processing queued updates""" + """Send the newest pending value for each display as soon as it is free.""" while self.running: - time.sleep(0.05) # Check every 50ms - - current_time = time.time() - if current_time - self._last_update_time < self.update_interval: - continue - - updates_to_send = [] - + time.sleep(0.01) + now = time.time() + to_send = [] + claimed = set() # one write per display per pass with self._lock: - for key, value in self._pending_updates.items(): - if self._last_sent.get(key) != value: - updates_to_send.append((key, value)) - self._last_sent[key] = value - - self._pending_updates.clear() - - if updates_to_send: - self._last_update_time = current_time - processes = [] - - for (display, vcp_code), value in updates_to_send: - p = self.ddc.set_vcp_value_async(display, vcp_code, value) - processes.append(p) - - # Don't wait for completion - fire and forget - for p in processes: - try: - p.wait(timeout=0.01) - except subprocess.TimeoutExpired: - pass + for key, value in list(self._pending_updates.items()): + display = key[0] + if display in claimed or self._busy(display): + continue # keep it pending; the newest value wins + if now - self._last_send_time.get(display, 0.0) < self.update_interval: + continue + del self._pending_updates[key] + if self._last_sent.get(key) == value: + continue + self._last_sent[key] = value + claimed.add(display) + to_send.append((key, value)) + for (display, vcp_code), value in to_send: + self._in_flight[display] = self.ddc.set_vcp_value_async(display, vcp_code, value) + self._last_send_time[display] = now diff --git a/monitorsettings/controllers/backlight/blessed.py b/monitorsettings/controllers/backlight/blessed.py index d6ec5e9..7a91ff8 100644 --- a/monitorsettings/controllers/backlight/blessed.py +++ b/monitorsettings/controllers/backlight/blessed.py @@ -42,7 +42,6 @@ def run(self) -> None: print(f" Display {display}: {current}/{max_val}") print("\nStarting interactive mode...\n") - time.sleep(1) # Start background worker self.start_worker() diff --git a/monitorsettings/controllers/backlight/curses.py b/monitorsettings/controllers/backlight/curses.py index 4e7c741..c8ea640 100644 --- a/monitorsettings/controllers/backlight/curses.py +++ b/monitorsettings/controllers/backlight/curses.py @@ -100,7 +100,6 @@ def _init_displays(self) -> bool: self.stdscr.addstr(3 + i, 2, f"Display {display}: {current}/{max_val}") self.stdscr.refresh() - time.sleep(0.5) return True def draw_interface(self) -> None: diff --git a/tests/unit/test_base.py b/tests/unit/test_base.py index ee97bc4..be0ff23 100644 --- a/tests/unit/test_base.py +++ b/tests/unit/test_base.py @@ -56,6 +56,25 @@ def test_detect_displays(self, ddc_interface, mocker): assert displays == [1, 2] assert ddc_interface.displays == [1, 2] + assert ddc_interface.buses == {1: 1, 2: 2} + + def test_commands_address_detected_bus(self, ddc_interface, mocker): + """After detection, commands use --bus (no per-command detection)""" + mock_run = mocker.patch("subprocess.run") + mock_run.return_value = MagicMock( + stdout="Display 1\n I2C bus: /dev/i2c-2\n\nDisplay 2\n I2C bus: /dev/i2c-4\n" + ) + ddc_interface.detect_displays() + + mock_run.return_value = MagicMock(returncode=0) + ddc_interface.set_vcp_value(2, "0x10", 30) + + mock_run.assert_called_with( + ["ddcutil", "setvcp", "0x10", "30", "--bus", "4", "--noverify", "--skip-ddc-checks"], + capture_output=True, + timeout=3, + check=True, + ) def test_detect_displays_none_found(self, ddc_interface, mocker): """Test display detection when no displays found""" @@ -106,7 +125,7 @@ def test_set_vcp_value(self, ddc_interface, mocker): assert result is True mock_run.assert_called_with( - ["ddcutil", "setvcp", "0x10", "75", "-d", "1"], + ["ddcutil", "setvcp", "0x10", "75", "-d", "1", "--noverify", "--skip-ddc-checks"], capture_output=True, timeout=3, check=True, @@ -137,6 +156,7 @@ def test_command_interval_enforcement(self, ddc_interface, mocker): mock_sleep = mocker.patch("time.sleep") mock_time = mocker.patch("time.time") mock_time.side_effect = [0, 0, 0.2, 0.2, 0.3] # Simulate time passing + ddc_interface._command_interval = 1.0 ddc_interface._wait_for_command_interval() ddc_interface._wait_for_command_interval() @@ -216,3 +236,43 @@ def test_worker_deduplication(self, async_worker, mocker): async_worker._pending_updates.clear() assert mock_set_async.call_count == 0 + + def test_worker_one_write_per_display_newest_wins(self, async_worker, mocker): + """While a write is in flight, later values wait; only the newest is sent next""" + busy = MagicMock() + busy.poll.return_value = None # still running + done = MagicMock() + done.poll.return_value = 0 + mock_set_async = mocker.patch.object( + async_worker.ddc, "set_vcp_value_async", side_effect=[busy, done] + ) + async_worker.update_interval = 0.0 + async_worker.start() + async_worker.queue_update(1, "0x10", 10) + time.sleep(0.05) + async_worker.queue_update(1, "0x10", 11) + async_worker.queue_update(1, "0x10", 12) + time.sleep(0.05) + assert mock_set_async.call_count == 1 # 11 and 12 wait for the first write + + busy.poll.return_value = 0 # first write finishes + time.sleep(0.05) + async_worker.stop() + + assert [c.args[2] for c in mock_set_async.call_args_list] == [10, 12] + + def test_worker_displays_are_independent(self, async_worker, mocker): + """A busy display does not hold back another display""" + busy = MagicMock() + busy.poll.return_value = None + mock_set_async = mocker.patch.object( + async_worker.ddc, "set_vcp_value_async", return_value=busy + ) + async_worker.update_interval = 0.0 + async_worker.start() + async_worker.queue_update(1, "0x10", 10) + async_worker.queue_update(2, "0x10", 20) + time.sleep(0.05) + async_worker.stop() + + assert sorted(c.args[0] for c in mock_set_async.call_args_list) == [1, 2]