feat(firmware): make FirmwareUpdateService logger optional (NullLogger default) (part of #340)

[tylerkron]

Summary

FirmwareUpdateService required a non-nullable ILogger<FirmwareUpdateService>, so a DI-less consumer had to construct and pass one just to use the service. The logger parameter is now optional, defaulting to NullLogger<FirmwareUpdateService>.Instance — matching the optional-logger convention MessageProducer (and #360's DaqifiDevice) already use. Addresses acceptance-criterion 4 of #340.

Changes

• Constructor logger parameter: ILogger<FirmwareUpdateService> logger → ILogger<FirmwareUpdateService>? logger = null; the ?? throw ArgumentNullException fallback becomes ?? NullLogger<FirmwareUpdateService>.Instance. Non-breaking — the parameter already sits before the existing optional parameters, and callers passing a logger are unaffected. No flash-path logic changed.

Testing

• dotnet test — 1636 pass / 0 fail / 2 skipped (net9.0 + net10.0); FirmwareUpdateService suite 64/64.
• New test: constructing the service without a logger does not throw.
• No bench validation: a DI-ergonomics change with no device-facing behavior.

Scope note

Closes acceptance-criterion 4 of #340. The remaining item — optional loggers on the device finders and transports — is a separate follow-up. Does not close #340 (criteria 1–2 are #360, this is criterion 4).

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Make FirmwareUpdateService logger optional (NullLogger default)

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Make FirmwareUpdateService usable without DI by defaulting logger to NullLogger.
• Update constructor docs to describe the optional logger behavior.
• Add regression test verifying construction without a logger does not throw.

Diagram

graph TD
  C(["DI-less consumer"]) --> F["FirmwareUpdateService"] --> L{{"ILogger<FirmwareUpdateService>"}}
  F --> N["NullLogger.Instance"]
  T(["Unit tests"]) --> F

  subgraph Legend
    direction LR
    _user(["Caller/Test"]) ~~~ _svc["Service"] ~~~ _ext{{"Abstraction"}} ~~~ _impl["Default impl"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add an overload without ILogger (keep non-nullable primary ctor)

• ➕ Avoids nullable parameter in the main signature
• ➕ Keeps explicit requirement for DI-heavy callers
• ➖ Adds API surface area to maintain
• ➖ Still requires deciding which overload is canonical in docs/examples

2. Accept ILoggerFactory and create logger internally

• ➕ Avoids passing typed ILogger explicitly
• ➕ Aligns with patterns where services create their own loggers
• ➖ Still not DI-less unless a factory is created manually
• ➖ More complex change than needed for the stated acceptance criterion

Recommendation: Proceed with the PR’s approach (optional ILogger with NullLogger default). It is the smallest non-breaking API change, matches existing library convention, and keeps logging behavior explicit for callers who care while removing friction for DI-less consumers.

Files changed (2) +21 / -6

Enhancement (1) +9 / -6

FirmwareUpdateService.csMake logger optional and default to NullLogger +9/-6

Make logger optional and default to NullLogger

• Updates FirmwareUpdateService constructor to accept a nullable ILogger with a default null value, and falls back to NullLogger<FirmwareUpdateService>.Instance when omitted. Updates XML docs to explicitly describe the default no-op logger behavior.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs

Tests (1) +12 / -0

FirmwareUpdateServiceTests.csAdd regression test for construction without a logger +12/-0

Add regression test for construction without a logger

• Adds a new unit test asserting FirmwareUpdateService can be constructed without providing an ILogger. This guards the new optional-logger behavior and prevents regressions for DI-less consumers.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

[tylerkron]

Qodo review complete (0 unresolved), CI green, full suite 1636 pass. Ready for review.

[qodo-code-review]

review complete (0 unresolved), CI green, full suite 1636 pass. Ready for review.

The change is narrowly scoped: FirmwareUpdateService now safely defaults to NullLogger, with a regression test covering construction without an explicit logger. With CI green and the full suite passing, it looks ready for review.