Repository navigation
Resolve spotread -y selector from the instrument table (1.9.2) - #8
Merged
Merged
Conversation
DisplayTypeExtensions.ToSpotreadFlag mapped each DisplayType to a fixed Argyll technology letter (e, o, b, l, c, m, p). Argyll builds the -y table per instrument at run time: a driver's static base rows plus one row per installed CCSS/CCMX. The i1D3 driver's static rows are only n|l (Non-Refresh) and r|c (Refresh), and the technology letters exist only when the X-Rite EDR corrections have been imported into Argyll's data directory. On a fresh install spotread therefore exits with "Failed to locate display type matching 'o'" for OLED, and the same happens for LcdLed (e) and LcdWideGamut (b). SpotreadDisplayTypeTable.Parse reads the -y block from spotread usage text (first row prefixed with -y, continuation rows indented, the generic "l|c Other" row, one- and two-character selectors, log-prefixed lines). SpotreadDisplayTypeTable.Resolve picks, in order: a non-base row whose description names the technology, preferring Argyll's own letter for it; the base row for the type's refresh class (OLED and all LCD types are non-refresh per disptechs.c; CRT, plasma and projector are refresh); a row accepting the generic selector; the generic selector l or c when the table is unknown. ColorimeterService parses the table from the spotread -? output it already captures at detection, stores it on ColorimeterInfo.DisplayTypes, and passes it to SpotreadSession.StartAsync for the persistent and transient sessions. SetDisplayType logs the resolved selector and the reason. SpotreadSession recognises the rejection diagnostic, stops classifying the usage lines that follow it, and keeps them in a dedicated buffer. StartProcessAsync waits for the exited process's output to drain (Process.WaitForExit without a timeout is the only call that guarantees the async handlers ran) before parsing that buffer, and StartAsync restarts once with a selector resolved against the table spotread itself printed. The failure message lists the selectors the instrument accepts. Session ownership in StartProcessAsync is now a try/finally. Tests pin the reporter's clean-install i1D3 usage, a usage with imported EDR rows, and a usage with no enumerated instrument, and check the resolved selector for every DisplayType against each. Session tests cover argument construction with and without a table and the rejection capture. Release notes and version 1.9.2.
1 task done
Drop "wide gamut" from the LcdWideGamut keywords: it matched the CCFL wide-gamut EDR row (L) ahead of the LED phosphor rows when no RGB LED row (b) was installed. A default row that names a technology ("LCD, CCFL Backlight [Default,CB1]" on non-i1D3 drivers) is now a correction, not a base row.
ColorimeterService re-runs the spotread -? probe before opening a session when the cached table has no instrument rows (meter plugged in after detection), and adopts the table a session actually resolved against, so a rejection retry happens once per app run rather than once per session.
SpotreadSession.StartProcessAsync: the rejection check now covers the 15 s timeout path, the drain uses WaitForExitAsync with a 3 s token, the enriched message is built once after the drain, DisplayTypeRejectedException propagates as the InvalidOperationException it already is, and the retry rebuilds the argument list instead of patching it by index.
Release notes state the CCFL LCD selector change on clean installs.
Collaborator
Author
|
Review findings fixed in 7b26140:
Not addressed: scoping the table per instrument when two meters are plugged in, and surfacing the resolved row in the setup UI. Both are follow-ups. |
SpotreadDisplayTypeTable.ForInstrument keeps only the rows whose instrument short name (Argyll inst_sname) matches the port descriptor of the opened meter (inst_name), plus the generic row, so a second connected meter's rows are not selected. ShortNameForDescriptor carries the name pairs from insttypes.c. ColorimeterService applies it at detection, on refresh, and SpotreadSession applies it to the table parsed from a rejection. Resolve now returns a SpotreadDisplayTypeChoice with the matched row. CalibrationSetupViewModel shows it under the correction picker (MeterCalibrationText) and passes meterUsesGenericCalibration to BuildPreflightMessages, which warns when no correction row exists for the type and no correction file is chosen.
Collaborator
Author
|
Follow-ups done in ab17f1b:
|
OemCorrectionImporter enumerates *.edr under the X-Rite, Calibrite and I-O DATA folders of every Program Files root and runs the bundled oeminst -v on them, which converts each EDR to a CCSS in Argyll's per-user data folder. ParseOutput reads the "Wrote '<path>'" lines and the exit code. Nothing is downloaded; oeminst's own search covers only one fixed X-Rite path, so the files are passed explicitly. CalibrationSetupViewModel.ImportOemCorrectionsAsync runs it from a new "Import EDRs" button next to the correction picker, shows the result under it, reloads the correction list, and re-runs meter detection so the -y table gains the technology rows. The generic-calibration preflight warning names the button. Tests cover the vendor folder search, output parsing (CRT.ccss from Argyll's ref folder is not counted), the no-files and missing-oeminst paths.
Collaborator
Author
|
EDR import added in 820c28f: an Import EDRs button runs the bundled oeminst on the i1Display EDR files of an installed i1Profiler or Calibrite PROFILER, then re-detects the meter. That's what puts the OLED/WLED/RGB LED rows into the -y table on a clean install. Verified end to end here through oeminst into %APPDATA%\ArgyllCMS and back out of spotread -?. |
ColorimeterService.InitializeAsync calls OemCorrectionImporter.EnsureImportedAsync before spotread -?: it lists the EDR files under the vendor folders, drops those whose converted CCSS already exists in Argyll's per-user or system data folder, and runs oeminst on the rest. Nothing to do on machines without X-Rite/Calibrite software, and a one-time cost of about a second on machines with it. Failures are logged and never block detection. The Import EDRs button, its command and status text are removed; the preflight warning no longer names it.
Collaborator
Author
|
5aea0e9: the EDR import now runs silently inside meter detection when an installed i1Profiler/Calibrite PROFILER has EDR files that Argyll doesn't have yet. Button removed. |
…review fixes StartProcessAsync drained output only after seeing the rejection flag, so when Process.Exited faulted the ready task before the stderr callback delivered the diagnostic, the rejection was missed and no retry ran. It now drains whenever the process has exited, then checks the flag. The retry gets a second strategy: if the instrument's own table resolves to the rejected selector again (a merged two-meter table the descriptor could not scope), rows carrying that selector are dropped and it resolves once more. GenericSelector returns n or r instead of l or c. On an install with imported EDRs, l and c are reassigned to the CCFL and CRT corrections, so an unknown table would have measured an OLED through a CCFL correction. Drivers that only know l and c reject n and r and the retry resolves from their table. Resolve matches technology keywords against Argyll's technology name only (the text before the parenthesised descriptor), so a free-text descriptor cannot capture a display type. ParseDeviceListOutput no longer reports a bare serial port as a detected instrument, so a PC with a COM header and no meter shows "No colorimeter detected" and Refresh re-enumerates. The pre-session table probe runs once per service and shares the parse/scope/log block with detection. spotread -? and oeminst run through one BoundedProcess helper. EnsureImportedAsync tries each EDR at most once per process. Setup screen: an unknown table reads as "not read yet" and does not warn; CRT, plasma and projector use their display names; a .ccmx is described as applied on top of the -y row. The calibration start log records the meter calibration line.
Collaborator
Author
|
Second review, fixed in e6357f2: drain-before-check race in the retry, base n/r selector for unknown tables (l/c get reassigned to CCFL/CRT corrections on EDR-rich installs), keyword match on Argyll's technology name only, second retry strategy for unscoped merged tables, bare COM ports no longer count as a detected meter, one-shot table probe, shared bounded process runner, oeminst tried once per EDR per process, setup text for unknown tables/CRT/CCMX. Not changed: the silent EDR import can shift which row a later calibration uses; the resolved row is now in the calibration start log and the setup screen. |
…table EnsureImportedAsync added files to the Attempted set before running oeminst, so a cancelled import (setup window closed mid-run) left them skipped for the rest of the process. They are now marked after the run completes and left pending on cancellation. The pre-session table probe likewise keeps its one retry when the caller cancelled it. RefreshMeterCalibrationText decided "table not read yet" from a null resolved row, which Resolve also returns for a known table with no usable row. It now checks the instrument table itself.
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Fixes #7. The
-ydisplay-type flag was a fixed letter per display type, but Argyll only lists the technology letters (o,e,b, ...) when X-Rite EDR corrections are imported. A clean i1D3 install has justn|l,r|cand1, so OLED, LCD LED and wide-gamut LCD all failed at spotread startup.The selector is now resolved against the table spotread prints for the connected instrument (parsed from the
-?output at detection): a matching technology row if installed, else the base row for the refresh class (OLED is non-refresh perdisptechs.c), elsel/c. If spotread still rejects it, the session parses the table from the rejection usage text and restarts once.Behaviour change on a clean i1D3: LCD CCFL now sends
-y 1(the built-in CCFL PVA row) instead of-y l. With EDR rows present every type resolves to the same letter as before.Checklist
dotnet test src/Gloam.sln -c Releasepasses: 1,388 tests.dotnet build src/Gloam.sln -c Release -p:GloamStrictBuild=trueis warning-free.src/Gloam.Corehave tests (reporter's clean-install table, an EDR-rich table, and a no-instrument table, resolved for every display type; rejection capture in the session).What was measured? Retry path exercised on an i1 Display Pro with Argyll 3.5.0 by forcing an invalid selector; the retry reached the ready prompt. No colorimetric comparison, the selector does not touch the measurement math.