From cc76c6d3b4c106f954a24bf106f63c55a0c2dff9 Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Thu, 20 Aug 2026 23:33:18 +0800 Subject: [PATCH 01/10] docs: design non-blocking external brightness control --- ...026-08-20-ddc-brightness-latency-design.md | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-20-ddc-brightness-latency-design.md diff --git a/docs/superpowers/specs/2026-08-20-ddc-brightness-latency-design.md b/docs/superpowers/specs/2026-08-20-ddc-brightness-latency-design.md new file mode 100644 index 0000000..cff0003 --- /dev/null +++ b/docs/superpowers/specs/2026-08-20-ddc-brightness-latency-design.md @@ -0,0 +1,133 @@ +# Non-blocking external brightness control + +Status: approved design, pre-implementation. +Date: 2026-08-20 + +## Purpose + +External brightness changes must react immediately even when another monitor's +DDC/CI channel is slow or unavailable. A failed monitor must not delay a healthy +monitor, and two identical monitors with no numeric serial must still map to the +correct hardware channels. + +## Root cause and measured reproduction + +The affected setup has two AOC U32N10 displays. Both report vendor 1507, +product 12816, and numeric serial 0. Their stable CoreDisplay locations are +different (`dispext0` and `dispext1`). + +`DDCService.buildAVServiceMapByProximity()` performs a synchronous +`IOAVServiceReadI2C` before accepting each external channel. On this hardware: + +- `dispext0`: the liveness read fails after 12.131 seconds. +- `dispext1`: the same read succeeds in 0.010 seconds. + +All monitors currently share one `ddcQueue`, so the slow call stalls reads and +writes for both displays. The delay is intermittent because it appears only +when an adjustment lands behind a probe or retry. The slow channel also makes +actual DDC calls block, so deleting only the liveness read is insufficient. + +## Design + +### Stable channel matching + +Extend `DDCServiceMatcher.Identity` with an optional IORegistry location. +CoreDisplay's `IODisplayLocation` is the target-side value; the path of the +framebuffer preceding each `DCPAVServiceProxy` is the service-side value. + +Matching order becomes: + +1. exact non-empty location; +2. vendor + product + non-zero serial; +3. vendor + product; +4. existing traversal-order fallback. + +The location match distinguishes the two U32N10 panels even though their +vendor, product, numeric serial, EDID UUID, and product name are identical. +Systems where CoreDisplay does not expose a location retain today's behavior. + +Enumerate every external `DCPAVServiceProxy` without issuing the synchronous +liveness read. Real VCP operations remain the authority on whether a channel +works; an unsolicited read is neither a reliable capability test nor safe for +latency. + +### Per-display I/O isolation + +Replace the single DDC operation queue with one serial queue per +`CGDirectDisplayID`. Operations for one display remain ordered, while a blocked +driver call cannot hold another display's writes. Cache and quarantine state +that was implicitly protected by the global queue receives explicit locking. +Display removal drops its queue and state through the existing cache cleanup. + +### Responsive unknown-DDC path + +Known-good DDC and known-software displays keep their current paths. When DDC +availability is still unknown: + +1. apply the requested level immediately with the existing software gamma path; +2. enqueue the coalesced hardware DDC write on that display's queue; +3. on success, mark DDC available and remove the temporary gamma preview; +4. on final failure, mark DDC unavailable and retain software brightness. + +`DDCService.writeAsync` already performs its bounded retries, so one final +failure is enough to choose software for the session; the outer three-failure +layer is removed. Reconnect invalidation permits a recovered or recabled monitor +to be probed again. + +The DDC completion only removes the preview when no newer target is pending. +This prevents a late completion for an old slider value from overwriting the +newest preview. Below the existing 15% blend threshold, success restores the +normal DDC-plus-gamma blend rather than an identity gamma table. + +## Scope + +Expected production changes: + +- `Crisp/Models/DDCServiceMatcher.swift` +- `Crisp/Services/DDCService.swift` +- `Crisp/Services/BrightnessService.swift` +- focused matcher and per-display queue isolation tests + +No UI, localization, new dependency, or user preference is added. Built-in +brightness is unchanged; its measured `DisplayServicesSetBrightness` cost is +0.009 ms average (120 same-value calls), so it is not part of this defect. + +## Error handling + +- Missing CoreDisplay or location: use identity and traversal fallback. +- DDC read/write blocks: only that display's hardware queue waits; the visible + adjustment has already occurred through software gamma. +- DDC write eventually succeeds: switch to hardware without applying a stale + completion over a newer target. +- DDC write fails after its existing retries: stay in software mode until the + display reconnects. + +## Testing and verification + +- Unit test: location wins for two otherwise-identical displays whose CoreGraphics + and IORegistry orders differ. +- Unit test: missing locations preserve existing exact/model/fallback behavior. +- Unit test: a blocked operation for display A does not delay display B, while + operations for one display remain serial. +- Run `make check`. +- Hardware verification on the dual-U32N10 setup: + - repeatedly drag both sliders immediately after launch and panel open; + - confirm the healthy display is never delayed by the slow channel; + - confirm the slow display changes immediately through software fallback; + - confirm each slider controls its own physical panel; + - rapidly retarget a slider while its DDC write is pending and confirm a late + completion never restores an older level; + - reconnect both displays and repeat to exercise cache invalidation. + +Success means no visible brightness change waits for the measured 12-second I/O +timeout, while known-good monitors continue using hardware DDC. + +## Rejected alternatives + +- Only split the queue: fixes the healthy display but leaves the slow display + unresponsive. +- Only remove the liveness read: later reads or writes can still block. +- Add a manual "Force software" setting: exposes an implementation failure to + users and does not fix cross-display blocking. +- Run concurrent reads and writes against one channel: risks corrupting fragile + monitor DDC controllers; per-display serialization remains mandatory. From 4f75b3c5f6e607b36f51f7504d080eea51c73637 Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 00:34:45 +0800 Subject: [PATCH 02/10] docs: plan non-blocking external brightness fix --- .../2026-08-21-ddc-brightness-latency.md | 602 ++++++++++++++++++ 1 file changed, 602 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-21-ddc-brightness-latency.md diff --git a/docs/superpowers/plans/2026-08-21-ddc-brightness-latency.md b/docs/superpowers/plans/2026-08-21-ddc-brightness-latency.md new file mode 100644 index 0000000..f5479d7 --- /dev/null +++ b/docs/superpowers/plans/2026-08-21-ddc-brightness-latency.md @@ -0,0 +1,602 @@ +# Non-blocking External Brightness Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make external brightness react immediately when one DDC/CI channel blocks, without letting that channel delay another display or mis-pairing identical monitors. + +**Architecture:** Match Apple Silicon DDC channels by the existing CoreDisplay/IORegistry location before numeric identity, enumerate channels without a blocking liveness read, and route all DDC work through one serial queue per display. While DDC availability is unknown, use the existing gamma path as an immediate preview; the final retried DDC result either replaces that preview with the normal hardware/gamma blend or keeps software brightness for the session. + +**Tech Stack:** Swift 5/6, CoreGraphics, IOKit/CoreDisplay runtime lookup, Grand Central Dispatch, XCTest, XcodeGen. + +**Starting point:** Branch `fix/ddc-brightness-latency`, design commit `cc76c6d`, and approved specification `docs/superpowers/specs/2026-08-20-ddc-brightness-latency-design.md`. Run every command from the repository root. + +--- + +## File map + +- Modify `Crisp/Models/DDCServiceMatcher.swift`: add optional display location and make it the highest-priority match. +- Create `Crisp/Models/DDCOperationQueuePool.swift`: own the minimum locked map of per-display serial queues. +- Modify `Crisp/Services/DDCService.swift`: collect both sides' locations, remove the liveness read, use the queue pool, and lock read-quarantine state explicitly. +- Modify `Crisp/Services/BrightnessService.swift`: preview unknown DDC changes in gamma and make one final DDC failure authoritative. +- Modify `CrispTests/DDCServiceMatcherTests.swift`: pin location-first and location-missing behavior. +- Create `CrispTests/DDCOperationQueuePoolTests.swift`: pin cross-display isolation and same-display serialization. +- Modify `project.yml`: compile the queue pool into the headless test target. + +### Task 1: Match identical displays by stable location + +**Files:** +- Modify: `CrispTests/DDCServiceMatcherTests.swift` +- Modify: `Crisp/Models/DDCServiceMatcher.swift` + +- [ ] **Step 1: Add failing location tests** + +Insert these tests before `// MARK: - Strategy 2` in `CrispTests/DDCServiceMatcherTests.swift`: + +```swift + func testLocationWinsWhenIdenticalDisplayOrderIsReversed() { + let serviceA = DDCServiceMatcher.Identity( + vendor: 1507, product: 12816, serial: 0, location: "IOService:/dispext0@B0000000" + ) + let serviceB = DDCServiceMatcher.Identity( + vendor: 1507, product: 12816, serial: 0, location: "IOService:/dispext1@90000000" + ) + let result = DDCServiceMatcher.match( + services: [serviceA, serviceB], + displays: [ + (id: 2, identity: serviceB), + (id: 5, identity: serviceA) + ] + ) + + XCTAssertEqual(result.byDisplayID, [2: 1, 5: 0]) + XCTAssertFalse(result.ambiguous) + } + + func testMissingLocationsPreserveModelFallback() { + let service = DDCServiceMatcher.Identity(vendor: 1507, product: 12816, serial: 0) + let result = DDCServiceMatcher.match( + services: [service], + displays: [ + (id: 5, identity: .init(vendor: 1507, product: 12816, serial: 0)), + (id: 2, identity: .init(vendor: 1, product: 2, serial: 0)) + ] + ) + + XCTAssertEqual(result.byDisplayID, [5: 0]) + XCTAssertFalse(result.ambiguous) + } +``` + +- [ ] **Step 2: Run the matcher tests and verify the new initializer fails** + +Run: + +```bash +make test +``` + +Expected: FAIL compiling `DDCServiceMatcherTests` because `Identity` has no `location` parameter. + +- [ ] **Step 3: Add location to `Identity` and prioritize it** + +Replace `Identity` in `Crisp/Models/DDCServiceMatcher.swift` with: + +```swift + struct Identity: Equatable { + let vendor: UInt32 + let product: UInt32 + let serial: UInt32 + let location: String? + + init(vendor: UInt32, product: UInt32, serial: UInt32, location: String? = nil) { + self.vendor = vendor + self.product = product + self.serial = serial + self.location = location + } + } +``` + +Replace the three declarations starting at `let exact = displays.first` with: + +```swift + let byLocation = idty.location.flatMap { location in + guard !location.isEmpty else { return nil } + return displays.first { + !usedDisplays.contains($0.id) && $0.identity.location == location + } + } + let exact = byLocation ?? displays.first { + !usedDisplays.contains($0.id) + && idty.serial != 0 + && $0.identity.serial != 0 + && $0.identity.vendor == idty.vendor + && $0.identity.product == idty.product + && $0.identity.serial == idty.serial + } + let byModel = exact ?? displays.first { + !usedDisplays.contains($0.id) + && $0.identity.vendor == idty.vendor + && $0.identity.product == idty.product + } +``` + +Update the matcher comments to state the order: exact non-empty location, vendor/product/non-zero serial, vendor/product, traversal fallback. + +- [ ] **Step 4: Run the tests** + +Run: `make test` + +Expected: PASS, including `testLocationWinsWhenIdenticalDisplayOrderIsReversed` and all existing matcher behavior. + +- [ ] **Step 5: Commit the matcher change** + +```bash +git add Crisp/Models/DDCServiceMatcher.swift CrispTests/DDCServiceMatcherTests.swift +git commit -m "fix: match DDC channels by display location" +``` + +--- + +### Task 2: Prove per-display queue isolation + +**Files:** +- Create: `CrispTests/DDCOperationQueuePoolTests.swift` +- Create: `Crisp/Models/DDCOperationQueuePool.swift` +- Modify: `project.yml` + +- [ ] **Step 1: Write the failing queue tests** + +Create `CrispTests/DDCOperationQueuePoolTests.swift`: + +```swift +import XCTest + +final class DDCOperationQueuePoolTests: XCTestCase { + func testBlockedDisplayDoesNotDelayAnotherDisplay() { + let pool = DDCOperationQueuePool() + let blockedStarted = DispatchSemaphore(value: 0) + let releaseBlocked = DispatchSemaphore(value: 0) + let otherFinished = DispatchSemaphore(value: 0) + + pool.queue(for: 2).async { + blockedStarted.signal() + releaseBlocked.wait() + } + XCTAssertEqual(blockedStarted.wait(timeout: .now() + 1), .success) + + pool.queue(for: 5).async { otherFinished.signal() } + XCTAssertEqual(otherFinished.wait(timeout: .now() + 0.5), .success) + releaseBlocked.signal() + } + + func testOperationsForOneDisplayRemainSerial() { + let pool = DDCOperationQueuePool() + let firstStarted = DispatchSemaphore(value: 0) + let releaseFirst = DispatchSemaphore(value: 0) + let secondStarted = DispatchSemaphore(value: 0) + + pool.queue(for: 2).async { + firstStarted.signal() + releaseFirst.wait() + } + XCTAssertEqual(firstStarted.wait(timeout: .now() + 1), .success) + + pool.queue(for: 2).async { secondStarted.signal() } + XCTAssertEqual(secondStarted.wait(timeout: .now() + 0.1), .timedOut) + releaseFirst.signal() + XCTAssertEqual(secondStarted.wait(timeout: .now() + 1), .success) + } +} +``` + +- [ ] **Step 2: Run the tests and verify the missing type fails** + +Run: `make test` + +Expected: FAIL compiling `DDCOperationQueuePoolTests` with `cannot find 'DDCOperationQueuePool' in scope`. + +- [ ] **Step 3: Add the minimum locked queue pool** + +Create `Crisp/Models/DDCOperationQueuePool.swift`: + +```swift +import Foundation +import CoreGraphics + +/// Keeps DDC operations serial per physical display without coupling displays. +final class DDCOperationQueuePool: @unchecked Sendable { + private let lock = NSLock() + private var queues: [CGDirectDisplayID: DispatchQueue] = [:] + + func queue(for displayID: CGDirectDisplayID) -> DispatchQueue { + lock.withLock { + if let queue = queues[displayID] { return queue } + let queue = DispatchQueue( + label: "com.crisp.ddc.\(displayID)", + qos: .userInitiated + ) + queues[displayID] = queue + return queue + } + } + + func removeQueue(for displayID: CGDirectDisplayID) { + lock.withLock { _ = queues.removeValue(forKey: displayID) } + } +} +``` + +Add this source after `DDCServiceMatcher.swift` in the `CrispTests` source list in `project.yml`: + +```yaml + - path: Crisp/Models/DDCOperationQueuePool.swift +``` + +- [ ] **Step 4: Run the tests** + +Run: `make test` + +Expected: PASS; display 5 finishes while display 2 is blocked, and display 2's second operation waits for its first. + +- [ ] **Step 5: Commit the queue primitive** + +```bash +git add project.yml Crisp/Models/DDCOperationQueuePool.swift CrispTests/DDCOperationQueuePoolTests.swift +git commit -m "fix: isolate DDC work by display queue" +``` + +--- + +### Task 3: Remove blocking discovery and wire isolated queues + +**Files:** +- Modify: `Crisp/Services/DDCService.swift` + +- [ ] **Step 1: Load CoreDisplay's display dictionary through the existing runtime-lookup pattern** + +Add this file-private symbol after `CGDisplayIOServicePort`: + +```swift +private let _CoreDisplayCreateInfoDictionary: + (@convention(c) (CGDirectDisplayID) -> Unmanaged?)? = { + guard let handle = dlopen( + "/System/Library/Frameworks/CoreDisplay.framework/CoreDisplay", + RTLD_LAZY + ), let symbol = dlsym(handle, "CoreDisplay_DisplayCreateInfoDictionary") else { + return nil + } + return unsafeBitCast( + symbol, + to: (@convention(c) (CGDirectDisplayID) -> Unmanaged?).self + ) + }() +``` + +Add these helpers beside `displayIdentity(from:)`: + +```swift + private func coreDisplayLocation(for displayID: CGDirectDisplayID) -> String? { + guard let dictionary = _CoreDisplayCreateInfoDictionary?(displayID)?.takeRetainedValue() + as NSDictionary? else { return nil } + return dictionary[kIODisplayLocationKey] as? String + } + + private func ioRegistryPath(for entry: io_service_t) -> String? { + let path = UnsafeMutablePointer.allocate(capacity: 1024) + defer { path.deallocate() } + guard IORegistryEntryGetPath(entry, kIOServicePlane, path) == KERN_SUCCESS else { + return nil + } + return String(cString: path) + } +``` + +Change `displayIdentity` to accept and store the optional location: + +```swift + private func displayIdentity( + from productAttributes: [String: Any], + location: String? = nil + ) -> DDCServiceMatcher.Identity? { + func u32(_ value: Any?) -> UInt32? { + if let v = value as? UInt32 { return v } + if let v = value as? Int { return UInt32(bitPattern: Int32(truncatingIfNeeded: v)) } + if let v = value as? NSNumber { return v.uint32Value } + return nil + } + guard let vendor = u32(productAttributes["LegacyManufacturerID"]), + let product = u32(productAttributes["ProductID"]) else { return nil } + return DDCServiceMatcher.Identity( + vendor: vendor, + product: product, + serial: u32(productAttributes["SerialNumber"]) ?? 0, + location: location + ) + } +``` + +- [ ] **Step 2: Collect framebuffer paths and enumerate channels without I2C** + +In the framebuffer branch of `buildAVServiceMapByProximity()`, pass the registry path: + +```swift + let pa = da["ProductAttributes"] as? [String: Any], + let id = displayIdentity(from: pa, location: ioRegistryPath(for: entry)) { + lastIdentity = id + } +``` + +Replace the liveness-read block for `DCPAVServiceProxy` with: + +```swift + if location == nil || location == "External", + let avService = IOAVServiceCreateWithService(kCFAllocatorDefault, entry) { + ordered.append(avService) + identities.append(lastIdentity) + } +``` + +When building `displays`, add the CoreDisplay location: + +```swift + let displays: [(id: CGDirectDisplayID, identity: DDCServiceMatcher.Identity)] = externalIDs.map { + (id: $0, identity: DDCServiceMatcher.Identity( + vendor: CGDisplayVendorNumber($0), + product: CGDisplayModelNumber($0), + serial: CGDisplaySerialNumber($0), + location: coreDisplayLocation(for: $0))) + } +``` + +Update comments that say “working AVServices” or “answers I2C” to “external AVServices”; real VCP operations now determine capability. + +- [ ] **Step 3: Replace the global queue and protect quarantine dictionaries** + +Replace `ddcQueue` with: + +```swift + private let operationQueues = DDCOperationQueuePool() +``` + +Add beside `readFailStreak`: + +```swift + private let readStateLock = NSLock() +``` + +Replace `readSynchronous` with: + +```swift + private func readSynchronous( + displayID: CGDirectDisplayID, + command: UInt8 + ) -> (current: UInt16, max: UInt16)? { + let quarantined = readStateLock.withLock { () -> Bool in + guard let until = readQuarantineUntil[displayID] else { return false } + guard Date() >= until else { return true } + readQuarantineUntil.removeValue(forKey: displayID) + readFailStreak[displayID] = 0 + return false + } + guard !quarantined else { return nil } + +#if arch(arm64) + let result = arm64Read(displayID: displayID, command: command) +#else + let result = intelReadSynchronous(displayID: displayID, command: command) +#endif + + readStateLock.withLock { + if result == nil { + let streak = readFailStreak[displayID, default: 0] + 1 + readFailStreak[displayID] = streak + if streak >= readQuarantineThreshold { + readQuarantineUntil[displayID] = Date().addingTimeInterval(readQuarantineInterval) + } + } else { + readFailStreak[displayID] = 0 + } + } + return result + } +``` + +Replace every `ddcQueue.async` in `writeAsync`, `readAsync`, and `readBatchVCPCodes` with: + +```swift +operationQueues.queue(for: displayID).async { +``` + +Keep each existing closure body and retry loop unchanged. + +Replace the read-state cleanup in `clearCache(for:)` with: + +```swift + readStateLock.withLock { + readFailStreak.removeValue(forKey: displayID) + readQuarantineUntil.removeValue(forKey: displayID) + } + operationQueues.removeQueue(for: displayID) +``` + +Replace the asynchronous read-state cleanup in `invalidateAllChannelMappings()` with: + +```swift + readStateLock.withLock { + readFailStreak.removeAll() + readQuarantineUntil.removeAll() + } +``` + +Do not remove all operation queues during a global channel-map invalidation: online display IDs keep the same queue, so an in-flight operation cannot overlap a newly-created queue. Removed displays still drop their queue through `clearCache(for:)`. + +- [ ] **Step 4: Compile and run unit tests** + +Run: + +```bash +make compile +make test +``` + +Expected: `Crisp-bin` builds and all tests pass without the removed `ddcQueue` symbol. + +- [ ] **Step 5: Commit DDC service wiring** + +```bash +git add Crisp/Services/DDCService.swift +git commit -m "fix: prevent one DDC channel blocking other displays" +``` + +--- + +### Task 4: Make unknown and failed DDC writes visibly immediate + +**Files:** +- Modify: `Crisp/Services/BrightnessService.swift` + +- [ ] **Step 1: Remove the redundant outer failure counter** + +Delete: + +```swift + private var ddcFailStreak: [CGDirectDisplayID: Int] = [:] +``` + +Also delete `ddcFailStreak.removeValue(forKey: displayID)` from `invalidateDDCState(for:)`. `DDCService.writeAsync` retains its existing three low-level attempts. + +- [ ] **Step 2: Preview unknown DDC brightness and preserve the established low-end blend** + +In `writeDDCBrightnessCoalesced`, replace the gamma-update block beginning with `queue.async` with: + +```swift + let ddcStatus = ddcAvailableLock.withLock { ddcAvailable[displayID] } + queue.async { [weak self] in + guard let self else { return } + if ddcStatus == nil { + self.setSoftwareBrightness(percent, for: displayID) + } else if ddcStatus == true { + if percent < self.gammaBlendThreshold { + self.setSoftwareBrightness( + percent / self.gammaBlendThreshold * 100.0, + for: displayID + ) + } else if let factor = self.currentSoftwareBrightness(for: displayID), factor < 1.0 { + self.setSoftwareBrightness(100.0, for: displayID) + } + } + } +``` + +This keeps known-good behavior unchanged and gives the unknown path an immediate software result before any DDC read/write can finish. + +- [ ] **Step 3: Make the final retried write result authoritative** + +Replace the body of the `writeAsync` completion in `pumpDDCWrite(for:)` with: + +```swift + guard let self else { return } + if success { + self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = true } + let hasNewerTarget = self.ddcPumpLock.withLock { + self.pendingDDCPercent[displayID] != nil + } + if !hasNewerTarget { + self.queue.async { + let stillCurrent = self.ddcPumpLock.withLock { + self.pendingDDCPercent[displayID] == nil + } + guard stillCurrent else { return } + let softwarePercent = percent < self.gammaBlendThreshold + ? percent / self.gammaBlendThreshold * 100.0 + : 100.0 + self.setSoftwareBrightness(softwarePercent, for: displayID) + } + } + self.pumpDDCWrite(for: displayID) + } else { + self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = false } + let fallbackPercent = self.ddcPumpLock.withLock { () -> Double in + let latest = self.pendingDDCPercent.removeValue(forKey: displayID) ?? percent + self.ddcPumpActive.remove(displayID) + return latest + } + self.queue.async { + self.setSoftwareBrightness(fallbackPercent, for: displayID) + } + } +``` + +Success clears an unknown preview only if it is still the latest target. A final failure stops the pump, applies the newest pending value in software, and makes future calls use the existing `currentStatus == false` branch. + +- [ ] **Step 4: Compile and run the full local checks** + +Run: + +```bash +make check +``` + +Expected: lint is silent, all tests pass, localization export passes, and the command ends with `check passed: lint clean, tests green, localization keys complete`. + +- [ ] **Step 5: Commit responsive fallback behavior** + +```bash +git add Crisp/Services/BrightnessService.swift +git commit -m "fix: preview slow DDC brightness changes in software" +``` + +--- + +### Task 5: Hardware verification and PR-ready diff + +**Files:** +- No source edits expected. + +- [ ] **Step 1: Create a disposable test app and launch the branch build** + +Run: + +```bash +ditto /Applications/Crisp.app /tmp/Crisp-DDC-Test.app +CRISP_APP=/tmp/Crisp-DDC-Test.app make dev +``` + +Expected: `Crisp 1.5.0 running` from `/tmp/Crisp-DDC-Test.app`; the signed release in `/Applications` remains unchanged. + +- [ ] **Step 2: Verify the two U32N10 displays** + +With both AOC U32N10 displays connected: + +1. Open Crisp and immediately drag each external slider repeatedly. +2. Confirm the slow `dispext0` panel changes immediately through gamma instead of waiting roughly 12 seconds. +3. While `dispext0` is probing, drag `dispext1` and confirm it changes independently. +4. Confirm each slider controls its own physical panel. +5. Rapidly move one slider to several values and stop; confirm no late DDC completion restores an older level. +6. Disconnect and reconnect both displays, then repeat steps 1–5 to exercise state invalidation. + +Expected: neither visible change waits for DDC, the healthy channel is not blocked, and no stale completion or swapped mapping is visible. + +- [ ] **Step 3: Restore the release app after the hardware run** + +Run: + +```bash +pkill -x Crisp || true +open /Applications/Crisp.app +``` + +Expected: the installed notarized v1.5.0 app is running again. + +- [ ] **Step 4: Verify the final branch** + +Run: + +```bash +git status --short --branch +git diff --check origin/main...HEAD +git log --oneline origin/main..HEAD +``` + +Expected: clean `fix/ddc-brightness-latency` branch, no whitespace errors, and only the design/plan plus the focused matcher, queue, DDC service, brightness service, test, and `project.yml` commits. From 53e2236bb4182a8cda2855139afa9f1dd90ca0ac Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 09:55:28 +0800 Subject: [PATCH 03/10] chore: ignore local worktrees --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index fd73a5a..a529f27 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,7 @@ DerivedData/ Crisp.xcodeproj/ Crisp.dmg .superpowers/ +.worktrees/ Crisp-bin # Working/internal docs kept local-only (research, release-note drafts) From db5379ce1c019bd6c22f542c680a47cb210db809 Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 09:57:39 +0800 Subject: [PATCH 04/10] fix: match DDC channels by display location --- Crisp/Models/DDCServiceMatcher.swift | 21 +++++++++++++--- CrispTests/DDCServiceMatcherTests.swift | 33 +++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/Crisp/Models/DDCServiceMatcher.swift b/Crisp/Models/DDCServiceMatcher.swift index 52596ef..b600d1a 100644 --- a/Crisp/Models/DDCServiceMatcher.swift +++ b/Crisp/Models/DDCServiceMatcher.swift @@ -20,11 +20,20 @@ enum DDCServiceMatcher { /// A display's vendor/product/serial identity mirrors the IORegistry /// `ProductAttributes` (`LegacyManufacturerID` / `ProductID` / `SerialNumber`) /// which line up with `CGDisplayVendorNumber` / `CGDisplayModelNumber` / - /// `CGDisplaySerialNumber` for the same physical display. + /// `CGDisplaySerialNumber` for the same physical display. Location is the + /// stable CoreDisplay/IORegistry path when macOS exposes it. struct Identity: Equatable { let vendor: UInt32 let product: UInt32 let serial: UInt32 + let location: String? + + init(vendor: UInt32, product: UInt32, serial: UInt32, location: String? = nil) { + self.vendor = vendor + self.product = product + self.serial = serial + self.location = location + } } /// The outcome of a matching pass. @@ -53,11 +62,17 @@ enum DDCServiceMatcher { var usedDisplays = Set() var unmatched: [Int] = [] - // Strategy 1: identity matching (vendor+product+serial, then vendor+product). + // Strategy 1: stable location, then non-zero serial, then model identity. for i in services.indices { guard let idty = services[i] else { unmatched.append(i); continue } - let exact = displays.first { + let byLocation = displays.first { + guard let location = idty.location, !location.isEmpty else { return false } + return !usedDisplays.contains($0.id) && $0.identity.location == location + } + let exact = byLocation ?? displays.first { !usedDisplays.contains($0.id) + && idty.serial != 0 + && $0.identity.serial != 0 && $0.identity.vendor == idty.vendor && $0.identity.product == idty.product && $0.identity.serial == idty.serial diff --git a/CrispTests/DDCServiceMatcherTests.swift b/CrispTests/DDCServiceMatcherTests.swift index 7090075..0478e05 100644 --- a/CrispTests/DDCServiceMatcherTests.swift +++ b/CrispTests/DDCServiceMatcherTests.swift @@ -124,6 +124,39 @@ final class DDCServiceMatcherTests: XCTestCase { XCTAssertFalse(result.ambiguous) } + func testLocationWinsWhenIdenticalDisplayOrderIsReversed() { + let serviceA = DDCServiceMatcher.Identity( + vendor: 1507, product: 12816, serial: 0, location: "IOService:/dispext0@B0000000" + ) + let serviceB = DDCServiceMatcher.Identity( + vendor: 1507, product: 12816, serial: 0, location: "IOService:/dispext1@90000000" + ) + let result = DDCServiceMatcher.match( + services: [serviceA, serviceB], + displays: [ + (id: 2, identity: serviceB), + (id: 5, identity: serviceA) + ] + ) + + XCTAssertEqual(result.byDisplayID, [2: 1, 5: 0]) + XCTAssertFalse(result.ambiguous) + } + + func testMissingLocationsPreserveModelFallback() { + let service = DDCServiceMatcher.Identity(vendor: 1507, product: 12816, serial: 0) + let result = DDCServiceMatcher.match( + services: [service], + displays: [ + (id: 5, identity: .init(vendor: 1507, product: 12816, serial: 0)), + (id: 2, identity: .init(vendor: 1, product: 2, serial: 0)) + ] + ) + + XCTAssertEqual(result.byDisplayID, [5: 0]) + XCTAssertFalse(result.ambiguous) + } + // MARK: - Strategy 2: traversal-order fallback /// *Traversal-order fallback, two no-identity services.* leftovers are sorted From 556367f286bb96f4a0e775b9611394661ab8a798 Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 09:58:36 +0800 Subject: [PATCH 05/10] fix: isolate DDC work by display queue --- Crisp/Models/DDCOperationQueuePool.swift | 24 +++++++++++++ CrispTests/DDCOperationQueuePoolTests.swift | 38 +++++++++++++++++++++ project.yml | 1 + 3 files changed, 63 insertions(+) create mode 100644 Crisp/Models/DDCOperationQueuePool.swift create mode 100644 CrispTests/DDCOperationQueuePoolTests.swift diff --git a/Crisp/Models/DDCOperationQueuePool.swift b/Crisp/Models/DDCOperationQueuePool.swift new file mode 100644 index 0000000..917ec03 --- /dev/null +++ b/Crisp/Models/DDCOperationQueuePool.swift @@ -0,0 +1,24 @@ +import Foundation +import CoreGraphics + +/// Keeps DDC operations serial per physical display without coupling displays. +final class DDCOperationQueuePool: @unchecked Sendable { + private let lock = NSLock() + private var queues: [CGDirectDisplayID: DispatchQueue] = [:] + + func queue(for displayID: CGDirectDisplayID) -> DispatchQueue { + lock.withLock { + if let queue = queues[displayID] { return queue } + let queue = DispatchQueue( + label: "com.crisp.ddc.\(displayID)", + qos: .userInitiated + ) + queues[displayID] = queue + return queue + } + } + + func removeQueue(for displayID: CGDirectDisplayID) { + lock.withLock { _ = queues.removeValue(forKey: displayID) } + } +} diff --git a/CrispTests/DDCOperationQueuePoolTests.swift b/CrispTests/DDCOperationQueuePoolTests.swift new file mode 100644 index 0000000..874aedf --- /dev/null +++ b/CrispTests/DDCOperationQueuePoolTests.swift @@ -0,0 +1,38 @@ +import XCTest + +final class DDCOperationQueuePoolTests: XCTestCase { + func testBlockedDisplayDoesNotDelayAnotherDisplay() { + let pool = DDCOperationQueuePool() + let blockedStarted = DispatchSemaphore(value: 0) + let releaseBlocked = DispatchSemaphore(value: 0) + let otherFinished = DispatchSemaphore(value: 0) + + pool.queue(for: 2).async { + blockedStarted.signal() + releaseBlocked.wait() + } + XCTAssertEqual(blockedStarted.wait(timeout: .now() + 1), .success) + + pool.queue(for: 5).async { otherFinished.signal() } + XCTAssertEqual(otherFinished.wait(timeout: .now() + 0.5), .success) + releaseBlocked.signal() + } + + func testOperationsForOneDisplayRemainSerial() { + let pool = DDCOperationQueuePool() + let firstStarted = DispatchSemaphore(value: 0) + let releaseFirst = DispatchSemaphore(value: 0) + let secondStarted = DispatchSemaphore(value: 0) + + pool.queue(for: 2).async { + firstStarted.signal() + releaseFirst.wait() + } + XCTAssertEqual(firstStarted.wait(timeout: .now() + 1), .success) + + pool.queue(for: 2).async { secondStarted.signal() } + XCTAssertEqual(secondStarted.wait(timeout: .now() + 0.1), .timedOut) + releaseFirst.signal() + XCTAssertEqual(secondStarted.wait(timeout: .now() + 1), .success) + } +} diff --git a/project.yml b/project.yml index 7c6dc7d..889f21f 100644 --- a/project.yml +++ b/project.yml @@ -57,6 +57,7 @@ targets: - path: CrispTests - path: Crisp/Models/DisplayModeGeometry.swift - path: Crisp/Models/DDCServiceMatcher.swift + - path: Crisp/Models/DDCOperationQueuePool.swift - path: Crisp/Models/RefreshRateFormat.swift - path: Crisp/Models/GammaPersistenceKey.swift - path: Crisp/Models/VariableRefreshModes.swift From 22cc82e2937921beb744f7f2dfc379315468bac8 Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 10:01:37 +0800 Subject: [PATCH 06/10] fix: prevent one DDC channel blocking other displays --- Crisp/Services/DDCService.swift | 127 ++++++++++++++++++++++---------- 1 file changed, 87 insertions(+), 40 deletions(-) diff --git a/Crisp/Services/DDCService.swift b/Crisp/Services/DDCService.swift index fe94cc9..21458a3 100644 --- a/Crisp/Services/DDCService.swift +++ b/Crisp/Services/DDCService.swift @@ -8,6 +8,20 @@ import IOKit.graphics @_silgen_name("CGDisplayIOServicePort") private func CGDisplayIOServicePort(_ display: CGDirectDisplayID) -> io_service_t +private let coreDisplayCreateInfoDictionary: + (@convention(c) (CGDirectDisplayID) -> Unmanaged?)? = { + guard let handle = dlopen( + "/System/Library/Frameworks/CoreDisplay.framework/CoreDisplay", + RTLD_LAZY + ), let symbol = dlsym(handle, "CoreDisplay_DisplayCreateInfoDictionary") else { + return nil + } + return unsafeBitCast( + symbol, + to: (@convention(c) (CGDirectDisplayID) -> Unmanaged?).self + ) + }() + /// DDC/CI I2C communication service for external displays. /// Supports two hardware paths: /// - ARM64 (Apple Silicon): IOAVService via DCPAVServiceProxy @@ -22,7 +36,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { static let volumeVCP: UInt8 = 0x62 static let powerVCP: UInt8 = 0xD6 - private let ddcQueue = DispatchQueue(label: "com.crisp.ddc", qos: .userInitiated) + private let operationQueues = DDCOperationQueuePool() // MARK: - VCP Read Cache (5-second TTL) @@ -41,7 +55,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { #if arch(arm64) private var avServiceCache: [CGDirectDisplayID: IOAVServiceRef] = [:] private let avServiceLock = NSLock() - /// Ordered list of all working external AVServices found during last enumeration. + /// Ordered list of all external AVServices found during last enumeration. private var allExternalAVServices: [IOAVServiceRef] = [] #endif @@ -69,13 +83,14 @@ final class DDCService: ObservableObject, @unchecked Sendable { /// same proximity strategy MonitorControl uses. /// /// Matching order: - /// 1. Identity: vendor+product+serial, then vendor+product, against CG displays. - /// 2. Traversal-order fallback for anything identity matching missed (e.g. two + /// 1. Stable CoreDisplay/IORegistry location. + /// 2. Identity: vendor+product+non-zero serial, then vendor+product. + /// 3. Traversal-order fallback for anything identity matching missed (e.g. two /// identical monitors that share vendor/product/serial). This preserves correct /// pairing far better than the old sorted-index because the AVService order /// follows the framebuffer order within the same subtree. /// - /// Returns the map plus the working AVServices in traversal order. + /// Returns the map plus the external AVServices in traversal order. private func buildAVServiceMapByProximity() -> (map: [CGDirectDisplayID: IOAVServiceRef], ordered: [IOAVServiceRef]) { // External CG displays we need to map. var displayCount: UInt32 = 0 @@ -109,11 +124,12 @@ final class DDCService: ObservableObject, @unchecked Sendable { entry, "DisplayAttributes" as CFString, kCFAllocatorDefault, 0 )?.takeRetainedValue() as? [String: Any], let pa = da["ProductAttributes"] as? [String: Any], - let id = displayIdentity(from: pa) { + let id = displayIdentity(from: pa, location: ioRegistryPath(for: entry)) { lastIdentity = id } - // A DCPAVServiceProxy that answers I2C is a live DDC channel. + // Enumerate external channels without probing them. Some monitors block an + // unsolicited read for seconds; real VCP operations decide capability. if ioClassName(entry) == "DCPAVServiceProxy" { let location = IORegistryEntryCreateCFProperty( entry, "Location" as CFString, kCFAllocatorDefault, 0 @@ -121,11 +137,8 @@ final class DDCService: ObservableObject, @unchecked Sendable { // Some drivers omit "Location"; still attempt those. Skip explicit non-External. if location == nil || location == "External", let avService = IOAVServiceCreateWithService(kCFAllocatorDefault, entry) { - var testBuf = [UInt8](repeating: 0, count: 32) - if IOAVServiceReadI2C(avService, 0x37, 0x51, &testBuf, 32) == kIOReturnSuccess { - ordered.append(avService) - identities.append(lastIdentity) - } + ordered.append(avService) + identities.append(lastIdentity) } } @@ -142,7 +155,8 @@ final class DDCService: ObservableObject, @unchecked Sendable { (id: $0, identity: DDCServiceMatcher.Identity( vendor: CGDisplayVendorNumber($0), product: CGDisplayModelNumber($0), - serial: CGDisplaySerialNumber($0))) + serial: CGDisplaySerialNumber($0), + location: coreDisplayLocation(for: $0))) } let result = DDCServiceMatcher.match(services: identities, displays: displays) @@ -162,10 +176,28 @@ final class DDCService: ObservableObject, @unchecked Sendable { return (map, ordered) } + private func coreDisplayLocation(for displayID: CGDirectDisplayID) -> String? { + guard let dictionary = coreDisplayCreateInfoDictionary?(displayID)?.takeRetainedValue() + as NSDictionary? else { return nil } + return dictionary[kIODisplayLocationKey] as? String + } + + private func ioRegistryPath(for entry: io_service_t) -> String? { + let path = UnsafeMutablePointer.allocate(capacity: 1024) + defer { path.deallocate() } + guard IORegistryEntryGetPath(entry, kIOServicePlane, path) == KERN_SUCCESS else { + return nil + } + return String(cString: path) + } + /// Extracts vendor/product/serial from a ProductAttributes dictionary. The numeric /// LegacyManufacturerID / ProductID / SerialNumber match CGDisplayVendorNumber / /// CGDisplayModelNumber / CGDisplaySerialNumber for the same physical display. - private func displayIdentity(from productAttributes: [String: Any]) -> DDCServiceMatcher.Identity? { + private func displayIdentity( + from productAttributes: [String: Any], + location: String? = nil + ) -> DDCServiceMatcher.Identity? { func u32(_ value: Any?) -> UInt32? { if let v = value as? UInt32 { return v } if let v = value as? Int { return UInt32(bitPattern: Int32(truncatingIfNeeded: v)) } @@ -174,8 +206,12 @@ final class DDCService: ObservableObject, @unchecked Sendable { } guard let vendor = u32(productAttributes["LegacyManufacturerID"]), let product = u32(productAttributes["ProductID"]) else { return nil } - return DDCServiceMatcher.Identity(vendor: vendor, product: product, - serial: u32(productAttributes["SerialNumber"]) ?? 0) + return DDCServiceMatcher.Identity( + vendor: vendor, + product: product, + serial: u32(productAttributes["SerialNumber"]) ?? 0, + location: location + ) } /// Returns the IOKit class name of a registry entry. @@ -187,7 +223,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { } /// Finds the IOAVService for the given display. Caches the result per display. - /// Returns nil if no working AVService is found (built-in displays, or displays + /// Returns nil if no AVService is found (built-in displays, or displays /// that don't support DDC over the Apple Silicon AV path). /// /// Matching strategy: depth-first IOService traversal that pairs each DDC channel @@ -203,7 +239,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { } avServiceLock.unlock() - // Slow path: enumerate the IOService registry depth-first, pairing each working + // Slow path: enumerate the IOService registry depth-first, pairing each external // DDC channel with the nearest preceding display identity. let (serviceMap, ordered) = buildAVServiceMapByProximity() @@ -398,7 +434,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { return cs } - // MARK: - Synchronous DDC I/O (called on ddcQueue) + // MARK: - Synchronous DDC I/O (called on a per-display queue) /// Synchronous DDC write (VCP Set). Returns true on success. /// On ARM64 uses the IOAVService path; on x86_64 uses the IOFramebuffer I2C path. @@ -420,9 +456,9 @@ final class DDCService: ObservableObject, @unchecked Sendable { /// cache is cleared on reconnect. A wedged DDC controller (AOC Q27G3XMN) /// streams garbage and degrades further under retry hammering, so backing /// off protects both the monitor and the shared DCP I2C engine. Writes - /// are unaffected; they keep working on wedged controllers. Accessed only - /// on ddcQueue. + /// are unaffected; they keep working on wedged controllers. private var readFailStreak: [CGDirectDisplayID: Int] = [:] + private let readStateLock = NSLock() private let readQuarantineThreshold = 6 /// Quarantine expiry per display: after it passes, one fresh probe window /// opens (streak resets); persistent failure re-quarantines. Without an @@ -432,25 +468,35 @@ final class DDCService: ObservableObject, @unchecked Sendable { private let readQuarantineInterval: TimeInterval = 600 /// Synchronous DDC read (VCP Get). Returns (current, max) or nil on failure. - private func readSynchronous(displayID: CGDirectDisplayID, command: UInt8) -> (current: UInt16, max: UInt16)? { - if let until = readQuarantineUntil[displayID] { - guard Date() >= until else { return nil } + private func readSynchronous( + displayID: CGDirectDisplayID, + command: UInt8 + ) -> (current: UInt16, max: UInt16)? { + let quarantined = readStateLock.withLock { () -> Bool in + guard let until = readQuarantineUntil[displayID] else { return false } + guard Date() >= until else { return true } readQuarantineUntil.removeValue(forKey: displayID) readFailStreak[displayID] = 0 + return false } + guard !quarantined else { return nil } + #if arch(arm64) let result = arm64Read(displayID: displayID, command: command) #else let result = intelReadSynchronous(displayID: displayID, command: command) #endif - if result == nil { - let streak = readFailStreak[displayID, default: 0] + 1 - readFailStreak[displayID] = streak - if streak >= readQuarantineThreshold { - readQuarantineUntil[displayID] = Date().addingTimeInterval(readQuarantineInterval) + + readStateLock.withLock { + if result == nil { + let streak = readFailStreak[displayID, default: 0] + 1 + readFailStreak[displayID] = streak + if streak >= readQuarantineThreshold { + readQuarantineUntil[displayID] = Date().addingTimeInterval(readQuarantineInterval) + } + } else { + readFailStreak[displayID] = 0 } - } else { - readFailStreak[displayID] = 0 } return result } @@ -585,10 +631,11 @@ final class DDCService: ObservableObject, @unchecked Sendable { cacheLock.lock() vcpCache.removeValue(forKey: displayID) cacheLock.unlock() - ddcQueue.async { - self.readFailStreak.removeValue(forKey: displayID) - self.readQuarantineUntil.removeValue(forKey: displayID) + readStateLock.withLock { + readFailStreak.removeValue(forKey: displayID) + readQuarantineUntil.removeValue(forKey: displayID) } + operationQueues.removeQueue(for: displayID) #if arch(arm64) invalidateAVServiceCache(for: displayID) #endif @@ -602,9 +649,9 @@ final class DDCService: ObservableObject, @unchecked Sendable { /// per-removed-ID cleanup never sees that, and the stale map then writes /// one monitor's brightness into the other's channel. func invalidateAllChannelMappings() { - ddcQueue.async { - self.readFailStreak.removeAll() - self.readQuarantineUntil.removeAll() + readStateLock.withLock { + readFailStreak.removeAll() + readQuarantineUntil.removeAll() } #if arch(arm64) avServiceLock.lock() @@ -624,7 +671,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { value: UInt16, completion: ((Bool) -> Void)? = nil ) { - ddcQueue.async { + operationQueues.queue(for: displayID).async { for attempt in 0..<3 { if self.writeSynchronous(displayID: displayID, command: command, value: value) { // Invalidate cached value so next read reflects the new setting. @@ -656,7 +703,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { } cacheLock.unlock() - ddcQueue.async { + operationQueues.queue(for: displayID).async { for attempt in 0..<3 { if let r = self.readSynchronous(displayID: displayID, command: command) { self.cacheLock.lock() @@ -696,7 +743,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { } return await withCheckedContinuation { continuation in - ddcQueue.async { + operationQueues.queue(for: displayID).async { var result: [UInt8: UInt16?] = [:] var cachedCodes = Set() From 5523bd9cdcc3366e4f35d20be8598db47446c06c Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 10:04:05 +0800 Subject: [PATCH 07/10] fix: preview slow DDC brightness changes in software --- Crisp/Services/BrightnessService.swift | 66 +++++++++++++++----------- 1 file changed, 39 insertions(+), 27 deletions(-) diff --git a/Crisp/Services/BrightnessService.swift b/Crisp/Services/BrightnessService.swift index 81b87bc..7baa7ab 100644 --- a/Crisp/Services/BrightnessService.swift +++ b/Crisp/Services/BrightnessService.swift @@ -439,7 +439,6 @@ final class BrightnessService: @unchecked Sendable { /// slider drag can never build a queue of stale writes behind the slow I2C bus. private var pendingDDCPercent: [CGDirectDisplayID: Double] = [:] private var ddcPumpActive: Set = [] - private var ddcFailStreak: [CGDirectDisplayID: Int] = [:] /// Timestamp of the last DDC brightness write per display, used to pace writes. private var lastDDCWriteInstant: [CGDirectDisplayID: DispatchTime] = [:] private let ddcPumpLock = NSLock() @@ -488,24 +487,25 @@ final class BrightnessService: @unchecked Sendable { let alreadyPumping = ddcPumpActive.contains(displayID) if !alreadyPumping { ddcPumpActive.insert(displayID) } ddcPumpLock.unlock() - if !alreadyPumping { pumpDDCWrite(for: displayID) } + let ddcStatus = ddcAvailableLock.withLock { ddcAvailable[displayID] } queue.async { [weak self] in guard let self else { return } - if percent < self.gammaBlendThreshold { - self.setSoftwareBrightness(percent / self.gammaBlendThreshold * 100.0, for: displayID) - } else if let f = self.currentSoftwareBrightness(for: displayID), f < 1.0 { - // Only clear a software dim once DDC has actually succeeded on - // this display. While it is still unproven (nil), a display - // whose writes all fail (Dell without a DDC channel) would - // otherwise flash to full on every attempt, fighting the gamma - // fallback that is actually doing the dimming. - let proven = self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] == true } - if proven { + if ddcStatus == nil { + self.setSoftwareBrightness(percent, for: displayID) + } else if ddcStatus == true { + if percent < self.gammaBlendThreshold { + self.setSoftwareBrightness( + percent / self.gammaBlendThreshold * 100.0, + for: displayID + ) + } else if let factor = self.currentSoftwareBrightness(for: displayID), factor < 1.0 { self.setSoftwareBrightness(100.0, for: displayID) } } } + // Queue the visible preview before hardware work can complete and clear it. + if !alreadyPumping { pumpDDCWrite(for: displayID) } } private func pumpDDCWrite(for displayID: CGDirectDisplayID) { @@ -563,24 +563,37 @@ final class BrightnessService: @unchecked Sendable { guard let self else { return } if success { self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = true } - self.ddcPumpLock.withLock { self.ddcFailStreak[displayID] = 0 } - } else { - let streak = self.ddcPumpLock.withLock { () -> Int in - let s = (self.ddcFailStreak[displayID] ?? 0) + 1 - self.ddcFailStreak[displayID] = s - return s + let hasNewerTarget = self.ddcPumpLock.withLock { + self.pendingDDCPercent[displayID] != nil } - // A single flaky I2C write must not flip the display into gamma mode - // mid-drag (DDC + gamma dimming stack up and later "reset" visibly). - // Only give up on DDC after 3 consecutive failures. - if streak >= 3 { - self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = false } - DispatchQueue.main.async { [weak self] in - self?.setSoftwareBrightness(percent, for: displayID) + if hasNewerTarget { + self.pumpDDCWrite(for: displayID) + } else { + self.queue.async { + let stillCurrent = self.ddcPumpLock.withLock { + self.pendingDDCPercent[displayID] == nil + } + if stillCurrent { + let softwarePercent = percent < self.gammaBlendThreshold + ? percent / self.gammaBlendThreshold * 100.0 + : 100.0 + self.setSoftwareBrightness(softwarePercent, for: displayID) + } + // Do not consume a newer target before the stale-preview check. + self.pumpDDCWrite(for: displayID) } } + } else { + self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = false } + let fallbackPercent = self.ddcPumpLock.withLock { () -> Double in + let latest = self.pendingDDCPercent.removeValue(forKey: displayID) ?? percent + self.ddcPumpActive.remove(displayID) + return latest + } + self.queue.async { + self.setSoftwareBrightness(fallbackPercent, for: displayID) + } } - self.pumpDDCWrite(for: displayID) } } @@ -754,7 +767,6 @@ final class BrightnessService: @unchecked Sendable { } ddcPumpLock.withLock { pendingDDCPercent.removeValue(forKey: displayID) - ddcFailStreak.removeValue(forKey: displayID) lastDDCWriteInstant.removeValue(forKey: displayID) // ddcPumpActive stays: the pump owns it and removes itself once it // sees no pending value. From 7660d3bf2cbdecf4a4f2f1dbcf39066b816a0beb Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 10:32:24 +0800 Subject: [PATCH 08/10] fix: ignore stale DDC work after reconnect --- Crisp/Models/DDCOperationGeneration.swift | 33 +++++++++ Crisp/Models/DDCOperationQueuePool.swift | 6 +- Crisp/Services/BrightnessService.swift | 75 ++++++++++++++------ Crisp/Services/DDCService.swift | 23 ++++-- CrispTests/DDCOperationGenerationTests.swift | 25 +++++++ project.yml | 1 + 6 files changed, 131 insertions(+), 32 deletions(-) create mode 100644 Crisp/Models/DDCOperationGeneration.swift create mode 100644 CrispTests/DDCOperationGenerationTests.swift diff --git a/Crisp/Models/DDCOperationGeneration.swift b/Crisp/Models/DDCOperationGeneration.swift new file mode 100644 index 0000000..ab43263 --- /dev/null +++ b/Crisp/Models/DDCOperationGeneration.swift @@ -0,0 +1,33 @@ +import CoreGraphics + +struct DDCOperationGeneration { + struct Token: Equatable { + let topology: UInt64 + let request: UInt64 + } + + private var topologies: [CGDirectDisplayID: UInt64] = [:] + private var requests: [CGDirectDisplayID: UInt64] = [:] + + mutating func nextRequest(for displayID: CGDirectDisplayID) -> Token { + requests[displayID, default: 0] &+= 1 + return Token( + topology: topologies[displayID, default: 0], + request: requests[displayID, default: 0] + ) + } + + mutating func invalidate(displayID: CGDirectDisplayID) { + topologies[displayID, default: 0] &+= 1 + requests[displayID, default: 0] &+= 1 + } + + func isCurrentTopology(_ token: Token, for displayID: CGDirectDisplayID) -> Bool { + token.topology == topologies[displayID, default: 0] + } + + func isLatestRequest(_ token: Token, for displayID: CGDirectDisplayID) -> Bool { + isCurrentTopology(token, for: displayID) + && token.request == requests[displayID, default: 0] + } +} diff --git a/Crisp/Models/DDCOperationQueuePool.swift b/Crisp/Models/DDCOperationQueuePool.swift index 917ec03..019fc49 100644 --- a/Crisp/Models/DDCOperationQueuePool.swift +++ b/Crisp/Models/DDCOperationQueuePool.swift @@ -4,6 +4,8 @@ import CoreGraphics /// Keeps DDC operations serial per physical display without coupling displays. final class DDCOperationQueuePool: @unchecked Sendable { private let lock = NSLock() + // ponytail: keep this small map for the process lifetime so a reused display ID + // cannot overlap an in-flight operation on a second queue. private var queues: [CGDirectDisplayID: DispatchQueue] = [:] func queue(for displayID: CGDirectDisplayID) -> DispatchQueue { @@ -17,8 +19,4 @@ final class DDCOperationQueuePool: @unchecked Sendable { return queue } } - - func removeQueue(for displayID: CGDirectDisplayID) { - lock.withLock { _ = queues.removeValue(forKey: displayID) } - } } diff --git a/Crisp/Services/BrightnessService.swift b/Crisp/Services/BrightnessService.swift index 7baa7ab..975309a 100644 --- a/Crisp/Services/BrightnessService.swift +++ b/Crisp/Services/BrightnessService.swift @@ -406,9 +406,7 @@ final class BrightnessService: @unchecked Sendable { if currentStatus == false { // DDC known unavailable, go straight to software fallback - queue.async { [weak self] in - self?.setSoftwareBrightness(hardware, for: displayID) - } + applyLatestSoftwareBrightness(hardware, for: displayID) } else { writeDDCBrightnessCoalesced(percent: hardware, for: displayID) } @@ -434,11 +432,17 @@ final class BrightnessService: @unchecked Sendable { // MARK: - Coalescing DDC Writer - /// Latest pending brightness percent per display. Only one DDC write is in flight + private struct PendingDDCTarget { + let percent: Double + let token: DDCOperationGeneration.Token + } + + /// Latest pending brightness target per display. Only one DDC write is in flight /// per display and intermediate targets are dropped (latest wins), so a fast /// slider drag can never build a queue of stale writes behind the slow I2C bus. - private var pendingDDCPercent: [CGDirectDisplayID: Double] = [:] + private var pendingDDCTarget: [CGDirectDisplayID: PendingDDCTarget] = [:] private var ddcPumpActive: Set = [] + private var ddcOperationGeneration = DDCOperationGeneration() /// Timestamp of the last DDC brightness write per display, used to pace writes. private var lastDDCWriteInstant: [CGDirectDisplayID: DispatchTime] = [:] private let ddcPumpLock = NSLock() @@ -463,6 +467,20 @@ final class BrightnessService: @unchecked Sendable { /// auto-switch, and reconfiguration sync). Guarded by ddcAvailableLock. private var hdrDimmedDisplays: Set = [] + private func applyLatestSoftwareBrightness(_ percent: Double, for displayID: CGDirectDisplayID) { + let token = ddcPumpLock.withLock { + ddcOperationGeneration.nextRequest(for: displayID) + } + queue.async { [weak self] in + guard let self else { return } + let isLatest = self.ddcPumpLock.withLock { + self.ddcOperationGeneration.isLatestRequest(token, for: displayID) + } + guard isLatest else { return } + self.setSoftwareBrightness(percent, for: displayID) + } + } + func setHDRSoftwareDimming(_ on: Bool, for displayID: CGDirectDisplayID) { ddcAvailableLock.withLock { if on { hdrDimmedDisplays.insert(displayID) } else { hdrDimmedDisplays.remove(displayID) } @@ -477,13 +495,12 @@ final class BrightnessService: @unchecked Sendable { // gamma reset below clears the leftover software dim. let hdrDimmed = ddcAvailableLock.withLock { hdrDimmedDisplays.contains(displayID) } if hdrDimmed { - queue.async { [weak self] in - self?.setSoftwareBrightness(percent, for: displayID) - } + applyLatestSoftwareBrightness(percent, for: displayID) return } ddcPumpLock.lock() - pendingDDCPercent[displayID] = percent + let token = ddcOperationGeneration.nextRequest(for: displayID) + pendingDDCTarget[displayID] = PendingDDCTarget(percent: percent, token: token) let alreadyPumping = ddcPumpActive.contains(displayID) if !alreadyPumping { ddcPumpActive.insert(displayID) } ddcPumpLock.unlock() @@ -491,6 +508,10 @@ final class BrightnessService: @unchecked Sendable { let ddcStatus = ddcAvailableLock.withLock { ddcAvailable[displayID] } queue.async { [weak self] in guard let self else { return } + let isLatest = self.ddcPumpLock.withLock { + self.ddcOperationGeneration.isLatestRequest(token, for: displayID) + } + guard isLatest else { return } if ddcStatus == nil { self.setSoftwareBrightness(percent, for: displayID) } else if ddcStatus == true { @@ -513,7 +534,7 @@ final class BrightnessService: @unchecked Sendable { // Peek (don't consume yet): if we must wait to honour the pacing floor, // a newer drag value may arrive during the wait and should supersede this // one. Consuming only after the wait keeps "latest wins" intact. - guard pendingDDCPercent[displayID] != nil else { + guard pendingDDCTarget[displayID] != nil else { ddcPumpActive.remove(displayID) ddcPumpLock.unlock() return @@ -540,13 +561,14 @@ final class BrightnessService: @unchecked Sendable { // Now consume the latest pending value (drops any intermediate drag steps). ddcPumpLock.lock() - guard let percent = pendingDDCPercent.removeValue(forKey: displayID) else { + guard let target = pendingDDCTarget.removeValue(forKey: displayID) else { ddcPumpActive.remove(displayID) ddcPumpLock.unlock() return } lastDDCWriteInstant[displayID] = .now() ddcPumpLock.unlock() + let percent = target.percent // Denormalize percentage to display's native DDC range. // If max is unknown, default to 100 (safe for most monitors). @@ -561,17 +583,22 @@ final class BrightnessService: @unchecked Sendable { value: ddcValue ) { [weak self] success in guard let self else { return } + let currentTopology = self.ddcPumpLock.withLock { + self.ddcOperationGeneration.isCurrentTopology(target.token, for: displayID) + } + guard currentTopology else { return } if success { self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = true } let hasNewerTarget = self.ddcPumpLock.withLock { - self.pendingDDCPercent[displayID] != nil + self.pendingDDCTarget[displayID] != nil } if hasNewerTarget { self.pumpDDCWrite(for: displayID) } else { self.queue.async { let stillCurrent = self.ddcPumpLock.withLock { - self.pendingDDCPercent[displayID] == nil + self.ddcOperationGeneration.isLatestRequest(target.token, for: displayID) + && self.pendingDDCTarget[displayID] == nil } if stillCurrent { let softwarePercent = percent < self.gammaBlendThreshold @@ -585,13 +612,17 @@ final class BrightnessService: @unchecked Sendable { } } else { self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = false } - let fallbackPercent = self.ddcPumpLock.withLock { () -> Double in - let latest = self.pendingDDCPercent.removeValue(forKey: displayID) ?? percent + let fallback = self.ddcPumpLock.withLock { () -> PendingDDCTarget in + let latest = self.pendingDDCTarget.removeValue(forKey: displayID) ?? target self.ddcPumpActive.remove(displayID) return latest } self.queue.async { - self.setSoftwareBrightness(fallbackPercent, for: displayID) + let isLatest = self.ddcPumpLock.withLock { + self.ddcOperationGeneration.isLatestRequest(fallback.token, for: displayID) + } + guard isLatest else { return } + self.setSoftwareBrightness(fallback.percent, for: displayID) } } } @@ -661,7 +692,7 @@ final class BrightnessService: @unchecked Sendable { display.brightness = value // Above 100 the boost sync owns the transfer table (see setBrightness). if value <= 100 { - self.queue.async { self.setSoftwareBrightness(value, for: displayID) } + self.applyLatestSoftwareBrightness(value, for: displayID) } BrightnessBoostService.shared.syncOverlay(for: display) } @@ -724,9 +755,7 @@ final class BrightnessService: @unchecked Sendable { /// above 1.0 through here, on the same serial queue as the dim path, so /// slider motion above and below 100 is always one writer, one table. func setBoostFactor(_ factor: Double, for displayID: CGDirectDisplayID) { - queue.async { [weak self] in - self?.setSoftwareBrightness(factor * 100.0, for: displayID) - } + applyLatestSoftwareBrightness(factor * 100.0, for: displayID) } /// Resets the gamma table for a display back to the identity curve. @@ -766,10 +795,10 @@ final class BrightnessService: @unchecked Sendable { _ = softwareBrightnessFactors.removeValue(forKey: displayID) } ddcPumpLock.withLock { - pendingDDCPercent.removeValue(forKey: displayID) + ddcOperationGeneration.invalidate(displayID: displayID) + pendingDDCTarget.removeValue(forKey: displayID) + ddcPumpActive.remove(displayID) lastDDCWriteInstant.removeValue(forKey: displayID) - // ddcPumpActive stays: the pump owns it and removes itself once it - // sees no pending value. } } diff --git a/Crisp/Services/DDCService.swift b/Crisp/Services/DDCService.swift index 21458a3..a853461 100644 --- a/Crisp/Services/DDCService.swift +++ b/Crisp/Services/DDCService.swift @@ -55,6 +55,8 @@ final class DDCService: ObservableObject, @unchecked Sendable { #if arch(arm64) private var avServiceCache: [CGDirectDisplayID: IOAVServiceRef] = [:] private let avServiceLock = NSLock() + private let avServiceBuildLock = NSLock() + private var avServiceTopology: UInt64 = 0 /// Ordered list of all external AVServices found during last enumeration. private var allExternalAVServices: [IOAVServiceRef] = [] #endif @@ -239,6 +241,18 @@ final class DDCService: ObservableObject, @unchecked Sendable { } avServiceLock.unlock() + // Only one registry walk may publish a channel map at a time. + avServiceBuildLock.lock() + defer { avServiceBuildLock.unlock() } + + avServiceLock.lock() + if let cached = avServiceCache[displayID] { + avServiceLock.unlock() + return cached + } + let topology = avServiceTopology + avServiceLock.unlock() + // Slow path: enumerate the IOService registry depth-first, pairing each external // DDC channel with the nearest preceding display identity. let (serviceMap, ordered) = buildAVServiceMapByProximity() @@ -247,12 +261,10 @@ final class DDCService: ObservableObject, @unchecked Sendable { return nil } - // Re-check cache (double-checked locking) in case another thread enumerated - // and populated the cache while we were enumerating without the lock held. avServiceLock.lock() - if let cached = avServiceCache[displayID] { + guard topology == avServiceTopology else { avServiceLock.unlock() - return cached + return nil } allExternalAVServices = ordered for (extID, avService) in serviceMap { @@ -267,6 +279,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { /// Invalidates the cached IOAVService for the given display (e.g. after display reconnect). func invalidateAVServiceCache(for displayID: CGDirectDisplayID) { avServiceLock.lock() + avServiceTopology &+= 1 avServiceCache.removeValue(forKey: displayID) avServiceLock.unlock() } @@ -635,7 +648,6 @@ final class DDCService: ObservableObject, @unchecked Sendable { readFailStreak.removeValue(forKey: displayID) readQuarantineUntil.removeValue(forKey: displayID) } - operationQueues.removeQueue(for: displayID) #if arch(arm64) invalidateAVServiceCache(for: displayID) #endif @@ -655,6 +667,7 @@ final class DDCService: ObservableObject, @unchecked Sendable { } #if arch(arm64) avServiceLock.lock() + avServiceTopology &+= 1 avServiceCache.removeAll() allExternalAVServices.removeAll() avServiceLock.unlock() diff --git a/CrispTests/DDCOperationGenerationTests.swift b/CrispTests/DDCOperationGenerationTests.swift new file mode 100644 index 0000000..ce5b14d --- /dev/null +++ b/CrispTests/DDCOperationGenerationTests.swift @@ -0,0 +1,25 @@ +import XCTest + +final class DDCOperationGenerationTests: XCTestCase { + func testNewRequestMakesOlderRequestStaleWithinSameTopology() { + var generations = DDCOperationGeneration() + let first = generations.nextRequest(for: 5) + let second = generations.nextRequest(for: 5) + + XCTAssertTrue(generations.isCurrentTopology(first, for: 5)) + XCTAssertFalse(generations.isLatestRequest(first, for: 5)) + XCTAssertTrue(generations.isLatestRequest(second, for: 5)) + } + + func testInvalidationRejectsCompletionFromPreviousTopology() { + var generations = DDCOperationGeneration() + let beforeReconnect = generations.nextRequest(for: 5) + + generations.invalidate(displayID: 5) + let afterReconnect = generations.nextRequest(for: 5) + + XCTAssertFalse(generations.isCurrentTopology(beforeReconnect, for: 5)) + XCTAssertFalse(generations.isLatestRequest(beforeReconnect, for: 5)) + XCTAssertTrue(generations.isLatestRequest(afterReconnect, for: 5)) + } +} diff --git a/project.yml b/project.yml index 889f21f..2e58599 100644 --- a/project.yml +++ b/project.yml @@ -58,6 +58,7 @@ targets: - path: Crisp/Models/DisplayModeGeometry.swift - path: Crisp/Models/DDCServiceMatcher.swift - path: Crisp/Models/DDCOperationQueuePool.swift + - path: Crisp/Models/DDCOperationGeneration.swift - path: Crisp/Models/RefreshRateFormat.swift - path: Crisp/Models/GammaPersistenceKey.swift - path: Crisp/Models/VariableRefreshModes.swift From 664d2ccdbada3d0978ee23ca454e948a3b164f89 Mon Sep 17 00:00:00 2001 From: Chisa Kotegawa <1522837885zyl@gmail.com> Date: Fri, 21 Aug 2026 10:46:42 +0800 Subject: [PATCH 09/10] fix: discard DDC callbacks across topology changes --- Crisp/Models/DDCOperationGeneration.swift | 10 +- Crisp/Services/BrightnessService.swift | 131 +++++++++++++------ Crisp/Services/DisplayManager.swift | 5 + CrispTests/DDCOperationGenerationTests.swift | 8 +- 4 files changed, 106 insertions(+), 48 deletions(-) diff --git a/Crisp/Models/DDCOperationGeneration.swift b/Crisp/Models/DDCOperationGeneration.swift index ab43263..34860c4 100644 --- a/Crisp/Models/DDCOperationGeneration.swift +++ b/Crisp/Models/DDCOperationGeneration.swift @@ -9,14 +9,18 @@ struct DDCOperationGeneration { private var topologies: [CGDirectDisplayID: UInt64] = [:] private var requests: [CGDirectDisplayID: UInt64] = [:] - mutating func nextRequest(for displayID: CGDirectDisplayID) -> Token { - requests[displayID, default: 0] &+= 1 - return Token( + func currentToken(for displayID: CGDirectDisplayID) -> Token { + Token( topology: topologies[displayID, default: 0], request: requests[displayID, default: 0] ) } + mutating func nextRequest(for displayID: CGDirectDisplayID) -> Token { + requests[displayID, default: 0] &+= 1 + return currentToken(for: displayID) + } + mutating func invalidate(displayID: CGDirectDisplayID) { topologies[displayID, default: 0] &+= 1 requests[displayID, default: 0] &+= 1 diff --git a/Crisp/Services/BrightnessService.swift b/Crisp/Services/BrightnessService.swift index 975309a..83177c3 100644 --- a/Crisp/Services/BrightnessService.swift +++ b/Crisp/Services/BrightnessService.swift @@ -299,6 +299,9 @@ final class BrightnessService: @unchecked Sendable { } } } else { + let readToken = ddcPumpLock.withLock { + ddcOperationGeneration.currentToken(for: displayID) + } // First check if DDC is already known to be unavailable; if so skip the // async DDC call and just read the current gamma-derived brightness. let knownUnavailable: Bool = ddcAvailableLock.withLock { @@ -316,19 +319,31 @@ final class BrightnessService: @unchecked Sendable { guard let self else { return } if let result = result, result.max > 0 { let brightness = Double(result.current) / Double(result.max) * 100.0 - self.ddcAvailableLock.lock() - let firstRead = self.ddcAvailable[displayID] != true - self.ddcAvailable[displayID] = true - self.ddcMaxBrightness[displayID] = result.max - self.ddcAvailableLock.unlock() + let firstRead: Bool? = self.ddcPumpLock.withLock { + guard self.ddcOperationGeneration.isCurrentTopology( + readToken, for: displayID + ) else { return nil } + return self.ddcAvailableLock.withLock { + let firstRead = self.ddcAvailable[displayID] != true + self.ddcAvailable[displayID] = true + self.ddcMaxBrightness[displayID] = result.max + return firstRead + } + } + guard let firstRead else { return } Task { @MainActor in // DDC reads quantize (many panels expose a coarser internal // scale than they accept), so a value we just set can read back // 1-2% off and twitch the slider on every open. Adopt the read // only on the first seed, or when it differs enough to be a real // external change (the monitor's own buttons), not read noise. - if firstRead || abs(brightness - display.brightness) > 3.0 { - display.brightness = brightness + self.ddcPumpLock.withLock { + guard self.ddcOperationGeneration.isCurrentTopology( + readToken, for: displayID + ) else { return } + if firstRead || abs(brightness - display.brightness) > 3.0 { + display.brightness = brightness + } } } } @@ -526,11 +541,18 @@ final class BrightnessService: @unchecked Sendable { } } // Queue the visible preview before hardware work can complete and clear it. - if !alreadyPumping { pumpDDCWrite(for: displayID) } + if !alreadyPumping { pumpDDCWrite(for: displayID, topology: token) } } - private func pumpDDCWrite(for displayID: CGDirectDisplayID) { + private func pumpDDCWrite( + for displayID: CGDirectDisplayID, + topology: DDCOperationGeneration.Token + ) { ddcPumpLock.lock() + guard ddcOperationGeneration.isCurrentTopology(topology, for: displayID) else { + ddcPumpLock.unlock() + return + } // Peek (don't consume yet): if we must wait to honour the pacing floor, // a newer drag value may arrive during the wait and should supersede this // one. Consuming only after the wait keeps "latest wins" intact. @@ -553,7 +575,7 @@ final class BrightnessService: @unchecked Sendable { let remaining = minDDCWriteInterval - elapsed if remaining > 0 { queue.asyncAfter(deadline: .now() + remaining) { [weak self] in - self?.pumpDDCWrite(for: displayID) + self?.pumpDDCWrite(for: displayID, topology: topology) } return } @@ -561,13 +583,16 @@ final class BrightnessService: @unchecked Sendable { // Now consume the latest pending value (drops any intermediate drag steps). ddcPumpLock.lock() + guard ddcOperationGeneration.isCurrentTopology(topology, for: displayID) else { + ddcPumpLock.unlock() + return + } guard let target = pendingDDCTarget.removeValue(forKey: displayID) else { ddcPumpActive.remove(displayID) ddcPumpLock.unlock() return } lastDDCWriteInstant[displayID] = .now() - ddcPumpLock.unlock() let percent = target.percent // Denormalize percentage to display's native DDC range. @@ -583,49 +608,55 @@ final class BrightnessService: @unchecked Sendable { value: ddcValue ) { [weak self] success in guard let self else { return } - let currentTopology = self.ddcPumpLock.withLock { - self.ddcOperationGeneration.isCurrentTopology(target.token, for: displayID) - } - guard currentTopology else { return } if success { - self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = true } - let hasNewerTarget = self.ddcPumpLock.withLock { - self.pendingDDCTarget[displayID] != nil + let hasNewerTarget: Bool? = self.ddcPumpLock.withLock { + guard self.ddcOperationGeneration.isCurrentTopology( + target.token, for: displayID + ) else { return nil } + self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = true } + return self.pendingDDCTarget[displayID] != nil } + guard let hasNewerTarget else { return } if hasNewerTarget { - self.pumpDDCWrite(for: displayID) + self.pumpDDCWrite(for: displayID, topology: target.token) } else { self.queue.async { - let stillCurrent = self.ddcPumpLock.withLock { - self.ddcOperationGeneration.isLatestRequest(target.token, for: displayID) - && self.pendingDDCTarget[displayID] == nil - } - if stillCurrent { + self.ddcPumpLock.withLock { + guard self.ddcOperationGeneration.isLatestRequest( + target.token, for: displayID + ), self.pendingDDCTarget[displayID] == nil else { return } let softwarePercent = percent < self.gammaBlendThreshold ? percent / self.gammaBlendThreshold * 100.0 : 100.0 self.setSoftwareBrightness(softwarePercent, for: displayID) } - // Do not consume a newer target before the stale-preview check. - self.pumpDDCWrite(for: displayID) + self.pumpDDCWrite(for: displayID, topology: target.token) } } } else { - self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = false } - let fallback = self.ddcPumpLock.withLock { () -> PendingDDCTarget in + let fallback: PendingDDCTarget? = self.ddcPumpLock.withLock { + guard self.ddcOperationGeneration.isCurrentTopology( + target.token, for: displayID + ) else { return nil } + self.ddcAvailableLock.withLock { self.ddcAvailable[displayID] = false } let latest = self.pendingDDCTarget.removeValue(forKey: displayID) ?? target self.ddcPumpActive.remove(displayID) return latest } + guard let fallback else { return } self.queue.async { - let isLatest = self.ddcPumpLock.withLock { - self.ddcOperationGeneration.isLatestRequest(fallback.token, for: displayID) + self.ddcPumpLock.withLock { + guard self.ddcOperationGeneration.isLatestRequest( + fallback.token, for: displayID + ) else { return } + self.setSoftwareBrightness(fallback.percent, for: displayID) } - guard isLatest else { return } - self.setSoftwareBrightness(fallback.percent, for: displayID) } } } + // Keep topology invalidation behind the enqueue so a reconnect cannot put an + // old request after the new display's first request on the per-display queue. + ddcPumpLock.unlock() } // MARK: - Smooth Brightness Transitions @@ -774,32 +805,48 @@ final class BrightnessService: @unchecked Sendable { ddcAvailableLock.withLock { ddcAvailable[displayID] } } + /// Invalidates transport state without discarding the display's saved software settings. + /// Called for every online external ID when macOS reports a topology change because IDs + /// can swap physical panels without ever leaving the online display list. + @MainActor + func invalidateDDCTopology(for displayIDs: Set) { + for displayID in displayIDs { + animators[displayID]?.cancel() + } + ddcPumpLock.withLock { + for displayID in displayIDs { + ddcOperationGeneration.invalidate(displayID: displayID) + pendingDDCTarget.removeValue(forKey: displayID) + ddcPumpActive.remove(displayID) + lastDDCWriteInstant.removeValue(forKey: displayID) + } + ddcAvailableLock.withLock { + for displayID in displayIDs { + ddcAvailable.removeValue(forKey: displayID) + ddcMaxBrightness.removeValue(forKey: displayID) + } + } + } + } + /// Clears all per-display state for a disconnected display. /// Call this when a display is removed so stale state cannot pollute a reconnect. @MainActor func invalidateDDCState(for displayID: CGDirectDisplayID) { + invalidateDDCTopology(for: [displayID]) ddcAvailableLock.withLock { - ddcAvailable.removeValue(forKey: displayID) - ddcMaxBrightness.removeValue(forKey: displayID) // Display IDs are reused: without this, a disconnected HDR // display's software-dimming routing would stick to whatever // display inherits its ID next. - hdrDimmedDisplays.remove(displayID) + _ = hdrDimmedDisplays.remove(displayID) } // Same ID-reuse hazard for the rest: reapplySoftwareBrightnessIfNeeded // reads the in-memory factor first, so a display inheriting this ID // would silently get the departed display's dimming factor. - animators[displayID]?.cancel() animators.removeValue(forKey: displayID) softwareBrightnessLock.withLock { _ = softwareBrightnessFactors.removeValue(forKey: displayID) } - ddcPumpLock.withLock { - ddcOperationGeneration.invalidate(displayID: displayID) - pendingDDCTarget.removeValue(forKey: displayID) - ddcPumpActive.remove(displayID) - lastDDCWriteInstant.removeValue(forKey: displayID) - } } /// Re-applies the software brightness for a display after wake from sleep or hot-plug. diff --git a/Crisp/Services/DisplayManager.swift b/Crisp/Services/DisplayManager.swift index 256ed6d..0e7e0ae 100644 --- a/Crisp/Services/DisplayManager.swift +++ b/Crisp/Services/DisplayManager.swift @@ -151,6 +151,11 @@ class DisplayManager: ObservableObject { let currentIDs = Set(displays.map { $0.displayID }) let newIDSet = Set((0.. Date: Fri, 21 Aug 2026 10:51:28 +0800 Subject: [PATCH 10/10] fix: ignore stale DDC reads after user input --- Crisp/Services/BrightnessService.swift | 2 +- CrispTests/DDCOperationGenerationTests.swift | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Crisp/Services/BrightnessService.swift b/Crisp/Services/BrightnessService.swift index 83177c3..801a000 100644 --- a/Crisp/Services/BrightnessService.swift +++ b/Crisp/Services/BrightnessService.swift @@ -338,7 +338,7 @@ final class BrightnessService: @unchecked Sendable { // only on the first seed, or when it differs enough to be a real // external change (the monitor's own buttons), not read noise. self.ddcPumpLock.withLock { - guard self.ddcOperationGeneration.isCurrentTopology( + guard self.ddcOperationGeneration.isLatestRequest( readToken, for: displayID ) else { return } if firstRead || abs(brightness - display.brightness) > 3.0 { diff --git a/CrispTests/DDCOperationGenerationTests.swift b/CrispTests/DDCOperationGenerationTests.swift index 4a8a697..4a44fc1 100644 --- a/CrispTests/DDCOperationGenerationTests.swift +++ b/CrispTests/DDCOperationGenerationTests.swift @@ -3,9 +3,11 @@ import XCTest final class DDCOperationGenerationTests: XCTestCase { func testNewRequestMakesOlderRequestStaleWithinSameTopology() { var generations = DDCOperationGeneration() + let read = generations.currentToken(for: 5) let first = generations.nextRequest(for: 5) let second = generations.nextRequest(for: 5) + XCTAssertFalse(generations.isLatestRequest(read, for: 5)) XCTAssertTrue(generations.isCurrentTopology(first, for: 5)) XCTAssertFalse(generations.isLatestRequest(first, for: 5)) XCTAssertTrue(generations.isLatestRequest(second, for: 5))