Resolve spotread -y selector from the instrument table (1.9.2)

docs/release-notes/v1.9.2.md

@@ -0,0 +1,12 @@
+# Gloam 1.9.2
+
+Calibration no longer fails at instrument startup with "Failed to locate display type matching 'o'" (issue #7).
+
+- The spotread display-type selector (`-y`) is now taken from the table spotread prints for the connected instrument instead of a fixed letter per display type. ArgyllCMS only lists the technology letters (`o` OLED, `e` white LED, `b` RGB LED, `m` plasma, `p` projector) when matching X-Rite EDR corrections have been imported, which a fresh install never has. On a clean install the i1 Display Pro family accepts only `n|l` (Non-Refresh), `r|c` (Refresh) and a built-in CCFL row, so OLED, LCD LED and wide-gamut LCD all failed there.
+- OLED, LCD LED and wide-gamut LCD now resolve to the instrument's Non-Refresh base calibration when no matching correction is installed; CRT, plasma and projector resolve to the Refresh base. When a matching EDR row exists, the same letters as before are used. One measured-output change: on a clean i1D3, CCFL LCD now uses the built-in "LCD CCFL PVA" spectral row (`-y 1`) instead of the Non-Refresh base it reached through `-y l` before, because it is the only CCFL row listed. If you calibrated a CCFL display with 1.9.1, re-run the calibration. When a `.ccss` meter correction is selected, it continues to supply both the spectral calibration and the refresh mode, as spotread already did.
+- If spotread still rejects the selector, Gloam reads the table spotread prints on the way out and restarts once with a selector from that table. The failure message now lists the selectors the instrument accepts.
+- With two meters connected, only the rows spotread printed for the meter Gloam opened are considered.
+- The calibration setup screen now shows which calibration row the meter will use for the selected display type, and warns when the meter has no correction for that type and no CCSS/CCMX file is chosen.
+- If X-Rite i1Profiler or Calibrite PROFILER is installed, Gloam now converts their i1Display EDR calibration files into ArgyllCMS corrections with Argyll's oeminst the first time it looks for the meter. This is what gives the meter its OLED, white LED and RGB LED calibration rows. Nothing is downloaded and nothing happens on PCs without that software.
+
+Validation: all 1,388 tests pass and the strict Release build has zero warnings or errors. The rejection and retry path was exercised against an i1 Display Pro on Argyll 3.5.0 by forcing an invalid selector; the retry restarted with a listed selector and reached the ready prompt. The reporter's clean-install table and a table with imported EDR rows are pinned as test fixtures.

src/Gloam.Core/Calibration/BoundedProcess.cs

@@ -0,0 +1,74 @@
+using System;
+using System.Diagnostics;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Gloam.Core.Calibration
+{
+    /// <summary>Result of <see cref="BoundedProcess.RunAsync"/>.</summary>
+    public sealed class BoundedProcessResult
+    {
+        public BoundedProcessResult(int exitCode, string stdout, string stderr)
+        {
+            ExitCode = exitCode;
+            Stdout = stdout;
+            Stderr = stderr;
+        }
+
+        public int ExitCode { get; }
+        public string Stdout { get; }
+        public string Stderr { get; }
+
+        /// <summary>stdout followed by stderr, the shape the Argyll output parsers expect.</summary>
+        public string Combined => Stdout + "\n" + Stderr;
+    }
+
+    /// <summary>
+    /// Runs a console tool to completion with both streams captured and a hard time limit.
+    /// Shared by the Argyll one-shot invocations (spotread -?, oeminst).
+    /// </summary>
+    public static class BoundedProcess
+    {
+        /// <summary>
+        /// Starts <paramref name="psi"/> and waits for exit. Throws
+        /// <see cref="OperationCanceledException"/> after killing the process tree when
+        /// <paramref name="timeout"/> elapses or <paramref name="cancellationToken"/> fires.
+        /// </summary>
+        public static async Task<BoundedProcessResult> RunAsync(
+            ProcessStartInfo psi, TimeSpan timeout, CancellationToken cancellationToken)
+        {
+            psi.UseShellExecute = false;
+            psi.CreateNoWindow = true;
+            psi.RedirectStandardOutput = true;
+            psi.RedirectStandardError = true;
+
+            using var process = Process.Start(psi)
+                ?? throw new InvalidOperationException($"{psi.FileName} did not start.");
+            using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+            cts.CancelAfter(timeout);
+
+            var stdout = process.StandardOutput.ReadToEndAsync(cts.Token);
+            var stderr = process.StandardError.ReadToEndAsync(cts.Token);
+            try
+            {
+                await process.WaitForExitAsync(cts.Token);
+                return new BoundedProcessResult(process.ExitCode, await stdout, await stderr);
+            }
+            catch (OperationCanceledException)
+            {
+                try
+                {
+                    process.Kill(entireProcessTree: true);
+                }
+                catch (Exception ex)
+                {
+                    Log.DebugRateLimited(
+                        "bounded-process-termination",
+                        $"Could not terminate the timed-out process {psi.FileName}: {ex.Message}",
+                        TimeSpan.FromMinutes(10));
+                }
+                throw;
+            }
+        }
+    }
+}

src/Gloam.Core/Calibration/ColorimeterService.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.Globalization;
 using System.IO;
@@ -190,6 +191,12 @@ public async Task<bool> InitializeAsync(CancellationToken cancellationToken = de
 
                 RaiseStatusChanged(ColorimeterStatus.Searching, "Searching for colorimeter...");
 
+                // Convert any i1Display EDR files from installed X-Rite/Calibrite software that
+                // Argyll does not have yet, so detection below sees the technology rows.
+                // No-op after the first run and on machines without that software.
+                string argyllBin = Path.GetDirectoryName(_spotreadPath) ?? _argyllBinPath;
+                await OemCorrectionImporter.EnsureImportedAsync(argyllBin, Log, cancellationToken);
+
                 // Detect connected colorimeter
                 _connectedColorimeter = await DetectColorimeterAsync(cancellationToken);
 
@@ -248,9 +255,19 @@ public void SetDisplayIndex(int index)
         public void SetDisplayType(DisplayType type)
         {
             _displayType = type;
-            Log($"Display type set to: {type} (flag: -{type.ToSpotreadFlag()})");
+            var choice = ResolveDisplayTypeChoice(type);
+            Log($"Display type set to: {type} (spotread -y {choice.Selector}: {choice.Reason})");
         }
 
+        /// <summary>
+        /// How the connected instrument will measure <paramref name="type"/>: the selector
+        /// spotread gets and the table row it comes from. Uses the table captured at
+        /// detection (or adopted from a session), so it is exact for colorimeters with an
+        /// enumerated table and a best guess otherwise.
+        /// </summary>
+        public SpotreadDisplayTypeChoice ResolveDisplayTypeChoice(DisplayType type)
+            => SpotreadDisplayTypeTable.Resolve(type, _connectedColorimeter?.DisplayTypes);
+
         /// <summary>
         /// Gets the current display type setting.
         /// </summary>
@@ -348,9 +365,12 @@ public virtual async Task BeginMeasurementSessionAsync(bool hdrMode, Cancellatio
             Log($"Opening persistent spotread session (instrument {instrumentIndex}, HDR={hdrMode})");
             try
             {
+                await RefreshDisplayTypeTableIfUnknownAsync(cancellationToken);
                 _session = await SpotreadSession.StartAsync(
                     _spotreadPath, instrumentIndex, _displayType, hdrMode, Log, cancellationToken,
-                    _correctionFilePath);
+                    _correctionFilePath, displayTypeTable: _connectedColorimeter?.DisplayTypes,
+                    instrumentDescriptor: _connectedColorimeter?.InstrumentDescriptor);
+                AdoptDisplayTypeTable(_session.EffectiveDisplayTypeTable);
                 _sessionHdrMode = hdrMode;
                 RaiseStatusChanged(ColorimeterStatus.Ready, "Spotread session ready");
             }
@@ -367,6 +387,52 @@ public virtual async Task BeginMeasurementSessionAsync(bool hdrMode, Cancellatio
             }
         }
 
+        /// <summary>
+        /// Keeps the table a session actually resolved against (after a rejection retry it is
+        /// the one spotread printed for the opened instrument) so later sessions start right.
+        /// </summary>
+        private void AdoptDisplayTypeTable(IReadOnlyList<SpotreadDisplayTypeEntry> table)
+        {
+            if (_connectedColorimeter == null || !SpotreadDisplayTypeTable.HasInstrumentRows(table))
+                return;
+            if (ReferenceEquals(_connectedColorimeter.DisplayTypes, table))
+                return;
+            _connectedColorimeter.DisplayTypes = table;
+            Log($"spotread -y table updated from session: {SpotreadDisplayTypeTable.Describe(table)}");
+        }
+
+        // The usage text lists the -y selectors the enumerated instrument accepts. Argyll
+        // builds that table per instrument and per installed correction, so it is the only
+        // reliable source for the display-type flag (issue #7).
+        private static IReadOnlyList<SpotreadDisplayTypeEntry> ScopedDisplayTypeTable(string? usage, string? descriptor)
+        {
+            var table = SpotreadDisplayTypeTable.ForInstrument(SpotreadDisplayTypeTable.Parse(usage), descriptor);
+            Log($"spotread -y table: {SpotreadDisplayTypeTable.Describe(table)}");
+            return table;
+        }
+
+        private bool _displayTypeProbeRetried;
+
+        /// <summary>
+        /// Detection only sees the -y table when the instrument is enumerated at that moment
+        /// (the meter may be plugged in after the app starts). If the cached table has no
+        /// instrument rows, re-run the usage probe once per service before opening a session.
+        /// </summary>
+        private async Task RefreshDisplayTypeTableIfUnknownAsync(CancellationToken cancellationToken)
+        {
+            if (_connectedColorimeter == null || _displayTypeProbeRetried
+                || SpotreadDisplayTypeTable.HasInstrumentRows(_connectedColorimeter.DisplayTypes))
+                return;
+            var usage = await RunSpotreadCommandAsync(TimeSpan.FromSeconds(10), cancellationToken, "-?");
+            // An interrupted probe (caller cancelled) has not used the one retry.
+            _displayTypeProbeRetried = !cancellationToken.IsCancellationRequested;
+            var table = ScopedDisplayTypeTable(usage, _connectedColorimeter.InstrumentDescriptor);
+            if (SpotreadDisplayTypeTable.HasInstrumentRows(table))
+                _connectedColorimeter.DisplayTypes = table;
+            else
+                Log("spotread -y table still unknown (no instrument enumerated by -?); using the base selector and the rejection retry");
+        }
+
         /// <summary>
         /// Closes the persistent spotread session. Safe to call if no session is active.
         /// </summary>
@@ -540,9 +606,12 @@ private async Task<CieXyz> TakeMeasurementAsync(bool hdrMode, CancellationToken
             SpotreadSession transient;
             try
             {
+                await RefreshDisplayTypeTableIfUnknownAsync(cancellationToken);
                 transient = await SpotreadSession.StartAsync(
                     _spotreadPath, instrumentIndex, _displayType, hdrMode, Log, cancellationToken,
-                    _correctionFilePath);
+                    _correctionFilePath, displayTypeTable: _connectedColorimeter?.DisplayTypes,
+                    instrumentDescriptor: _connectedColorimeter?.InstrumentDescriptor);
+                AdoptDisplayTypeTable(transient.EffectiveDisplayTypeTable);
             }
             catch (InvalidOperationException ex) when (UsbDriverHelper.IsDriverError(ex.Message))
             {
@@ -567,14 +636,17 @@ private async Task<CieXyz> TakeMeasurementAsync(bool hdrMode, CancellationToken
 
             // Use help output to get the instrument list (spotread doesn't support -l)
             var listResult = await RunSpotreadCommandAsync(TimeSpan.FromSeconds(10), cancellationToken, "-?");
+            IReadOnlyList<SpotreadDisplayTypeEntry> displayTypes = Array.Empty<SpotreadDisplayTypeEntry>();
             if (listResult != null)
             {
                 Log($"spotread -? output: {listResult}");
 
                 // Parse actual connected device from list
                 var deviceInfo = ParseDeviceListOutput(listResult);
+                displayTypes = ScopedDisplayTypeTable(listResult, deviceInfo?.InstrumentDescriptor);
                 if (deviceInfo != null)
                 {
+                    deviceInfo.DisplayTypes = displayTypes;
                     Log($"Detected device from -?: {deviceInfo.Model}");
                     return deviceInfo;
                 }
@@ -589,7 +661,8 @@ private async Task<CieXyz> TakeMeasurementAsync(bool hdrMode, CancellationToken
                 return new ColorimeterInfo
                 {
                     Model = "Colorimeter Detected",
-                    IsHdrCapable = true // Assume capable, actual capability tested during measurement
+                    IsHdrCapable = true, // Assume capable, actual capability tested during measurement
+                    DisplayTypes = displayTypes
                 };
             }
 
@@ -605,73 +678,32 @@ private async Task<CieXyz> TakeMeasurementAsync(bool hdrMode, CancellationToken
             if (string.IsNullOrEmpty(_spotreadPath))
                 return null;
 
-            var psi = new ProcessStartInfo(_spotreadPath)
-            {
-                UseShellExecute = false,
-                CreateNoWindow = true,
-                RedirectStandardOutput = true,
-                RedirectStandardError = true
-            };
+            var psi = new ProcessStartInfo(_spotreadPath);
             foreach (string arg in args)
-            {
                 psi.ArgumentList.Add(arg);
-            }
 
             try
             {
-                using var process = Process.Start(psi);
-                if (process == null) return null;
-
-                using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
-                cts.CancelAfter(timeout);
-
-                var stdoutTask = process.StandardOutput.ReadToEndAsync(cts.Token);
-                var stderrTask = process.StandardError.ReadToEndAsync(cts.Token);
-
-                try
-                {
-                    await process.WaitForExitAsync(cts.Token);
-                }
-                catch (OperationCanceledException)
-                {
-                    try
-                    {
-                        process.Kill(entireProcessTree: true);
-                    }
-                    catch (Exception ex)
-                    {
-                        Gloam.Core.Log.DebugRateLimited(
-                            "colorimeter-process-termination",
-                            $"Could not terminate the cancelled instrument process: {ex.Message}",
-                            TimeSpan.FromMinutes(10));
-                    }
-                    return null;
-                }
-
-                string output, error;
-                try
-                {
-                    output = await stdoutTask;
-                    error = await stderrTask;
-                }
-                catch (OperationCanceledException)
-                {
-                    return null;
-                }
-
-                return output + "\n" + error;
+                var result = await BoundedProcess.RunAsync(psi, timeout, cancellationToken);
+                return result.Combined;
+            }
+            catch (OperationCanceledException)
+            {
+                return null;
             }
             catch (Exception ex)
             {
-                Log($"Error running spotread {args}: {ex.Message}");
+                Log($"Error running spotread {string.Join(" ", args)}: {ex.Message}");
                 return null;
             }
         }
 
         /// <summary>
         /// Parses device information from spotread output.
         /// </summary>
-        private static ColorimeterInfo? ParseDeviceListOutput(string output)
+        private static readonly Regex BareSerialPortPattern = new(@"^(COM\d+|/dev/tty\S*)$", RegexOptions.Compiled | RegexOptions.IgnoreCase);
+
+        internal static ColorimeterInfo? ParseDeviceListOutput(string output)
         {
             // Common colorimeter patterns in ArgyllCMS output
             var devicePatterns = new[]
@@ -716,9 +748,13 @@ private async Task<CieXyz> TakeMeasurementAsync(bool hdrMode, CancellationToken
                 }
             }
 
-            if (listMatches.Count > 0)
+            // A bare serial port ("COM6") is what every PC with a COM header lists; it is not
+            // an instrument. Argyll cannot name a serial instrument without opening it.
+            foreach (Match entry in listMatches)
             {
-                var firstEntry = listMatches[0];
+                var firstEntry = entry;
+                if (BareSerialPortPattern.IsMatch(firstEntry.Groups[2].Value.Trim()))
+                    continue;
                 if (int.TryParse(firstEntry.Groups[1].Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out int index))
                 {
                     string descriptor = firstEntry.Groups[2].Value;
@@ -848,6 +884,13 @@ public class ColorimeterInfo
         /// </summary>
         public string? InstrumentDescriptor { get; init; }
 
+        /// <summary>
+        /// The <c>-y</c> display-type table spotread printed for the enumerated instrument(s)
+        /// at detection time. Empty when no instrument was enumerated. Settable because it is
+        /// attached after the descriptor parse that constructs this object.
+        /// </summary>
+        public IReadOnlyList<SpotreadDisplayTypeEntry> DisplayTypes { get; set; } = Array.Empty<SpotreadDisplayTypeEntry>();
+
         /// <summary>
         /// Whether this colorimeter supports HDR measurement modes.
         /// </summary>