feat(device): route DaqifiDevice diagnostics through an optional ILogger (part of #340)

[tylerkron]

Summary

DaqifiDevice emitted all its diagnostics — including the bad-calibration / missing-resolution warnings that mean systematically wrong scaled samples — via Trace.WriteLine, which the real consumers (on Microsoft.Extensions.Logging) never see. It now accepts an optional ILogger (default NullLogger), reachable via DeviceConnectionOptions.Logger through the factory, and every Trace.WriteLine site routes through it. Non-breaking — the logger constructor params are optional.

Changes

• DaqifiDevice / DaqifiStreamingDevice constructors accept ILogger? logger = null (→ NullLogger.Instance); DeviceConnectionOptions.Logger threads it through DaqifiDeviceFactory.
• All 12 Trace.WriteLine sites in DaqifiDevice converted to ILogger message templates (no interpolation — TreatWarningsAsErrors/CA2254 clean):
  • Warning: the three PopulateAnalogChannels calibration/resolution anomalies ("scaled samples may be systematically wrong/affected"), drain-queue-not-converged, and classified-event subscriber exceptions.
  • Debug: ExecuteTextCommandAsync timing chatter and drain-queue empty-reply termination.
• RaiseClassifiedEvent is now an instance method so it can log via _logger.

Testing

• dotnet test — Core 1639 pass / 0 fail / 2 skipped, MCP 23 pass (net9.0 + net10.0).
• 4 unit tests: a zero-resolution status logs a Warning (rendered from the template, incl. the device name) through the injected logger; a valid status logs no warning; a device with no logger falls back to NullLogger and doesn't throw on the warning path; DeviceConnectionOptions.Logger defaults null.
• No bench validation: this routes existing diagnostics to a logger — an observability change with no device-facing behavior to exercise.

Scope note

Covers acceptance criteria 1–2 of #340 (central DaqifiDevice + factory reachability). The remaining items — optional loggers on the finders/transports and a NullLogger default for FirmwareUpdateService — are left for follow-up PRs. Does not close #340. Also note: Trace.WriteLine output is no longer emitted for non-opted-in consumers (they were not reliably consuming it — the feature's premise), diagnostics now flow to ILogger.

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Route DaqifiDevice diagnostics through optional ILogger

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add optional ILogger plumbing from connection options through factory into DaqifiDevice.
• Replace Trace.WriteLine diagnostics with structured ILogger templates and severity levels.
• Add unit tests validating warnings are emitted (or safely ignored via NullLogger).

Diagram

graph TD
  caller["Library consumer"] --> options["DeviceConnectionOptions.Logger"] --> factory["DaqifiDeviceFactory"] --> device["DaqifiStreamingDevice / DaqifiDevice"] --> logger["ILogger (or NullLogger)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep Trace/TraceSource and add a listener bridge

• ➕ Preserves existing Trace output behavior for legacy consumers
• ➕ Can route Trace to ILogger centrally via a listener adapter
• ➖ Still encourages string-based logging instead of structured templates
• ➖ Listener configuration is often global/process-wide and harder to reason about per-device

2. Expose DiagnosticSource/Activity for device events

• ➕ Richer observability semantics (spans/events) and tooling integration
• ➕ Can coexist with logging and support high-cardinality diagnostics safely
• ➖ More architectural weight than needed for converting existing warnings/debug
• ➖ Would require defining event schema and consumer guidance

3. Use an ILoggerFactory on options instead of ILogger

• ➕ Supports category-based loggers and consistent naming
• ➕ Avoids sharing a single logger instance across multiple devices
• ➖ Bigger API surface change and more plumbing
• ➖ Not necessary if the caller already supplies an appropriately scoped ILogger

Recommendation: The PR’s approach (optional ILogger with NullLogger fallback, threaded via DeviceConnectionOptions and the factory) is the best fit for the stated goal: it is non-breaking, keeps diagnostics local to the device instance, and upgrades Trace.WriteLine messages to structured templates with appropriate levels. Consider ILoggerFactory only if category/scoping becomes important across many devices.

Files changed (5) +131 / -21

Enhancement (4) +50 / -21

DaqifiDevice.csInject optional ILogger and replace Trace.WriteLine with structured logs +35/-18

Inject optional ILogger and replace Trace.WriteLine with structured logs

• Adds an optional ILogger parameter to constructors and stores a non-null logger via NullLogger.Instance fallback. Converts device diagnostics (SCPI timing, drain-queue behavior, calibration/resolution anomalies, subscriber exceptions) from Trace.WriteLine to LogDebug/LogWarning with message templates, and makes RaiseClassifiedEvent an instance method to log via _logger.

src/Daqifi.Core/Device/DaqifiDevice.cs

DaqifiDeviceFactory.csThread DeviceConnectionOptions.Logger into device construction +1/-1

Thread DeviceConnectionOptions.Logger into device construction

• Updates the transport connection path to pass options.Logger into DaqifiStreamingDevice so created devices emit diagnostics through the provided ILogger.

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs

DaqifiStreamingDevice.csAdd optional ILogger parameters to streaming device constructors +7/-2

Add optional ILogger parameters to streaming device constructors

• Extends DaqifiStreamingDevice constructors to accept an optional ILogger and forwards it to the base DaqifiDevice constructors, preserving backwards compatibility via default null.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

DeviceConnectionOptions.csAdd Logger option to configure device diagnostics sink +7/-0

Add Logger option to configure device diagnostics sink

• Adds a nullable ILogger property to DeviceConnectionOptions to allow consumers to provide a logging sink for device diagnostics; documents that a no-op logger is used when null.

src/Daqifi.Core/Device/DeviceConnectionOptions.cs

Tests (1) +81 / -0

DaqifiDeviceLoggerTests.csAdd tests for DaqifiDevice ILogger warnings and defaults +81/-0

Add tests for DaqifiDevice ILogger warnings and defaults

• Introduces unit tests that verify PopulateChannels emits a Warning when resolution is unusable, emits no warning for valid status, and does not throw when no logger is provided (NullLogger fallback). Also asserts DeviceConnectionOptions.Logger defaults to null.

src/Daqifi.Core.Tests/Device/DaqifiDeviceLoggerTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 feat: consistent ILogger seam — device diagnostics currently go to Trace.WriteLine

1. Logger lost in factory ✓ Resolved 🐞 Bug ≡ Correctness

Description

ConnectFromDeviceInfoAsync rebuilds DeviceConnectionOptions (modifiedOptions) but does not copy
Logger, so ConnectWithTransportAsync passes a null Logger and the device silently falls back to
NullLogger.Instance. This makes injected logging via the device-discovery factory paths ineffective
(warnings/debug never reach the consumer pipeline).

Code

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[407]

+            device = new DaqifiStreamingDevice(options.DeviceName, transport, options.Logger);

Relevance

⭐⭐⭐ High

Repo has history of fixing “lost option” copy/clone bugs (e.g., dropped capability flag fixed in PR
#348).

PR-#348

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The factory’s device-info paths create a new DeviceConnectionOptions instance but omit Logger, so
the later construction site that now passes options.Logger always receives null on those paths,
despite Logger being a supported option.

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[309-334]
src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[351-371]
src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[389-410]
src/Daqifi.Core/Device/DeviceConnectionOptions.cs[40-45]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DeviceConnectionOptions.Logger` is dropped when the factory creates `modifiedOptions` inside `ConnectWiFiDeviceAsync` and `ConnectSerialDeviceAsync`. As a result, `ConnectWithTransportAsync` receives an options instance whose `Logger` is null and constructs `DaqifiStreamingDevice` with a null logger (which then becomes `NullLogger.Instance`).

### Issue Context
The PR introduces `DeviceConnectionOptions.Logger` and threads it into `ConnectWithTransportAsync` via the new `DaqifiStreamingDevice(..., options.Logger)` call. However, the device-info connection helpers currently copy only a subset of options.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[309-334]
- src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[351-371]
- src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[389-410]
- src/Daqifi.Core/Device/DeviceConnectionOptions.cs[40-45]

### Suggested change
Add `Logger = effectiveOptions.Logger` to both `modifiedOptions` initializers (WiFi + Serial device-info paths) so the injected logger survives to `ConnectWithTransportAsync`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Logger can rethrow ✓ Resolved 🐞 Bug ☼ Reliability

Description

RaiseClassifiedEvent catches subscriber exceptions but then calls _logger.LogWarning inside the
catch without guarding against logger failures; if the logger throws, the exception escapes and the
frame can still skip MessageReceived/downstream processing. This defeats the method’s core purpose
of isolating message-processing from misbehaving subscribers.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[1251]

+                _logger.LogWarning(ex, "[{EventName}] classified-event subscriber threw", eventName);

Relevance

⭐⭐⭐ High

Team previously accepted guarding ILogger calls to prevent logger exceptions killing core loops (PR
#260).

PR-#260

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The call order shows RaiseClassifiedEvent runs before OnMessageReceived; an exception escaping
RaiseClassifiedEvent would prevent MessageReceived from firing for that frame. The repo has
prior precedent for wrapping logger calls to keep background/control-flow threads alive if logging
fails.

src/Daqifi.Core/Device/DaqifiDevice.cs[1206-1224]
src/Daqifi.Core/Device/DaqifiDevice.cs[1238-1253]
PR-#260

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RaiseClassifiedEvent` is intended to prevent a classified-event subscriber exception from stopping further processing for that frame (e.g., firing `MessageReceived`). However, the new `_logger.LogWarning(...)` in the catch block is not protected; if a provided `ILogger` implementation throws, the exception will escape and the protection fails.

### Issue Context
`OnStatusMessageReceived`/`OnStreamMessageReceived` invoke `RaiseClassifiedEvent(...)` before continuing with `OnMessageReceived(...)`. The code comment explicitly states an uncaught exception here would drop the frame for other consumers.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1206-1224]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1238-1253]

### Suggested change
Wrap the `_logger.LogWarning(...)` call in its own try/catch (or introduce a small `SafeLog(Action)` helper used by all device logging sites) so `RaiseClassifiedEvent` cannot throw due to logger failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[tylerkron]

Both fixed in the latest commit:

1. Logger lost in factory — ConnectFromDeviceInfoAsync's WiFi/serial paths now copy Logger = effectiveOptions.Logger into the rebuilt DeviceConnectionOptions, so injected logging reaches the device on the discovery paths too.
2. Logger can rethrow — all 12 DaqifiDevice logger calls are wrapped in a new SafeLog (mirrors MessageProducer.SafeLog), so a throwing ILogger can't escape — critically in RaiseClassifiedEvent's catch. Added a throwing-logger regression test. Full suite 1640 pass.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 52abfc1

[tylerkron]

Qodo re-review clean (0 unresolved), CI green, full suite 1640 pass. Ready for review.

[qodo-code-review]

re-review clean (0 unresolved), CI green, full suite 1640 pass. Ready for review.

The PR is clean from Qodo’s perspective: no unresolved findings remain, and the reported CI status is green with all 1,640 tests passing. It’s ready for review.