Skip to content

fix: honor LocalInterfaceAddress when connecting WiFi devices#187

Merged
tylerkron merged 3 commits into
mainfrom
claude/crazy-torvalds-35cae9
May 14, 2026
Merged

fix: honor LocalInterfaceAddress when connecting WiFi devices#187
tylerkron merged 3 commits into
mainfrom
claude/crazy-torvalds-35cae9

Conversation

@tylerkron
Copy link
Copy Markdown
Contributor

Summary

  • TcpStreamTransport now takes an optional localInterface and, when supplied, binds the outbound socket to that address before connecting (multi-homed hosts can't otherwise control egress NIC).
  • DaqifiDeviceFactory.ConnectFromDeviceInfoAsync threads deviceInfo.LocalInterfaceAddress into the transport for the WiFi path; serial/HID paths and existing ConnectTcpAsync callers are unchanged.
  • Adds focused tests: transport-level bind verification (loopback succeeds, unassigned address fails with SocketException) plus a factory-level test that proves LocalInterfaceAddress is threaded through end-to-end.

Closes #147.

Test plan

  • dotnet build src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj — clean
  • dotnet test --filter "FullyQualifiedName~LocalInterface" — 8 passed on net9.0 and net10.0
  • Full suite: 895 passed / 0 failed / 2 skipped on both target frameworks

🤖 Generated with Claude Code

…evices

WiFiDeviceFinder already records the discovering NIC in
IDeviceInfo.LocalInterfaceAddress, but the TCP connect path ignored it
and let the OS routing table pick the egress interface. On multi-homed
hosts that meant a device discovered on one NIC could be unconnectable
because the follow-up TCP connection went out a different NIC.

TcpStreamTransport now accepts an optional localInterface and, when
supplied, binds the outbound socket to it before connecting.
DaqifiDeviceFactory.ConnectFromDeviceInfoAsync threads
deviceInfo.LocalInterfaceAddress through for the WiFi path. Behavior is
unchanged when no local interface is supplied.

Closes #147

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tylerkron tylerkron requested a review from a team as a code owner May 5, 2026 04:07
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Honor LocalInterfaceAddress in WiFi device TCP connections

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Honor LocalInterfaceAddress when connecting WiFi devices on multi-homed hosts
• TcpStreamTransport now accepts optional localInterface parameter for socket binding
• DaqifiDeviceFactory threads deviceInfo.LocalInterfaceAddress through WiFi connection path
• Added comprehensive tests validating local interface binding and error handling
Diagram
flowchart LR
  A["WiFi Device Discovery"] -->|LocalInterfaceAddress| B["DaqifiDeviceFactory"]
  B -->|threads through| C["TcpStreamTransport"]
  C -->|binds socket to| D["Specified NIC"]
  D -->|connects via| E["Correct Egress Interface"]
Loading

Grey Divider

File Changes

1. src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs ✨ Enhancement +23/-3

Add local interface binding support to TCP transport

• Added optional _localInterface field to store local interface address
• Updated both constructors to accept optional localInterface parameter with documentation
• Added LocalInterface property to expose the configured local interface
• Modified ConnectAsync to bind TcpClient socket to local interface when specified

src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs


2. src/Daqifi.Core/Device/DaqifiDeviceFactory.cs 🐞 Bug fix +7/-3

Thread LocalInterfaceAddress through WiFi connection path

• Modified ConnectWiFiDeviceAsync to create TcpStreamTransport with
 deviceInfo.LocalInterfaceAddress
• Changed from calling ConnectTcpAsync to directly instantiating transport and calling
 ConnectWithTransportAsync
• Added explanatory comment about multi-homed host egress interface selection

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs


3. src/Daqifi.Core.Tests/Communication/Transport/TcpStreamTransportTests.cs 🧪 Tests +59/-0

Add local interface binding validation tests

• Added test verifying constructor exposes LocalInterface when provided
• Added test verifying LocalInterface is null when not provided
• Added test confirming SocketException thrown when binding to unassigned address
• Added test validating successful connection with loopback local interface binding

src/Daqifi.Core.Tests/Communication/Transport/TcpStreamTransportTests.cs


View more (1)
4. src/Daqifi.Core.Tests/Device/DaqifiDeviceFactoryTests.cs 🧪 Tests +42/-0

Add factory-level LocalInterfaceAddress threading test

• Added System.Net.Sockets using statement for SocketException
• Added integration test verifying factory threads LocalInterfaceAddress to transport
• Test uses unassigned RFC 5737 TEST-NET-1 address to confirm binding occurs

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


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 5, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. WiFi port not validated ✓ Resolved 🐞 Bug ≡ Correctness
Description
ConnectWiFiDeviceAsync now constructs TcpStreamTransport with deviceInfo.Port.Value without
validating it is within 1..65535, which regresses the factory’s prior fail-fast behavior and can
turn invalid discovery ports (e.g., 0) into lower-level socket/connect failures.
Code

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[R311-314]

+        var transport = new TcpStreamTransport(
            deviceInfo.IPAddress,
            deviceInfo.Port.Value,
-            modifiedOptions,
-            cancellationToken).ConfigureAwait(false);
+            deviceInfo.LocalInterfaceAddress);
Evidence
The WiFi path only checks Port.HasValue and then uses Port.Value directly when creating the
transport, but the factory’s TCP helpers validate port range via ValidatePort(port). Discovery
populates DeviceInfo.Port straight from the protobuf DevicePort field, which can be 0/invalid, so
the missing validation is reachable from normal discovery data.

src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[275-317]
src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[35-88]
src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[356-367]
src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs[313-333]

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

## Issue description
`ConnectWiFiDeviceAsync(...)` uses `deviceInfo.Port.Value` without validating the TCP port range (1..65535). This regresses the previous behavior where the WiFi path flowed through `ConnectTcpAsync(...)`, which calls `ValidatePort(port)`.

## Issue Context
Ports come from discovery (`WiFiDeviceFinder.ParseDeviceInfo` sets `Port = (int)message.DevicePort`), so invalid values (notably `0`) are plausible and should fail fast with a consistent `ArgumentOutOfRangeException` like other factory TCP entry points.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[275-318]
- src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[356-367]

## Suggested change
Add `ValidatePort(deviceInfo.Port.Value);` immediately after the `Port.HasValue` check in `ConnectWiFiDeviceAsync` (before constructing `TcpStreamTransport`). Alternatively, introduce a `ConnectTcpAsync` overload that accepts `localInterface` and reuse the existing validation path.

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


Grey Divider

Qodo Logo

Comment thread src/Daqifi.Core/Device/DaqifiDeviceFactory.cs
tylerkron and others added 2 commits May 5, 2026 09:53
The earlier refactor of ConnectWiFiDeviceAsync to construct
TcpStreamTransport directly bypassed ConnectTcpAsync's ValidatePort
call. IPEndPoint validates 0-65535, but the original behavior rejected
port 0 with an explicit ArgumentOutOfRangeException named "port" — keep
that for parity.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tylerkron
Copy link
Copy Markdown
Contributor Author

Hardware test results

Tested end-to-end against a NYQUIST1 device on WiFi via the example app (patched on a branch to exercise the new code path — see daqifi-core-example-app#28).

Connection path

Discovering WiFi devices (timeout 15s)...
Discovered NYQUIST1 at 192.168.1.39:9760 (LocalInterfaceAddress=192.168.1.29)
Connected to NYQUIST1 at 192.168.1.39:9760 via discovery, bound to local interface 192.168.1.29
Streaming at 10 Hz...

Confirmed:

  1. WiFiDeviceFinder populated LocalInterfaceAddress = 192.168.1.29 (host's en0) on the discovered DeviceInfo.
  2. DaqifiDeviceFactory.ConnectFromDeviceInfoAsync threaded that into TcpStreamTransport's new localInterface parameter.
  3. The outbound socket connected with 192.168.1.29:0 as its local endpoint.
  4. Streamed 3 ADC channels (--channels 7) at 10 Hz; received 100+ samples; --min-samples 10 satisfied; exit 0; clean disconnect.

Unit tests

dotnet test --filter "FullyQualifiedName~LocalInterface" on this branch:

Passed Daqifi.Core.Tests.Communication.Transport.TcpStreamTransportTests.TcpStreamTransport_Constructor_WithoutLocalInterface_LocalInterfaceIsNull
Passed Daqifi.Core.Tests.Communication.Transport.TcpStreamTransportTests.TcpStreamTransport_Constructor_WithLocalInterface_ExposesIt
Passed Daqifi.Core.Tests.Communication.Transport.TcpStreamTransportTests.TcpStreamTransport_ConnectAsync_WithLoopbackLocalInterface_ConnectsAndReportsBoundLocal
Passed Daqifi.Core.Tests.Communication.Transport.TcpStreamTransportTests.TcpStreamTransport_ConnectAsync_WithUnassignedLocalInterface_ThrowsSocketException
Passed Daqifi.Core.Tests.Device.Discovery.DeviceInfoTests.DeviceInfo_LocalInterfaceAddress_CanBeSetAndRetrieved
Passed Daqifi.Core.Tests.Device.Discovery.DeviceInfoTests.DeviceInfo_SerialDevice_LocalInterfaceAddress_IsNull
Passed Daqifi.Core.Tests.Device.Discovery.DeviceInfoTests.DeviceInfo_WiFiDevice_WithLocalInterfaceAddress_StoresCorrectly
Passed Daqifi.Core.Tests.Device.DaqifiDeviceFactoryTests.ConnectFromDeviceInfoAsync_WiFiWithLocalInterface_BindsOutboundSocketToIt

8/8 passed on both net9.0 and net10.0 — matches the PR description.

Caveats

  • Test host is effectively single-homed (only en0 has IPv4). This run proves the new code path works (socket binds, connects, streams) but does not demonstrate the multi-homed fix itself — that requires a host where the OS would otherwise pick the wrong NIC. The TcpStreamTransport_ConnectAsync_WithUnassignedLocalInterface_ThrowsSocketException unit test is what actually proves the bind takes effect at the socket layer (it would silently succeed if localInterface were ignored).
  • The legacy --ip path (which uses ConnectTcpAsync, unchanged by this PR) was also verified to still work.

Note on WiFi streaming flakiness during testing

Late in my test session the device's WiFi side stopped emitting DaqifiOutMessage stream messages — the TCP connection still established cleanly (both via this PR's new path AND via the unchanged --ip path), but 0 samples arrived. USB-serial streaming on the same device kept working fine. This is not a regression from this PR — affected unchanged code paths equally — and was almost certainly device-side state from accumulated test runs. Flagging it only because it's the sort of thing worth a quick power-cycle before more hardware testing.

@tylerkron tylerkron merged commit 8bc1458 into main May 14, 2026
1 check passed
@tylerkron tylerkron deleted the claude/crazy-torvalds-35cae9 branch May 14, 2026 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Honor LocalInterfaceAddress when connecting discovered WiFi devices

1 participant