[tylerkron]

User description

Summary

Previously SerialDeviceFinder just listed available serial ports without verifying if they were actually DAQiFi devices. Now it probes each port to identify genuine DAQiFi devices.

Changes

Port Probing Logic

• Opens serial port with DTR enabled
• Waits 1 second for device wake-up
• Sends initialization commands (DisableEcho, StopStreaming, TurnDeviceOn, SetProtobufStreamFormat)
• Sends GetDeviceInfo command with retry logic (up to 3 retries, 1 second apart)
• Waits up to 4 seconds for a protobuf status message response
• Extracts device info (serial number, firmware version, part number) from the response
• Returns null for ports that don't respond as DAQiFi devices
• Properly cleans up resources in all cases (port, producer, consumer)

Port Filtering

Adds FilterProbableDaqifiPorts() to improve discovery efficiency:

• Excludes debug, Bluetooth, and wlan ports (unlikely to be DAQiFi devices)
• On macOS, prefers /dev/cu.* over /dev/tty.* ports (cu is for outgoing connections)

Device Info Extraction

From the status message, extracts:

• Name: Device part number (e.g., "Nq1") or falls back to port name
• SerialNumber: Device serial number
• FirmwareVersion: Device firmware version
• Type: Detected via DeviceTypeDetector.DetectFromPartNumber()
• PortName: The serial port path
• ConnectionType: Serial

Test Plan

• All 7 existing SerialDeviceFinder tests pass
• All 414 unit tests pass
• Tested with physical Nq1 device on /dev/cu.usbmodem101:

  Discovered 1 DAQiFi device(s):
    - Nq1 (/dev/cu.usbmodem101) SN:9090539562006014104 FW:3.2.0
  

• Verified streaming still works after discovery

Closes #97

🤖 Generated with Claude Code

---

PR Type

Enhancement, Bug fix

---

Description

• Add serial port connection support to DaqifiDeviceFactory with ConnectSerialAsync and ConnectSerial methods

• Implement port probing in SerialDeviceFinder to identify genuine DAQiFi devices via SCPI commands

• Change default serial baud rate from 115200 to 9600 to match DAQiFi firmware initialization

• Add port filtering to exclude debug, Bluetooth, and wlan ports; prefer /dev/cu.* on macOS

---

Diagram Walkthrough

flowchart LR
  A["DaqifiDeviceFactory"] -->|"ConnectSerialAsync/Sync"| B["SerialStreamTransport"]
  C["SerialDeviceFinder"] -->|"FilterProbableDaqifiPorts"| D["Filtered Port List"]
  D -->|"TryGetDeviceInfoAsync"| E["Port Probing"]
  E -->|"Send SCPI Commands"| F["MessageProducer"]
  E -->|"Receive Protobuf Response"| G["StreamMessageConsumer"]
  G -->|"Extract Device Info"| H["DeviceInfo"]
  A -->|"ConnectFromDeviceInfoAsync"| I["ConnectSerialDeviceAsync"]
  I -->|"Uses PortName"| B

Loading

File Walkthrough

	Relevant files
Tests	DaqifiDeviceFactoryTests.cs Add serial connection factory tests                                            src/Daqifi.Core.Tests/Device/DaqifiDeviceFactoryTests.cs Add 10 new test cases for ConnectSerialAsync with null/empty/whitespace port names and invalid baud rates Add 3 new test cases for ConnectSerial sync methods with validation Update ConnectFromDeviceInfoAsync serial device tests to expect ArgumentException instead of NotSupportedException Add test for ConnectFromDeviceInfoAsync with cancellation token support +148/-8
Bug fix	SerialStreamTransport.cs Update default serial baud rate to 9600                                    src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs Change default baud rate parameter from 115200 to 9600 in constructor +1/-1
Enhancement	DaqifiDeviceFactory.cs Add serial connection factory methods                                        src/Daqifi.Core/Device/DaqifiDeviceFactory.cs Add SerialDefaults class with BaudRate constant set to 9600 Add ConnectSerialAsync overloads for default and custom baud rates with full validation Add ConnectSerial sync overloads for default and custom baud rates Implement ConnectSerialDeviceAsync private method to handle serial device connections from device info Update ConnectFromDeviceInfoAsync to support Serial connection type instead of throwing NotSupportedException Refactor ConnectWithTransportAsync to accept IStreamTransport instead of TcpStreamTransport for transport-agnostic logic +131/-4  SerialDeviceFinder.cs Implement serial port probing and device discovery              src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs Change DefaultBaudRate from 115200 to 9600 Implement comprehensive port probing logic with SCPI command initialization and protobuf response handling Add retry logic for GetDeviceInfo command with up to 3 retries at 1-second intervals Add FilterProbableDaqifiPorts method to exclude debug/Bluetooth/wlan ports and prefer /dev/cu.* on macOS Extract device information (serial number, firmware version, part number) from status message responses Add proper resource cleanup for serial port, producer, and consumer in finally block Add ConvertDeviceType helper method to map between Device and Discovery namespace enums +256/-39

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #97 🟢 Probe each available serial port (open briefly with short timeouts) instead of listing all ports blindly. Send the device-info SCPI command (SYSTem:SYSInfoPB? / ScpiMessageProducer.GetDeviceInfo) when probing. Wait for a protobuf status response and validate it (e.g., via ProtobufProtocolHandler.DetectMessageType()), only accepting DAQiFi devices. If a valid response is received, populate DeviceInfo with real values (serial number, firmware version, part number, etc.). If timeout/invalid response occurs, treat the port as not a DAQiFi device and return null / skip it. Close/cleanup the serial port and related resources reliably even on errors. Handle ports that are busy/in use by skipping them. ⚪ Ensure discovery works cross-platform (Windows COM ports, Linux /dev/tty*, macOS /dev/cu.*) and is performant via reasonable timeout tuning.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Misleading documentation: The XML documentation states the default serial baud rate is 115200 while the implementation uses 9600, making the code non-self-documenting and potentially misleading for callers. Referred Code /// <summary> /// Connects to a DAQiFi device over a serial port asynchronously using the default baud rate (115200). /// </summary> /// <param name="portName">The name of the serial port (e.g., "COM3", "/dev/ttyUSB0").</param> /// <param name="options">Optional connection options. If null, uses default options.</param> /// <param name="cancellationToken">A cancellation token to cancel the operation.</param> /// <returns>A connected and optionally initialized <see cref="DaqifiDevice"/>.</returns> /// <exception cref="ArgumentNullException">Thrown when portName is null or empty.</exception> /// <exception cref="OperationCanceledException">Thrown when the operation is cancelled.</exception> public static Task<DaqifiDevice> ConnectSerialAsync( string portName, DeviceConnectionOptions? options = null, CancellationToken cancellationToken = default) { return ConnectSerialAsync(portName, SerialDefaults.BaudRate, options, cancellationToken); } /// <summary> /// Connects to a DAQiFi device over a serial port asynchronously with a specified baud rate. /// </summary> /// <param name="portName">The name of the serial port (e.g., "COM3", "/dev/ttyUSB0").</param> ... (clipped 45 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Silent exception swallowing: The probing/discovery logic broadly catches and ignores exceptions (and returns null) without logging or surfacing actionable context, creating silent failures that hinder production debugging. Referred Code try { var deviceInfo = await TryGetDeviceInfoAsync(portName, cancellationToken); if (deviceInfo != null) { discoveredDevices.Add(deviceInfo); OnDeviceDiscovered(deviceInfo); } } catch (OperationCanceledException) { break; } catch (Exception) { // Skip ports that fail to open or respond // This is normal as not all serial ports are DAQiFi devices } } OnDiscoveryCompleted(); ... (clipped 149 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Consolidate device initialization into a helper Extract the complex, hardcoded SCPI command sequence for device initialization from SerialDeviceFinder.TryGetDeviceInfoAsync into a reusable helper method. This will prevent code duplication when implementing full device connections. Examples: src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs [199-236] // Initialize device - send commands to prepare for communication // These are required for the device to respond properly producer.Send(ScpiMessageProducer.DisableDeviceEcho); await Task.Delay(100, cancellationToken); producer.Send(ScpiMessageProducer.StopStreaming); await Task.Delay(100, cancellationToken); producer.Send(ScpiMessageProducer.TurnDeviceOn); await Task.Delay(100, cancellationToken); ... (clipped 28 lines) Solution Walkthrough: Before: // in SerialDeviceFinder.cs private async Task<IDeviceInfo?> TryGetDeviceInfoAsync(...) { // ... setup port, producer, consumer try { // ... // Initialize device - hardcoded sequence producer.Send(ScpiMessageProducer.DisableDeviceEcho); await Task.Delay(100, cancellationToken); producer.Send(ScpiMessageProducer.StopStreaming); await Task.Delay(100, cancellationToken); producer.Send(ScpiMessageProducer.TurnDeviceOn); await Task.Delay(100, cancellationToken); producer.Send(ScpiMessageProducer.SetProtobufStreamFormat); await Task.Delay(100, cancellationToken); // Send GetDeviceInfo with retry logic... // ... } finally { // ... cleanup } } After: // in a new helper class public static class DeviceProber { public static async Task<DaqifiOutMessage?> ProbeAsync(IStreamTransport transport, CancellationToken cancellationToken) { // ... setup producer/consumer from transport // Send initialization sequence producer.Send(ScpiMessageProducer.DisableDeviceEcho); // ... other init commands // Send GetDeviceInfo with retry logic and wait for response // ... return statusMessage; } } // in SerialDeviceFinder.cs private async Task<IDeviceInfo?> TryGetDeviceInfoAsync(...) { // ... setup SerialPort var statusMessage = await DeviceProber.ProbeAsync(port, cancellationToken); if (statusMessage == null) return null; return new DeviceInfo { /* ... from statusMessage ... */ }; } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that complex device initialization logic is hardcoded in TryGetDeviceInfoAsync, and this logic is highly likely to be duplicated for full device connections, making this a significant design improvement for reusability and maintainability.	Medium
Possible issue	Use using statements for resource management Refactor TryGetDeviceInfoAsync to use using statements for IDisposable resources like SerialPort, MessageProducer, and StreamMessageConsumer to ensure proper and automatic cleanup. src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs [150-308] private async Task<IDeviceInfo?> TryGetDeviceInfoAsync(string portName, CancellationToken cancellationToken) { - SerialPort? port = null; - MessageProducer<string>? producer = null; - StreamMessageConsumer<DaqifiOutMessage>? consumer = null; + using var port = new SerialPort(portName, _baudRate) + { + ReadTimeout = ProbeTimeoutMs, + WriteTimeout = ProbeTimeoutMs + }; try { - // Create and configure the serial port - port = new SerialPort(portName, _baudRate) + // Try to open the port + port.Open(); + port.DtrEnable = true; + + // Wait for device to wake up (devices need time after DTR is enabled) + await Task.Delay(DeviceWakeUpDelayMs, cancellationToken); + + // Set up message producer and consumer + var stream = port.BaseStream; + using var producer = new MessageProducer<string>(stream); + producer.Start(); + + var parser = new ProtobufMessageParser(); + using var consumer = new StreamMessageConsumer<DaqifiOutMessage>(stream, parser); + + // Track status message reception + DaqifiOutMessage? statusMessage = null; + var messageReceived = new TaskCompletionSource<bool>(); + + consumer.MessageReceived += (_, args) => { - ReadTimeout = ProbeTimeoutMs, - WriteTimeout = ProbeTimeoutMs + if (args.Message.Data is DaqifiOutMessage message) + { + var messageType = ProtobufProtocolHandler.DetectMessageType(message); + if (messageType == ProtobufMessageType.Status) + { + statusMessage = message; + messageReceived.TrySetResult(true); + } + } }; -... + + consumer.Start(); + + // ... (rest of the logic remains the same) + + // The 'finally' block for cleanup is no longer needed + // as 'using' statements will handle disposal. + // The port closing logic can be moved to the end of the try block + // or handled by the port's Dispose method. + // For explicit control: + if (port.IsOpen) + { + port.DtrEnable = false; + // A small delay might still be needed before closing if DTR signal change needs time. + await Task.Delay(50, CancellationToken.None); // Use CancellationToken.None for cleanup + } + return deviceInfo; } - finally + catch (UnauthorizedAccessException) { - // Clean up resources - try - { - consumer?.StopSafely(500); - consumer?.Dispose(); - } - catch - { - // Ignore cleanup errors - } - - try - { - producer?.StopSafely(500); - producer?.Dispose(); - } - catch - { - // Ignore cleanup errors - } - - try - { - if (port is { IsOpen: true }) - { - port.DtrEnable = false; - await Task.Delay(50); // Give DTR time to be processed - port.Close(); - } - - port?.Dispose(); - } - catch - { - // Ignore cleanup errors - } + // Port is in use by another application + return null; + } + catch (Exception) + { + // Any other error - not a valid DAQiFi device or port unavailable + return null; } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that manual resource management via try-finally is verbose and error-prone. Replacing it with using statements is idiomatic C# and improves code safety, readability, and maintainability by guaranteeing resource disposal.	Medium
General	Simplify complex retry and timeout logic Simplify the retry logic in TryGetDeviceInfoAsync by replacing the complex while loop, which manually tracks time, with a simpler for loop that uses a timed await for each attempt. src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs [213-236] // Send GetDeviceInfo command with retry logic - var timeout = DateTime.UtcNow.AddMilliseconds(ResponseTimeoutMs); - var lastRequestTime = DateTime.MinValue; - var retryCount = 0; + for (var i = 0; i < MaxRetries && statusMessage == null && !cancellationToken.IsCancellationRequested; i++) + { + producer.Send(ScpiMessageProducer.GetDeviceInfo); - while (statusMessage == null && DateTime.UtcNow < timeout && !cancellationToken.IsCancellationRequested) - { - // Send request every RetryIntervalMs, up to MaxRetries times - if ((DateTime.UtcNow - lastRequestTime).TotalMilliseconds >= RetryIntervalMs && retryCount < MaxRetries) + try { - producer.Send(ScpiMessageProducer.GetDeviceInfo); - lastRequestTime = DateTime.UtcNow; - retryCount++; + // Wait for the response or a timeout + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeoutCts.CancelAfter(RetryIntervalMs); + + await Task.WhenAny(messageReceived.Task, Task.Delay(RetryIntervalMs, timeoutCts.Token)); } - - // Wait a bit for response - var remainingTime = Math.Min(PollIntervalMs, (int)(timeout - DateTime.UtcNow).TotalMilliseconds); - if (remainingTime > 0) + catch (OperationCanceledException) { - await Task.WhenAny( - messageReceived.Task, - Task.Delay(remainingTime, cancellationToken)); + // This could be from the overall cancellationToken or our retry timeout. + // If it's the overall token, the loop condition will handle it. + // If it's the retry timeout, we'll just continue to the next retry iteration. } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that the retry logic is complex. The proposed refactoring using a for loop and a timed await simplifies the code, making it more readable and maintainable without changing the core functionality.	Low
	✅ Use reliable OS detection Suggestion Impact: Updated the OS detection logic to use RuntimeInformation.IsOSPlatform for OSX/Linux and added the required System.Runtime.InteropServices using directive; also adjusted the related comment to mention Linux. code diff: +using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; using Daqifi.Core.Communication.Consumers; @@ -327,10 +328,10 @@ .Where(port => !excludePatterns.Any(pattern => port.Contains(pattern, StringComparison.OrdinalIgnoreCase))); - // On macOS, prefer /dev/cu.* ports (for outgoing connections) over /dev/tty.* ports + // On macOS/Linux, prefer /dev/cu.* ports (for outgoing connections) over /dev/tty.* ports // If we have both cu and tty versions of the same port, only use cu - if (Environment.OSVersion.Platform == PlatformID.Unix || - Environment.OSVersion.Platform == PlatformID.MacOSX) + if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX) || + RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { Replace the usage of Environment.OSVersion.Platform with the more reliable RuntimeInformation.IsOSPlatform for detecting macOS and Linux platforms. src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs [332-334] -if (Environment.OSVersion.Platform == PlatformID.Unix || - Environment.OSVersion.Platform == PlatformID.MacOSX) +if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX) || + RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion is correct; RuntimeInformation.IsOSPlatform is the modern and recommended way to check the operating system, as Environment.OSVersion can be unreliable. This change improves the robustness and future-proofing of the platform-specific logic.	Low
	✅ Use correct exception for invalid arguments Suggestion Impact: The commit added an explicit null check that throws ArgumentNullException, and changed the whitespace/empty check to throw ArgumentException instead of ArgumentNullException, aligning with the suggested .NET best practice (message text differs slightly). code diff: + if (portName == null) + { + throw new ArgumentNullException(nameof(portName)); + } + if (string.IsNullOrWhiteSpace(portName)) { - throw new ArgumentNullException(nameof(portName), "Port name cannot be null or empty."); + throw new ArgumentException("Port name cannot be empty or whitespace.", nameof(portName)); } In ConnectSerialAsync, throw ArgumentException for empty or whitespace portName values, reserving ArgumentNullException strictly for null values to align with .NET best practices. src/Daqifi.Core/Device/DaqifiDeviceFactory.cs [156-159] + if (portName == null) + { + throw new ArgumentNullException(nameof(portName)); + } if (string.IsNullOrWhiteSpace(portName)) { - throw new ArgumentNullException(nameof(portName), "Port name cannot be null or empty."); + throw new ArgumentException("Port name cannot be empty or consist only of white-space characters.", nameof(portName)); } [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a deviation from .NET best practices. Using the more specific ArgumentException for non-null invalid arguments improves the API's correctness and makes debugging easier for consumers of the library.	Low
	Assert DTR before opening In TryGetDeviceInfoAsync, move the DtrEnable = true assignment into the SerialPort object initializer for better code organization. src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs [159-167] port = new SerialPort(portName, _baudRate) { ReadTimeout = ProbeTimeoutMs, - WriteTimeout = ProbeTimeoutMs + WriteTimeout = ProbeTimeoutMs, + DtrEnable = true }; // Try to open the port port.Open(); -port.DtrEnable = true; Apply / Chat Suggestion importance[1-10]: 3 __ Why: This is a minor improvement that makes the SerialPort initialization more concise by moving a property assignment into the object initializer. While it may have a subtle effect on timing, the primary benefit is improved code style.	Low
Update