Fix external brightness delays with multiple displays - #71
Conversation
didriksg
left a comment
There was a problem hiding this comment.
This is the right fix for the right cause. One global DDC queue was always going to let a slow monitor hold the others hostage, and per-display queues plus location-based matching are what I'd have reached for too. The timing numbers from the two AOCs are appreciated; that's the kind of evidence I can't get here.
Two regressions I found reading the diff, both in the multi-display path this PR is about:
-
DisplayManager.refreshDisplaysnow callsinvalidateDDCTopology(for:)with every online external display on every add, remove or move event, not just the display that changed.invalidateDDCTopologycancels the animator, dropspendingDDCTarget, bumps the topology generation and clearsddcAvailablefor each of them. So: drag display A's slider to 80, and while the write waits out the 50 ms pacing floor, display B replugs (or you rearrange displays in System Settings, which is a.movedFlagwith no ID change). A's pending target is gone,pumpDDCWritesees a stale topology and returns, and the software fallback never runs either because it checks the same generation. The slider says 80, the monitor never got it. Same thing truncates a runningsetBrightnessSmoothfade from the brightness keys or a preset. Before this PR only the removed display's state was cleared. Could you scope the invalidation to the IDs that actually disappeared or changed identity or location? -
ddcFailStreakis deleted, so a single failed coalesced write now setsddcAvailable = falseuntil reconnect.docs/ddc-notes.mddocuments the three-failure latch on purpose: the AOC Q27G3XMN drops about half of its DDC commands at random, and with this change it would fall to gamma mode on the first drag.DDCService.writeAsyncretrying three times within 100 ms doesn't cover that, since the monitor ignores the retries too. Either keep a multi-cycle grace before latching, or make the case for the change in the PR so the notes can follow.
Two smaller things: please drop docs/superpowers/plans/... and docs/superpowers/specs/... (they're your planning notes, and already out of step with the code, for example the removeQueue that never got implemented) and the .worktrees/ line in .gitignore.
With those in, I'll run it on my displays and merge. Thanks for digging into this one.
Summary
Root cause
Crisp previously routed every DDC transaction through one global serial queue. On the reproduced setup, one external display could block an I2C read for about 12 seconds, which delayed brightness changes for the other display as well. Identical monitors could also be paired by unstable traversal order, while reconnect callbacks could update state belonging to a newer display topology.
Verification
Closes #72.