refactor(rtps)!: consolidate to a single rtps component + thorough docs (mermaid) - #712
refactor(rtps)!: consolidate to a single rtps component + thorough docs (mermaid)#712finger563 wants to merge 4 commits into
rtps component + thorough docs (mermaid)#712Conversation
…docs REFACTOR_PLAN.md's end state: a single component named `rtps` using the embeddedRTPS engine. This deletes the original from-scratch `components/rtps` (discovery + best-effort only, never ROS 2-interop, docs overstated its status) and renames the interop-proven `rtps_embedded` -> `rtps`, taking over the published `espp/rtps` component-registry slot. Rename + removal: - git rm old components/rtps; git mv rtps_embedded -> rtps; example rtps_embedded_example.cpp -> rtps_example.cpp and pc tests rtps_embedded_* -> rtps_* (golden .inc wire bytes unchanged - only comments). - Rewired every path/name reference: build.yml (dedup example entry), rtps_interop.yml (paths + harness cd), lib/espp.cmake (engine source paths), interop harness (run.sh/run_interop.sh/Dockerfile), Doxyfile (old rtps.hpp -> the 5 facade headers), Kconfig, idf_component.yml. upload_components.yml already listed `components/rtps`. The two historical design docs keep their rtps_embedded narrative intentionally. Documentation (mermaid): - Rewrote components/rtps/README.md (architecture + messaging-topology diagrams) and doc/en/protocols/rtps.rst (architecture, discovery, reliable pub/sub, RMI correlation, AMI state diagrams) + wired the facade headers into the API reference. All 7 diagrams validated with mmdc. sphinx uses raw (client-side) mermaid, so neither CI nor sphinx catches syntax errors - validate locally. Robustness (surfaced by the consolidation gate): - The native-cancel demo/test used an unbounded goal + fixed-sleep cancel; under CI timing a lost cancel let the goal run for minutes and stop()'s execute-thread join blocked on it. Now bounded n + cancel triggered off the first feedback (deterministic - the goal is accepted and executing, so cancel_goal() has its handle). BREAKING CHANGE: the from-scratch `rtps` component (espp::RtpsParticipant from rtps.hpp, SPDP/SEDP discovery scaffold) is removed. `espp/rtps` now provides the embeddedRTPS-backed facade (pub/sub + services + actions). Consumers of the old API must migrate to the new facade headers (rtps_participant.hpp etc.). Gate: golden byte-for-byte unchanged, interop 27/27 from components/rtps/interop, host lib builds, native cancel loopback + python demo pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull request overview
This PR completes the consolidation to a single rtps component by replacing the legacy from-scratch implementation with the embeddedRTPS-backed facade, updating host tests/interop harness paths, and refreshing docs (including mermaid diagrams).
Changes:
- Consolidate/rename the embeddedRTPS-backed component surface into
components/rtps(engine + facade + typed pub/sub, services, actions) and removertps_embedded. - Update CI/workflows, host-side golden/interop tests, and build system wiring to point at the new
rtpspaths. - Make cancellation demos/tests deterministic under CI load by bounding goals and canceling based on first feedback.
Reviewed changes
Copilot reviewed 45 out of 121 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| python/rtps_rpc_demo.py | Makes native action cancel test deterministic (bounded goal, cancel on first feedback). |
| pc/tests/rtps_typed_pubsub.cpp | Updates plan/doc path reference for typed pub/sub host test. |
| pc/tests/rtps_service_naming.cpp | Updates design doc path reference for service naming unit test. |
| pc/tests/rtps_pubsub.cpp | Updates component path references in pub/sub loopback test header comments. |
| pc/tests/rtps_native_action_loopback.cpp | Bounds cancel test goal duration to avoid CI hangs. |
| pc/tests/rtps_interop_sub.cpp | Renames usage text + plan/doc references to rtps. |
| pc/tests/rtps_interop_pub.cpp | Renames usage text + plan/doc references to rtps. |
| pc/tests/rtps_golden.inc | Renames golden include header comments and generator command. |
| pc/tests/rtps_golden.cpp | Renames golden test driver and include to rtps_*. |
| pc/tests/rtps_facade_frag.cpp | Updates plan/doc path reference for fragmentation test. |
| lib/python_bindings/rtps_bindings.cpp | Updates component path references in bindings header comments. |
| lib/espp.cmake | Updates host build include/source paths from rtps_embedded to rtps. |
| doc/en/protocols/rtps_rmi_ami.rst | Updates docs to refer to rtps component paths. |
| doc/Doxyfile | Switches Doxygen inputs from legacy rtps.hpp to the facade headers. |
| components/rtps/src/utils/Diagnostics.cpp | Adds/maintains engine diagnostics counter definitions under new component. |
| components/rtps/src/entities/Writer.cpp | Adds writer implementation under consolidated rtps engine sources. |
| components/rtps/src/entities/StatelessReader.cpp | Adds stateless reader implementation under consolidated rtps engine sources. |
| components/rtps/Kconfig | Renames menu and updates wording to match consolidated rtps component. |
| components/rtps/interop/run.sh | Updates harness entrypoint paths to components/rtps/interop. |
| components/rtps/interop/ros2_fibonacci_server.py | Adds ROS 2 action server helper for interop matrix. |
| components/rtps/interop/ros2_big_publisher.py | Adds ROS 2 big-payload publisher helper for fragmentation interop leg. |
| components/rtps/interop/ros2_add_two_ints_server.py | Adds ROS 2 service server helper for interop matrix. |
| components/rtps/interop/README.md | Updates instructions/paths and references to renamed golden test. |
| components/rtps/interop/Dockerfile | Adds ROS 2 Jazzy-based image for interop matrix + wheel build/testing. |
| components/rtps/include/rtps/utils/udpUtils.hpp | Adds embeddedRTPS UDP port/address helpers under rtps. |
| components/rtps/include/rtps/utils/sysFunctions.hpp | Adds embeddedRTPS time helpers under rtps. |
| components/rtps/include/rtps/utils/printutils.hpp | Adds embeddedRTPS debug printing helpers under rtps. |
| components/rtps/include/rtps/utils/Log.hpp | Adds embeddedRTPS verbosity macro header under rtps. |
| components/rtps/include/rtps/utils/hash.hpp | Adds embeddedRTPS string hashing helper under rtps. |
| components/rtps/include/rtps/utils/Diagnostics.hpp | Declares diagnostics counters surfaced by the engine. |
| components/rtps/include/rtps/utils/constants.hpp | Adds constants header placeholder under rtps. |
| components/rtps/include/rtps/utils/CdrBuffer.hpp | Updates golden-test reference in discovery CDR buffer docs. |
| components/rtps/include/rtps/storages/ThreadSafeCircularBuffer.tpp | Adds circular buffer implementation (with optional dynamic growth). |
| components/rtps/include/rtps/storages/ThreadSafeCircularBuffer.hpp | Adds circular buffer interface (with optional dynamic growth). |
| components/rtps/include/rtps/storages/StorageArray.hpp | Introduces static-vs-dynamic storage policy wrapper for engine pools. |
| components/rtps/include/rtps/storages/SimpleHistoryCache.hpp | Adds history cache with optional dynamic growth path. |
| components/rtps/include/rtps/storages/PayloadBuffer.hpp | Adds payload buffer wrapper used by cache changes. |
| components/rtps/include/rtps/storages/MemoryPool.hpp | Adds memory pool with optional dynamic growth path. |
| components/rtps/include/rtps/storages/CacheChange.hpp | Adds cache-change type including inline-QoS correlation support. |
| components/rtps/include/rtps/rtps.hpp | Adds top-level engine header entrypoint. |
| components/rtps/include/rtps/rpc/service_naming.hpp | Adds ROS 2 service name/type mangling helpers. |
| components/rtps/include/rtps/rpc/sample_identity.hpp | Adds related_sample_identity constants + serialization helpers. |
| components/rtps/include/rtps/rpc/action_naming.hpp | Adds ROS 2 action name/type mangling helpers. |
| components/rtps/include/rtps/messages/MessageReceiver.hpp | Adds message receiver interface for engine message parsing/dispatch. |
| components/rtps/include/rtps/entities/WriterProxy.hpp | Adds writer-proxy type for reader state tracking. |
| components/rtps/include/rtps/entities/Writer.hpp | Adds writer base class + related_sample_identity publish helper. |
| components/rtps/include/rtps/entities/StatelessWriter.hpp | Adds stateless writer interface and fragmentation hook. |
| components/rtps/include/rtps/entities/StatelessReader.hpp | Adds stateless reader interface. |
| components/rtps/include/rtps/entities/StatefulWriter.hpp | Adds stateful writer interface and scheduler-driven heartbeat design. |
| components/rtps/include/rtps/entities/StatefulReader.hpp | Adds stateful reader interface. |
| components/rtps/include/rtps/entities/ReaderProxy.hpp | Adds reader-proxy type for writer state tracking. |
| components/rtps/include/rtps/entities/Reader.hpp | Adds reader base class + inline-QoS correlation surface + reassembly slot. |
| components/rtps/include/rtps/entities/Participant.hpp | Adds engine participant interface and endpoint management. |
| components/rtps/include/rtps/entities/Domain.hpp | Adds engine domain interface + protocol scheduler task design. |
| components/rtps/include/rtps/discovery/TopicData.hpp | Adds SEDP topic data parsing/serialization and matching helpers. |
| components/rtps/include/rtps/discovery/SPDPAgent.hpp | Adds SPDP agent interface and scheduler-driven announce design. |
| components/rtps/include/rtps/discovery/SEDPAgent.hpp | Adds SEDP agent interface and endpoint match/delete logic. |
| components/rtps/include/rtps/discovery/ParticipantProxyData.hpp | Adds SPDP proxy data parsing and liveliness tracking helpers. |
| components/rtps/include/rtps/discovery/BuiltInEndpoints.hpp | Adds builtin endpoints bundle used by discovery agents. |
| components/rtps/include/rtps/config.hpp | Centralizes limits-profile selection and host default for dynamic storage. |
| components/rtps/include/rtps/config_host_large.hpp | Adds large host limits profile header. |
| components/rtps/include/rtps/config_esp32.hpp | Adds ESP32 embedded limits profile header. |
| components/rtps/include/rtps/config_desktop.hpp | Adds default host limits profile header. |
| components/rtps/include/rtps/communication/PacketInfo.hpp | Adds packet container used by transport/dispatch. |
| components/rtps/include/rtps/communication/EsppTransport.hpp | Adds espp socket-reactor/thread-pool based transport adapter. |
| components/rtps/include/rtps/common/Locator.hpp | Adds RTPS locator types + wire-format read helpers. |
| components/rtps/include/rtps_service.hpp | Adds typed service client/server wrappers over byte-level participant API. |
| components/rtps/include/rtps_pubsub.hpp | Adds typed pub/sub wrappers + small ROS 2 topic helper. |
| components/rtps/include/rtps_participant.hpp | Updates facade docs/references to consolidated rtps engine path. |
| components/rtps/include/rtps_message.hpp | Adds shared typed-layer concepts + serialize/deserialize helpers. |
| components/rtps/include/rtps_action.hpp | Adds typed action client/server wrappers over byte-level participant API. |
| components/rtps/idf_component.yml | Updates component metadata for consolidated rtps. |
| components/rtps/example/sdkconfig.defaults | Updates example defaults (performance + Ethernet + partitions). |
| components/rtps/example/README.md | Reorients example docs around Ethernet + typed APIs + ROS 2 interop. |
| components/rtps/example/pc/CMakeLists.txt | Renames host example project target to match rtps. |
| components/rtps/example/main/Kconfig.projbuild | Simplifies example Kconfig for publish period + optional self-test participant. |
| components/rtps/example/main/CMakeLists.txt | Updates example dependencies from wifi to ethernet-kit + typed deps. |
| components/rtps/example/CMakeLists.txt | Updates example component list for new dependencies and removes wifi. |
| components/rtps/CMakeLists.txt | Registers consolidated engine sources and Kconfig-driven build defines. |
| components/rtps_embedded/README.md | Removes old component README after consolidation. |
| components/rtps_embedded/idf_component.yml | Removes old component manifest after consolidation. |
| components/rtps_embedded/example/sdkconfig.defaults | Removes old example defaults after consolidation. |
| components/rtps_embedded/example/README.md | Removes old example README after consolidation. |
| components/rtps_embedded/example/partitions.csv | Removes old example partition table after consolidation. |
| components/rtps_embedded/example/main/Kconfig.projbuild | Removes old example Kconfig after consolidation. |
| components/rtps_embedded/example/main/CMakeLists.txt | Removes old example main component CMake after consolidation. |
| components/rtps_embedded/example/CMakeLists.txt | Removes old example project CMake after consolidation. |
| components/rtps_embedded/CMakeLists.txt | Removes old component build file after consolidation. |
| .github/workflows/rtps_interop.yml | Updates interop workflow triggers and working directory to rtps. |
| .github/workflows/build.yml | Removes rtps_embedded/example from the build matrix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address PR #712 review: - idf_component.yml: restore a documentation: link (dropped in the rename), pointing at the new docs page, matching socket/rtsp. - example/README.md: the Kconfig menu is now titled 'RTPS' (not 'RTPS (rtps)'). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed both Copilot comments in d4d55c6:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 121 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
doc/en/protocols/rtps.rst:1
- This table states
RTPS_LIMITS_PROFILEdefaults toembedded, but elsewhere in the repo the host build tooling indicates a host default (e.g.,lib/espp.cmakesetsRTPS_LIMITS_PROFILEtohostwhen not defined). To avoid confusing users, please document the default as platform-dependent (ESP-IDF: embedded; host/non-ESP: host), or align the actual default behavior to match the docs.
python/rtps_rpc_demo.py:1 - The return value of
nexecuting.wait(5.0)is ignored. If no feedback arrives (or the callback is not actually tied to feedback), the code will still callcancel_goal()without having established that the goal is executing/accepted, which can reintroduce the race/flakiness this change is trying to remove. Consider checking the boolean result ofwait()and marking the demo/test as failed (or taking an alternate deterministic step) when feedback doesn’t arrive in time, rather than proceeding to cancel unconditionally.
The consolidation removed the old from-scratch rtps.hpp (discovery scaffold); the esp32-p4-function-ev-board example was its only remaining consumer and broke with 'rtps.hpp: No such file or directory'. Migrate to the new facade: - rtps.hpp -> rtps_participant.hpp - Config: node_name/participant_id/advertised_address/announce_period/ on_participant_discovered/on_endpoint_discovered -> interface_address + on_publisher_matched (sets rtps_has_peers) + log_level - add_writer .topic_name -> .topic - gate publish on rtps_has_peers (matched) instead of discovered_participants() - the new facade doesn't expose remote peer addresses, so drop the peer-ping branch of the connectivity self-test (keep the gateway ping) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Declare 'published' inside the can_publish block where it is used, resolving the static-analysis finding introduced when the peer-state assignment was removed during the RTPS facade migration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 122 changed files in this pull request and generated no new comments.
Suppressed comments (3)
python/rtps_rpc_demo.py:1
nexecuting.wait(5.0)ignores its return value, so on timeout the code still callscancel_goal()even though the goal may not be accepted/executing (reintroducing the race this change is trying to eliminate). Consider branching on the wait result: only callcancel_goal()oncenexecutingis set, otherwise record a deterministic failure (or retry/wait longer) and proceed to shutdown cleanly.
doc/en/protocols/rtps.rst:1- The sequence ````ACKNACK
\ sis likely to render incorrectly in RST; it reads like an accidental escape. Prefer writing “``ACKNACK``s” (no escape) or rephrasing (e.g., “the reader sends ``ACKNACK`` messages for missing samples”).
doc/en/protocols/rtps.rst:1 - This table entry is ambiguous/misleading given elsewhere the host build defaults to
host(e.g.,lib/espp.cmakesetsRTPS_LIMITS_PROFILEto"host"when not defined). Consider clarifying the default by platform (e.g., “embeddeddefault on ESP-IDF;hostdefault for non-ESP builds”) to prevent users from assuming the wrong profile is active.
What & why
REFACTOR_PLAN.md's stated end state is a single component namedrtpsusing the embeddedRTPS engine. This PR executes that consolidation now that the RMI/AMI work has merged (#711):components/rtps(discovery + best-effort only, never ROS 2-interop; its README/RELIABLE_RTPS_PLAN.mdoverstated its status).rtps_embedded→rtps, taking over the publishedespp/rtpscomponent-registry slot.Rename + removal
git rmoldcomponents/rtps;git mv rtps_embedded → rtps; examplertps_embedded_example.cpp → rtps_example.cpp; pc testsrtps_embedded_* → rtps_*(golden.incwire bytes unchanged — only comments).build.yml(dedup the example matrix entry),rtps_interop.yml(path triggers + harnesscd),lib/espp.cmake(engine source paths), the interop harness (run.sh/run_interop.sh/Dockerfile),doc/Doxyfile(oldrtps.hpp→ the 5 facade headers),Kconfig,idf_component.yml.upload_components.ymlalready listedcomponents/rtps, so the registry slot transfers cleanly.REFACTOR_PLAN.md,RMI_AMI_DESIGN.md) intentionally keep theirrtps_embeddedtransition narrative.Documentation (mermaid)
components/rtps/README.md(layered-architecture + messaging-topology diagrams) anddoc/en/protocols/rtps.rst(architecture, discovery, reliable pub/sub HEARTBEAT/ACKNACK, RMIrelated_sample_identitycorrelation, AMI goal state machine) and wired the facade headers into the API reference.mmdc. Note: sphinx usesmermaid_output_format='raw'(client-side render), so neither CI nor the sphinx build catches mermaid syntax errors — they must be validated locally.Robustness (surfaced by the consolidation gate)
The native-cancel demo/test used an unbounded goal + fixed-sleep cancel. Under CI timing, a cancel that raced the goal-accept round-trip was dropped, the
n=100000goal ran for ~50 min, and — becausestop()now joins execute threads (from #711) —stop()blocked on it and wedged the gate. Fixed: boundedn+ cancel triggered off the first feedback (deterministic: the goal is accepted and executing, socancel_goal()has its handle).The from-scratch
rtpscomponent (espp::RtpsParticipantfromrtps.hpp, the SPDP/SEDP discovery scaffold) is removed.espp/rtpsnow provides the embeddedRTPS-backed facade (pub/sub + services + actions). Consumers of the old API migrate to the new facade headers (rtps_participant.hpp,rtps_pubsub.hpp,rtps_service.hpp,rtps_action.hpp).Gate
components/rtps/interop), incl. ROS 2 (Jazzy/FastDDS) both directions + python wheel demo.native_cancelpass; all mermaid diagrams render.🤖 Generated with Claude Code