feat(discovery): resolve TCP data port for manually-entered IPs (closes #244)

[tylerkron]

Summary

A host that never answered the UDP-30303 broadcast (a manually-entered IP) had no data port to connect with, forcing consumers like daqifi-desktop to hardcode 9760. This gives Core both a documented default and a way to resolve the real port.

Closes #244.

Changes

• DaqifiDeviceFactory.DefaultTcpDataPort (9760) — the firmware-default TCP data port, so consumers reference one documented constant instead of a magic literal for manual connections.
• WiFiDeviceFinder.ProbeTcpDataPortAsync(host, [discoveryPort,] timeout, ct) — unicasts the discovery query directly to a single host and returns the DevicePort from its reply, or null if it does not answer within the timeout (callers then fall back to DefaultTcpDataPort). It reuses the finder's existing query constant and ParseDeviceInfo/IsValidDiscoveryMessage rather than duplicating discovery parsing, ignores replies from other devices on the segment, maps its own timeout to null, and rethrows only genuine caller cancellation.

Together these implement both options the ticket proposed (a documented default and an active probe).

Tests

• Argument validation (null host, non-positive timeout, out-of-range discovery port).
• A loopback responder that answers with a delimited status protobuf, asserting the advertised port is returned end-to-end.
• A silent host → probe times out and returns null.
• Caller cancellation propagates an OperationCanceledException.
• Full suite green (1550 passing).

Bench validation

The bench rig is USB-serial, so the probe's UDP round-trip was exercised via the loopback integration test above rather than a WiFi device. The pieces that touch real hardware were confirmed against a live Nyquist (firmware 3.7.2): the device reports DevicePort = 9760 in its status frame — the same field ProbeTcpDataPortAsync reads — validating both DefaultTcpDataPort and the value the probe resolves.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Resolve TCP data port for manually-entered device IPs via unicast probe

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add a documented firmware-default TCP data port constant for manual connections.
• Add a unicast UDP discovery probe to resolve a specific host’s advertised TCP port.
• Add integration-style UDP loopback tests covering timeouts, validation, and cancellation.

Diagram

graph TD
  A["Client (manual IP)"] --> B["WiFiDeviceFinder.ProbeTcpDataPortAsync"] --> C["UDP unicast query (30303)"] --> D["Reply + discovery parse"] --> E{"Port resolved?"}
  E -->|"yes"| F["Use resolved port"] --> H["TCP stream connect"]
  E -->|"no"| G["DefaultTcpDataPort (9760)"] --> H

Loading

High-Level Assessment

The chosen approach (unicast UDP discovery probe + documented default port) is the best fit: it reuses existing discovery framing/parsing, avoids port-scanning, and cleanly distinguishes caller cancellation from probe timeout. Alternatives considered: (1) only documenting a default port would perpetuate incorrect connections for non-default firmware configs; (2) trying multiple TCP ports (scan) increases latency and network noise; (3) introducing mDNS/SSDP would add a new discovery protocol surface area.

Files changed (3) +214 / -0

Enhancement (1) +13 / -0

DaqifiDeviceFactory.csDocument and expose firmware-default TCP data port constant (9760) +13/-0

Document and expose firmware-default TCP data port constant (9760)

• Introduces DaqifiDeviceFactory.DefaultTcpDataPort with XML documentation describing when to use discovery-reported ports vs probing vs default fallback. Centralizes the previously hardcoded 9760 value for consumers.

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs

Bug fix (1) +98 / -0

WiFiDeviceFinder.csAdd unicast UDP probe to resolve a host’s advertised TCP data port +98/-0

Add unicast UDP probe to resolve a host’s advertised TCP data port

• Adds ProbeTcpDataPortAsync overloads that unicast the existing discovery query to a specific host and return the parsed DevicePort. Implements validation, filters responses to the target host, maps internal timeout/unreachable to null, and preserves caller cancellation semantics.

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs

Tests (1) +103 / -0

WiFiDeviceFinderTests.csAdd tests for TCP data-port probing and default port constant +103/-0

Add tests for TCP data-port probing and default port constant

• Adds unit/integration-style tests for DefaultTcpDataPort and ProbeTcpDataPortAsync argument validation. Includes a loopback UDP responder that returns a delimited status protobuf with DevicePort, plus timeout-to-null and caller-cancellation propagation coverage.

src/Daqifi.Core.Tests/Device/Discovery/WiFiDeviceFinderTests.cs

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 feat: Resolve TCP data port for manually-entered IP addresses

1. Probe listens wrong UDP port ✓ Resolved 🐞 Bug ≡ Correctness

Description

ProbeTcpDataPortAsync does not bind its UDP socket to the configured discoveryPort, so the request
is sent from an ephemeral source port and the probe can miss devices that reply to the well-known
discovery port (30303) instead of the sender port.

Code

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[R167-172]

+        var query = Encoding.ASCII.GetBytes(DaqifiFinderQuery);
+        var target = new IPEndPoint(host, discoveryPort);
+
+        using var udp = new UdpClient(host.AddressFamily);
+        using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+        cts.CancelAfter(timeout);

Relevance

⭐⭐⭐ High

Team accepted binding UDP discovery to well-known port (30303) to avoid ephemeral-port reply loss
(PR #180).

PR-#180

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The probe creates an unbound UdpClient (ephemeral local port), while the existing discovery
implementation explicitly binds to the discovery port because devices may target the well-known port
for replies; this mismatch can make the probe miss legitimate responses.

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[167-213]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[257-277]

Agent prompt

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

### Issue description
`ProbeTcpDataPortAsync` creates an unbound `UdpClient`, which causes the OS to choose an ephemeral local UDP port. The project’s broadcast discovery path already documents that some devices reply to the *well-known discovery port*; in those cases, the probe will never see the reply and will incorrectly return `null`.

### Issue Context
The existing broadcast discovery logic binds to the discovery port (with a fallback) specifically to handle devices that reply to the well-known port. The probe should do the same, binding to `discoveryPort` (not hardcoding 30303) and using `ReuseAddress`/`ExclusiveAddressUse=false` as needed.

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[167-213]
- src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[245-277]

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

---

2. Probe returns out-of-range port ✓ Resolved 🐞 Bug ☼ Reliability

Description

ProbeTcpDataPortAsync returns any positive DevicePort value, including values above 65535, which
violates the method’s “usable port” contract and can prevent callers from falling back to
DefaultTcpDataPort because the result is non-null.

Code

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[R195-200]

+                var info = ParseDeviceInfo(result.Buffer, result.RemoteEndPoint, null);
+                if (info?.Port is int port and > 0)
+                {
+                    return port;
+                }
+                // Parsed but no usable port; keep waiting within the timeout window.

Relevance

⭐⭐⭐ High

Strict TCP port-range validation was restored/required in discovery connection path (PR #187);
similar validation feedback partially accepted (PR #94).

PR-#187
PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The probe’s check is only > 0, while elsewhere the codebase defines the valid TCP port range as
1..65535; a previous fix in this area was specifically to restore port validation before connecting.

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[189-202]
src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[373-384]
PR-#187

Agent prompt

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

### Issue description
`ProbeTcpDataPortAsync` currently returns `port` when `port > 0`. This admits invalid TCP ports (> 65535). Because callers are expected to do `resolved ?? DefaultTcpDataPort`, returning an invalid *non-null* value can bypass the intended fallback path.

### Issue Context
The factory’s connection paths validate port ranges (1..65535) before using them. The probe should enforce the same constraint and treat out-of-range values as “not usable” (continue waiting within timeout, or return `null`).

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[189-202]
- src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[373-384]

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

---