Skip to content

feat(brightness): control external monitors through DDC/CI - #1832

Merged
eythaann merged 2 commits into
eythaann:masterfrom
z1pp090:feat/ddcci-brightness
Sep 23, 2026
Merged

feat(brightness): control external monitors through DDC/CI#1832
eythaann merged 2 commits into
eythaann:masterfrom
z1pp090:feat/ddcci-brightness

Conversation

@z1pp090

@z1pp090 z1pp090 commented Sep 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #1826. Follows up on your comment there: this wires the dormant DDC/CI code in windows_api/monitor/brightness.rs into BrightnessManager as a second backend next to WMI.

What changes

  • Probe instead of trusting capabilities. Every physical monitor is probed with GetMonitorBrightness on a background thread. A monitor is exposed only if it answers with a sane range (min < max, current inside it). GetMonitorCapabilities is intentionally not used as a gate: my BenQ ZOWIE XL panels fail it yet answer brightness fine, and Microsoft warns that other monitors report capabilities they don't have. Writes are clamped to the range the monitor itself reported, which is the mitigation for the "could be dangerous" note in the docs.
  • Same UI type. Values are normalised to a 0-100 percentage regardless of the raw range, so MonitorBrightness, quick-settings and the flyout work unchanged. instance_name is the monitor's stable id for DDC/CI entries; set_brightness dispatches by instance name (DDC/CI first, then WMI).
  • Polling + rescan. DDC/CI has no change events, so known monitors are re-read every 5 s to pick up changes made from the monitor's OSD (one I2C round trip per monitor, ~60 ms here, on the worker thread). MonitorManager events (ViewAdded / ViewRemoved / ViewsChanged) trigger a full rescan for hotplug. A failed read keeps the last value; it never drops the monitor by itself.
  • One lock for all DDC/CI transactions (DDCCI_LOCK), so polling and slider writes never overlap on the I2C bus.
  • Handle leak fixed: PHYSICAL_MONITOR handles are now released with DestroyPhysicalMonitor (both the previously dead code and get_physical_monitors callers were leaking them).
  • WMI path is unchanged apart from moving the WmiMonitorBrightness → MonitorBrightness mapping into the manager.

Tested

Windows 11 25H2, desktop, two BenQ ZOWIE XL over DisplayPort, no WMI brightness on this machine:

  • Both monitors detected at startup (min 0 / current 100 / max 100), two sliders in quick-settings.
  • Moving each slider changes the panel; verified by reading the brightness back over dxva2 from a separate process (50 and 75 read back exactly).
  • Changing brightness from outside Seelen (simulating the OSD) is reflected in the slider on the next poll.
  • No errors or warnings in the log from the brightness module; cargo test, clippy, rustfmt clean.

Not tested (no hardware / scenario available): monitor hotplug at runtime, sleep/resume, laptops that have both an internal WMI panel and an external DDC/CI monitor, monitors whose raw range is not 0-100, and monitors that time out on DDC/CI (they would just be skipped at scan, but a monitor that answers the probe and later hangs would block the worker thread for the dxva2 timeout).

Follow-up ideas (not in this PR)

  • Show the monitor's friendly name next to each slider when there is more than one; that needs a new field on MonitorBrightness and regenerated bindings.
  • Make the poll interval configurable or pause it while all monitors are asleep.

🤖 Generated with Claude Code

@z1pp090 z1pp090 mentioned this pull request Sep 19, 2026
Closed
@greptile-apps

greptile-apps Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 3/5

The PR is not yet safe to merge because concurrent scans can overwrite completed brightness writes and mirrored multi-panel displays are collapsed to one arbitrary physical monitor.

Summary

This PR activates DDC/CI brightness support alongside WMI, including background probing and polling, hotplug-triggered rescans, percentage normalization, serialized device transactions, and ownership-based cleanup of physical-monitor handles.

  • Adds a DDC/CI worker and combines its monitor state with existing WMI brightness state.
  • Probes monitor-reported ranges and maps raw brightness values to UI percentages.
  • Polls external monitors and rescans them after monitor topology events.
  • Releases PHYSICAL_MONITOR handles through an RAII wrapper.
  • The new state-publication sequence has a stale-update race, and physical monitors are incorrectly collapsed to one entry per logical HMONITOR.

Reviews (1) · Last reviewed commit: "feat(brightness): control external monit..."

greptile-apps[bot]
greptile-apps Bot reviewed Sep 19, 2026
View reviewed changes
Comment thread src/background/modules/monitors/brightness/application.rs
Comment thread src/background/windows_api/monitor/brightness.rs Outdated
z1pp090 added a commit to z1pp090/Seelen-UI that referenced this pull request Sep 19, 2026
@z1pp090 @claude
fix(brightness): publish DDC/CI reads under the lock, keep every panel
01340b7 
Addresses the two review findings on eythaann#1832:

- A poll or scan could read a value, release `DDCCI_LOCK`, lose the
  race against a slider write, and then publish the stale value on top
  of the fresh one. `DDCCI_LOCK` now covers both the device transaction
  and the update of the shared state, and the write path re-resolves
  the panel under the lock so a rescan cannot swap its handle
  mid-flight.
- A cloned/mirrored display group shares one `HMONITOR` but yields
  several `PHYSICAL_MONITOR` handles; only the first was kept. Each
  panel is now its own entry (`<stable id>` for the first, `<stable
  id>#<index>` for the rest) with its own handle, which is held open
  for the lifetime of the entry instead of being reopened per call.

Re-tested on two BenQ ZOWIE XL: both sliders drive their panel, an
external change is picked up on the next poll, values read back over
dxva2 match.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01StkifHpSLWH4J5TvuP56ur
@eythaann

eythaann commented Sep 23, 2026

Copy link
Copy Markdown
Owner

@z1pp090 hi, I'm ready to merge this, but please fix the conflicts.

z1pp090 and others added 2 commits September 23, 2026 13:35
@z1pp090 @claude
feat(brightness): control external monitors through DDC/CI
d3c42d2 
Desktops with external displays had no brightness control at all: the
WMI provider only covers internal panels. This wires up the dormant
DDC/CI helpers in `windows_api::monitor::brightness` as a second backend
of `BrightnessManager`.

- Every physical monitor is probed with `GetMonitorBrightness` on a
  background thread, and only monitors that answer with a sane range
  (`min < max`, `current` inside it) are exposed. `GetMonitorCapabilities`
  is deliberately not used as a gate: some monitors that do support
  brightness fail it (BenQ ZOWIE XL), and Microsoft documents that
  others report capabilities they do not have.
- Values are normalised to a 0-100 percentage for the UI regardless of
  the raw range, so the existing `MonitorBrightness` type and the
  quick-settings / flyout widgets work unchanged.
- DDC/CI has no change events, so known monitors are polled every 5 s
  to pick up changes made from the monitor OSD; `MonitorManager` events
  trigger a rescan on hotplug. All DDC/CI transactions are serialised
  behind one lock.
- `PHYSICAL_MONITOR` handles are now released with
  `DestroyPhysicalMonitor` (they were leaked before).
- The WMI path is unchanged; `set_brightness` dispatches by instance
  name.

Tested on Windows 11 with two BenQ ZOWIE XL over DisplayPort: both
sliders drive the panels (verified reading back over dxva2 from another
process), and an external change is reflected in the UI on the next
poll. Closes eythaann#1826.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01StkifHpSLWH4J5TvuP56ur
@z1pp090 @claude
fix(brightness): publish DDC/CI reads under the lock, keep every panel
fb7f77f 
Addresses the two review findings on eythaann#1832:

- A poll or scan could read a value, release `DDCCI_LOCK`, lose the
  race against a slider write, and then publish the stale value on top
  of the fresh one. `DDCCI_LOCK` now covers both the device transaction
  and the update of the shared state, and the write path re-resolves
  the panel under the lock so a rescan cannot swap its handle
  mid-flight.
- A cloned/mirrored display group shares one `HMONITOR` but yields
  several `PHYSICAL_MONITOR` handles; only the first was kept. Each
  panel is now its own entry (`<stable id>` for the first, `<stable
  id>#<index>` for the rest) with its own handle, which is held open
  for the lifetime of the entry instead of being reopened per call.

Re-tested on two BenQ ZOWIE XL: both sliders drive their panel, an
external change is picked up on the next poll, values read back over
dxva2 match.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01StkifHpSLWH4J5TvuP56ur
@z1pp090
z1pp090 force-pushed the feat/ddcci-brightness branch from 01340b7 to fb7f77f Compare September 23, 2026 11:38
@z1pp090

z1pp090 commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (1141b50), conflicts fixed. The only conflict was in modules/monitors/brightness/infrastructure.rs: the two commands now live in the new impl crate::tauri_handlers::Handlers block, with the same shorter bodies as before (get_all_brightness() already returns MonitorBrightness). No other changes. fmt, clippy (-D warnings) and cargo test pass locally.

@eythaann
eythaann merged commit 19a25d1 into eythaann:master Sep 23, 2026
4 checks passed
@z1pp090
z1pp090 deleted the feat/ddcci-brightness branch September 24, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments
@eythaann eythaann eythaann approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEAT] Brightness control for external monitors via DDC/CI

2 participants

@z1pp090 @eythaann