Skip to content

crispctl: add external HDR controls - #116

Merged
didriksg merged 2 commits into
didriksg:mainfrom
Juns-g:feature/crispctl-external-hdr
Sep 6, 2026
Merged

crispctl: add external HDR controls#116
didriksg merged 2 commits into
didriksg:mainfrom
Juns-g:feature/crispctl-external-hdr

Conversation

@Juns-g

@Juns-g Juns-g commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a strictly focused external-HDR slice to crispctl:

crispctl hdr get <display>
crispctl hdr set <display> on|off
  • Uses the same BrightnessBoostService eligibility, live-state, write, boost-collapse, and request-arbitration paths as Crisp's existing GUI HDR toggle.
  • Supports only HDR-capable external displays for which Crisp shows that toggle.
  • Refuses built-in displays with guidance to use Extra Brightness when eligible, and refuses unsupported external displays.
  • Performs one HDR write request at most, followed by bounded live read-back; it never retries the write and reports success only when the service accepted it and live state matches.
  • Carries the selected display's system UUID through delayed HDR-off work, revalidates the original runtime ID + UUID + eligibility before writes and read-back, and combines non-reusable request tokens with a connection epoch so disconnect/reconnect cannot revive a stale operation.
  • Keeps the existing JSON and exit-code conventions, adding an optional hdr response object with displayID and enabled.

Help and documentation

  • Adds only the two concise HDR rows to the conventional crispctl help table introduced by Give crispctl a conventional help screen and uuid selectors #107.
  • Extends the existing parser/help tests so the accepted commands and help cannot drift.
  • Documents eligibility, built-in remediation, authoritative reads, bounded verification, uncertain outcomes, no automatic retry, response JSON, and unchanged exit codes in README.
  • Also records the existing crispctl: add Extra Brightness controls #102 behavior noted in its Approved Review: Extra Brightness disable settles in about 0.5 seconds, transitional reads can temporarily show brightness above the current maximum, and the collapse finishes at 100 rather than restoring the pre-boost value.

Verification

  • DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer make check
    • SwiftLint clean
    • full test suite green
    • 209 localization keys present, 0 missing
  • Focused CrispControlModelTests: 23 passed, 0 failed, 0 skipped
  • Strict Release crispctl build with warnings as errors: passed
    • architectures: x86_64 arm64
  • Built CLI crispctl help: verified to contain the two HDR commands in the concise command table
  • Static added-line security scan: clean
  • git diff --check: clean
  • Independent frozen-range review: passed with 0 blocking findings
    • connection-epoch and request-token race paths were reviewed statically; no new CG callback injection scaffold was added

Hardware validation

Validated on 2026-09-06 against the exact PR head eba2ced8a733ec8e601fb5901835c091aac54d80, using an isolated Release build without installing or replacing /Applications/Crisp.app. Before every command sequence, the running app and crispctl.sock owner were verified to be that build.

  • Supported external display: LG TV SSCR2, system UUID CBD3871A-5CC4-42B6-9C2A-AC2D67FEDFC9, runtime ID 3.
  • Live hdr get reported the authoritative off state. A single hdr set ... on returned enabled: true; independent immediate and delayed reads remained true. AppKit EDR values changed from potential=1/current=1 to approximately potential=7.75/current=1.2, independently confirming the WindowServer HDR/EDR transition.
  • A separate single hdr set ... off returned enabled: false, and an independent read remained false. One intervening manual HDR-off action by the tester was explicitly excluded from the CLI-off evidence.
  • With Extra Brightness enabled, HDR became true, live maxBrightness became 200, and logical brightness was set to 120. A single hdr set ... off returned HDR false and safely collapsed Extra Brightness to off, brightness to 100, and maxBrightness to 100; immediate and one-second settled reads agreed.
  • The built-in display refused HDR get/set with exit code 3 and guidance to use Extra Brightness. A Sidecar external display without the explicit HDR capability also refused get/set with exit code 3. Their states did not change.
  • A real LG power-cycle removed the display from the live list while the app and socket stayed running. On reconnect it returned with the same system UUID and runtime ID, HDR false, Extra Brightness off, and no stale HDR mutation. The narrower case of deliberately disconnecting while an HDR write is in flight was not forced on hardware; request-token/UUID/connection-epoch handling remains covered by focused tests and the independent static review.
  • Final recovery was verified: the isolated build was stopped, the previously running PR crispctl: add Extra Brightness controls #102 test build again owned the socket, LG brightness was restored to 100 through DDC with Extra Brightness off, the built-in display remained at 100 with Extra Brightness off, and Sidecar was left unchanged.

Out of scope

No batch operations, connect/disconnect, protocol versioning, packaging/release, Homebrew, presets, skill files, website changes, localization changes, or unrelated refactor.

@Juns-g
Juns-g marked this pull request as ready for review September 5, 2026 17:09
@didriksg

didriksg commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Ran this on top of main against three displays here, and it holds up: every refusal path lands where it should, on and off work by id and by uuid with the headroom going 1.0 to 11.56 and back, and the boost collapse through hdr off matches your LG run (brightness 120 at max 200, then off, 100, max 100, backend back to DDC in 1.3 s). The model change is right, and the read-back loop with its final read is the shape I'd have written.

I pushed one commit onto your branch rather than ask for another round, since it's all deletion. Three things in it, the first two measured:

The connection epoch is gone. It's a global counter, so any add, remove or move of any display refused a switch in flight. With Extra Brightness on, an hdr off while another display was switched off was refused with epoch 2 != 0, and the callback carried a movedFlag for the HDR display itself, so any arrangement change that moved the display refused its own pending switch. setHDRPreference is the panel's path too. The uuid check you added already covers the case the epoch was for, per display, so that stays.

stableDisplayUUID was DisplayInfo.displayUUID line for line, so the list and the read use displayUUID again.

The README is down to one short paragraph for hdr; the reference lives in crispctl help.

One more thing the run turned up, and it was mine, not yours: an off 0.3 s after an on was refused 4 times in 16 with "not accepted", and the probe lines said it was the request token. The panel's HDR toggle resyncs from the live state after every screen change and re-sent the value it had just read, replacing your token while the CLI's off sat in its settle. Fixed in #124; it's older than your PR, and the CLI was the first thing that could collide with it. With both in, 0 refused in 16, and a hot-plug during an off goes through.

Merging once CI is green.

@didriksg didriksg mentioned this pull request Sep 6, 2026
didriksg added a commit that referenced this pull request Sep 6, 2026
HDRToggleView resyncs its switch from the live state on every screen-parameters notification, and every HDR flip fires one. The programmatic-change guard around that write does not hold, because SwiftUI runs onChange after the view update, by which time the flag is already reset, so each resync re-sent the value it had just read as a fresh setHDRPreference. Harmless while the toggle was the only writer, since the echo carries the same value, but each echo replaces the request token, and a request from anywhere else that is waiting out its settle at that moment is refused. Measured with crispctl's hdr set from #116: an off 0.3 s after an on was refused 4 times in 16, and 0 in 16 with this guard, which sends a request only when the switch differs from the live state.
@didriksg
didriksg merged commit 982a47f into didriksg:main Sep 6, 2026
1 check passed
didriksg added a commit that referenced this pull request Sep 6, 2026
@Juns-g

Juns-g commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough testing and for pushing the cleanup commit directly!

You're completely right on the epoch and UUID duplication — I was pairing with GPT-5.6 sol on this, and it definitely loves to overthink edge cases and over-engineer race protections. Your multi-display test caught that global counter trap nicely.

The trimmed README reads much cleaner as well. Glad the core flow and read-back logic held up, and thanks for tracking down that fast toggle race in #124 too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants