fix(dds): send unicast SPDP to the configured peer port - #177
Open
youtalk wants to merge 2 commits into
Open
Conversation
DDSTransportSession and RclTransportSession mapped only DDSPeer.address
into the CycloneDDS <Peer address="..."/> list, dropping DDSPeer.port.
Without a port, add_addresses_to_addrset_1() patches in the *participant*
unicast discovery port (7400 + 250 * domain + 10) and then adds one
locator per participant index up to MaxAutoParticipantIndex — 7410, 7412,
... 7426 on domain 0. A remote running the default ParticipantIndex
("none") binds ephemeral unicast ports, so nothing listens there: SPDP
went to dead ports, discovery never completed, and no error was raised.
The failure is indistinguishable from a firewall block or AP isolation,
which is what it gets misdiagnosed as.
Both call sites now emit DDSPeer.discoveryAddress — host:port, with a
bare IPv6 address bracketed because ddsi_ipaddr_from_string only reads a
port off a bracketed IPv6 address (fe80::1:7400 would otherwise parse as
a different address entirely).
Because the port is now load-bearing, the port: 7400 default is only
correct on domain 0; the docs point at DDSPeer.peer(address:domainId:)
for other domains.
Reported and diagnosed by @peichunhuang-1.
Fixes #176
Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
dds_bridge_create_session ignored a failed dds_create_domain and carried on to dds_create_participant, which creates an *implicit* domain on the DEFAULT configuration: multicast SPDP, no <Peers>, no network-interface pin. The session then reported itself connected while discovering nothing — the same "looks like a firewall block" symptom that made #176 so hard to pin down, and it hides exactly the peer misconfiguration the previous commit makes visible. The rejection now surfaces as DDSError.sessionCreationFailed. DDS_RETCODE_PRECONDITION_NOT_MET stays tolerated: it means the domain already exists (another session, or rmw), which is the documented process-lifetime limitation where the first session's config wins. Verified against the shipping CycloneDDS build: a peer of 192.0.2.10:7400 creates the domain (so the new host:port attribute form is accepted), while an unresolvable network interface now throws instead of silently falling back to default discovery. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #176.
Root cause
Confirmed against the CycloneDDS source in
vendor/cyclonedds, and it is exactly what @peichunhuang-1 diagnosed:DDSPeer.portnever reached the<Peer address="..."/>list.q_addrset.cadd_addresses_to_addrset_1()— a peer locator with no port makes CycloneDDS patch in the participant unicast discovery port (7400 + 250 * domain + 10) and add one locator per participant index up toMaxAutoParticipantIndex(default 9): 7410, 7412, … 7426 on domain 0. That matches the reported tcpdump exactly.ParticipantIndexisnone(defconfig.c,-2), whichddsi_portmapping.cmaps to port 0 — the remote's unicast ports are ephemeral, advertised over SPDP rather than derived. So SPDP left for ports nothing was bound to, discovery never completed, and nothing was reported. The failure is indistinguishable from a firewall block or AP isolation.maxidx = 0: one locator, the configured port.Not a regression — the port has been dropped since the first commit.
Two things beyond the report
The RCL path had the same drop.
RclTransportSession.swiftmapped$0.addresstoo, and since 1.3.0 RCL is the default backend on Apple. Fixing onlyDDSTransportSessionwould have left the main path broken.A second silent-failure defect on the same path.
dds_bridge_create_sessionignored a faileddds_create_domainand went on todds_create_participant, which creates an implicit domain on the DEFAULT configuration — multicast SPDP, no<Peers>, no interface pin — while reporting the session as connected. Any discovery config CycloneDDS rejects degraded silently into the same "looks like a firewall" symptom, and it would have hidden precisely the peer misconfiguration this PR makes visible. It now surfaces asDDSError.sessionCreationFailed;DDS_RETCODE_PRECONDITION_NOT_MET("domain already exists") stays tolerated, since that is the documented process-lifetime limitation where the first session's config wins.Changes
DDSPeer.discoveryAddress— the<Peer address>form:host:port, with a bare IPv6 address bracketed.ddsi_ipaddr_from_stringonly reads a port off a bracketed IPv6 address, sofe80::1:7400would otherwise parse as a different address with no port.DDSTransportSessionandRclTransportSessionboth emit it.dds_bridge.cfails loudly on a rejected discovery config.port: 7400default is only correct on domain 0 — the DDS article steers toDDSPeer.peer(address:domainId:)for other domains. Parity evidence updated from "bare host" to "host:port".Verification
discoveryAddressformatting (IPv4 / bare IPv6 / already-bracketed), port propagation through both transport sessions, port preservation into the generated XML, and a fail-loud regression test.swift format lint --strictclean.192.0.2.10:7400peer creates the domain — so the newhost:portattribute form is accepted — while an unresolvable interface name now throws instead of silently falling back to default discovery.SwiftROS2IntegrationTests) — the ROS 2 host was unreachable at the time of writing. @peichunhuang-1 confirmed the equivalent one-line change on their own setup.