From fb85084e011aefd064dbe6d10ac618a38da73ba8 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 16:13:16 +0200 Subject: [PATCH 01/19] docs: message transforms design (v1: Cloudini) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- .../2026-09-20-message-transforms-design.md | 234 ++++++++++++++++++ 1 file changed, 234 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-20-message-transforms-design.md diff --git a/docs/superpowers/specs/2026-09-20-message-transforms-design.md b/docs/superpowers/specs/2026-09-20-message-transforms-design.md new file mode 100644 index 0000000..67b6874 --- /dev/null +++ b/docs/superpowers/specs/2026-09-20-message-transforms-design.md @@ -0,0 +1,234 @@ +# Message Transforms (v1: Cloudini) — Design + +Date: 2026-09-20. Status: approved design, not implemented. +Supersedes the exploratory `docs/MESSAGE_TRANSFORMS.md` (PR #14). + +## Goal + +Let the server operator replace the payload of selected topics with a +transformed one before it is buffered and sent. First transform: +[Cloudini](https://github.com/facontidavide/cloudini) point cloud compression +(`sensor_msgs/msg/PointCloud2` → `point_cloud_interfaces/msg/CompressedPointCloud2`). +The existing `MessageStripper` becomes the second implementation. + +## Decisions + +| Question | Decision | +|---|---| +| Scope | Minimal seam + Cloudini. No per-client variants, no offer/force negotiation, no worker pool. | +| Who enables it | Server operator, off by default. A topic is transformed for all subscribers or none. | +| Configuration | One parameter, `transform_profile`, pointing to a JSON file. | +| Wire protocol | Unchanged. Transformed topics are simply advertised with their output type and schema. | +| Cloudini dependency | `find_package(cloudini_lib)`, FetchContent fallback (pinned tag + SHA256, static). Feature compiled out if neither works. | +| Double compression | Cloudini second stage is `NONE`; the frame-level zstd is the general-purpose stage. | +| `strip_large_messages` | `strip` becomes a transform name; the boolean stays as sugar. | +| Threading | Synchronous in the ingest callback. Measure before adding a worker. | + +Client requirement: a PlotJuggler whose `parser_ros` decodes +`CompressedPointCloud2` (`pj-official-plugins`, branch +`feat/compressed-pointcloud`). Older clients see a topic they cannot parse; +that is the operator's responsibility when enabling the profile. The plugin +already binds its parser from the type in `get_topics` and the +`encoding`/`definition` in the subscribe response, so it needs no protocol +change. + +## Architecture + +`BridgeServer` learns types and schemas only through `TopicSourceInterface` +and message bytes only through `SubscriptionManagerInterface`. It is not +modified (except for the informational fields below). + +``` +Ros2TopicSource ──► TransformingTopicSource ──► BridgeServer + │ records topic → (source_type, rule) + ▼ + TransformSet ◄── profile JSON + name→factory map + ▲ resolve(topic) → MessageTransform* +Ros2SubscriptionManager ──┘ (calls apply() on the rcl buffer, before any copy) +``` + +- **`TransformSet`** (`app/`): owns the ordered rules, the name→factory map, + and the per-topic transform instances. `resolve(topic, source_type)` returns + the matching rule (first match wins) or nothing. Thread-safe: written from + the request thread (`get_topics`/`subscribe`), read from the ingest thread. +- **`TransformingTopicSource`** (`app/`, decorator): `get_topics()` rewrites + `type` for matched topics and records the source type in the `TransformSet`; + `get_schema()` returns the transform's output schema; everything else passes + through. +- **Backend hook**: `Ros2SubscriptionManager::subscribe(name, advertised_type)` + asks the `TransformSet` for the topic's real source type and subscribes with + it. In the message callback, if a transform is resolved for the topic, call + `apply()` on the view over the rcl buffer and forward `out`; otherwise copy + as today. FastDDS can adopt the same three lines later; v1 wires ROS2 only. +- With no profile and `strip_large_messages: false`, no `TransformSet` is + created and behavior is byte-identical to today. + +Everything downstream (latched retention, heavy-frame threshold, rate gate, +serializer, backpressure) sees post-transform bytes with no special cases. A +2 MiB cloud that compresses below `heavy_frame_threshold_bytes` rejoins normal +aggregation. + +## Interface + +`app/include/pj_bridge/message_transform.hpp`, no ROS/DDS dependencies: + +```cpp +/// One instance per topic. Called from a single thread (the ingest executor). +/// Implementations SHOULD keep codec contexts and scratch buffers as members. +class MessageTransform { + public: + virtual ~MessageTransform() = default; + /// `in` is a view over the middleware's buffer, valid only during the call. + /// `out` is the final storage (already inside the shared_ptr handed to + /// MessageBuffer): cleared and filled in place, never moved or copied. + /// On error the sample is DROPPED by the caller — never forwarded raw, since + /// the client was told the output type at subscribe time. + virtual tl::expected apply( + std::span in, std::vector& out) = 0; +}; + +struct TransformOutputType { + std::string type_name; + std::string schema; +}; + +struct TransformFactory { + std::function accepts; + std::function output; + std::function, std::string>( + const nlohmann::json& params)> create; +}; +``` + +Allocation rationale: the input is never copied for a transformed topic (the +raw cloud is read in place from the rcl buffer); the only per-message +allocation is the output vector at compressed size, which is unavoidable +because `MessageBuffer` retains it. The input cannot use move semantics: the +rcl buffer is owned by rclcpp and a `std::vector` cannot adopt it, so a +read-only span is the only zero-copy option. The hook does +`auto out = std::make_shared>(); apply(in, *out);` — +the transform writes straight into the final storage. In v1 every message gets +a fresh vector; because `out` is caller-provided, the hook can later hand out +recycled vectors (`shared_ptr` with a recycling deleter — +no downstream change) if measurement justifies it. Not built in v1. + +## Transforms + +### `cloudini` (`app/`, compiled only when Cloudini is available) + +`getDeserializedPointCloudMessage` → `applyResolutionProfile` → optional +`applyVizLossyPreprocessing` → encode. Encoding uses the lower-level +`PointcloudEncoder::encode(ConstBufferView, BufferView)` + +`writePointCloudHeader` so the result is written directly into the +`std::vector` (the convenience +`convertPointCloud2ToCompressedCloud` writes a `vector`, which would +cost an extra output copy). Second stage `NONE`. The encoder and the +preprocessing buffer are members, reused while the field layout is unchanged. +Output schema comes from Cloudini's embedded `ros_message_definitions.hpp`, so +`point_cloud_interfaces` need not be installed. + +Params: + +| Key | Default | Meaning | +|---|---|---| +| `resolution` | `0.001` | Resolution for FLOAT32 fields not listed in `fields` (metres for xyz) | +| `fields` | `{}` | Per-field resolution; `0` removes the field | +| `viz_preprocessing` | `false` | NaN drop + voxel dedup + 1 µs FLOAT64 quantization | + +### `strip` (`ros2/`, registered by `ros2/main.cpp`) + +Thin adapter over the existing `MessageStripper`; output type = input type, +schema unchanged. It keeps its current cost (full typed deserialize / +re-serialize, plus one copy into a `SerializedMessage`). A CDR-level stripper +that avoids this is a separate follow-up. + +## Configuration + +ROS2 parameter `transform_profile` (string, default empty = disabled). + +```json +{ + "transforms": [ + { + "match_type": "sensor_msgs/msg/PointCloud2", + "match_topic": "/lidar/.*", + "transform": "cloudini", + "params": {"resolution": 0.001, "fields": {"intensity": 0.01}, "viz_preprocessing": true} + } + ] +} +``` + +- At least one of `match_type` (exact) / `match_topic` (full-match regex, same + semantics as `topic_whitelist`); when both are present both must match. +- Ordered, first match wins. +- `strip_large_messages: true` appends one `strip` rule per strippable type + **after** the profile's rules, so an explicit profile rule wins. + +**Fail at startup** (clear error, non-zero exit): unreadable or malformed +file, unknown top-level/rule/param key, unknown transform name, `cloudini` +named in a build without Cloudini, invalid regex, `create()` returning an +error, a `match_type` the named transform does not `accept()`. A rule without +`match_type` whose transform does not `accept()` a topic it matched by +`match_topic` is reported once as a warning and the topic is left +untransformed (types are only known at discovery time). + +**At runtime**, `apply()` failure → drop the sample, bump a counter, throttled +warning. This also applies to `strip` (today it forwards the original; changed +for uniformity). + +## Informational protocol additions (ignored by old clients) + +- `message_transforms` in the server capabilities list. +- Optional `"source_type"` on `get_topics` entries of transformed topics. +- Per-transform counters in the 5 s stats line: in bytes, out bytes, drops, + mean µs per sample. + +## Build + +The project moves from C++17 to **C++20** unconditionally: `std::span` needs +it, and `cloudini_lib` exports `cxx_std_20` as a PUBLIC requirement, so the +standard must not depend on whether the optional dependency was found. The +oldest targeted compiler is GCC 11 (Humble / Jammy), which supports it. + +```cmake +find_package(cloudini_lib QUIET) +if(NOT cloudini_lib_FOUND) # FetchContent fallback, pinned tag + URL_HASH + ... +endif() +# PJ_BRIDGE_HAS_CLOUDINI defined on pj_bridge_app when available +``` + +Buildfarm debs cannot fetch at build time, so they get the feature once +`cloudini_lib` is a resolvable rosdep key for the distro; GitHub-built debs, +conda packages and AppImages get it through the fallback. + +## Testing + +- Profile parsing: valid file; each startup failure mode. +- Rule matching and precedence, including the `strip_large_messages` sugar. +- `TransformingTopicSource` with the existing mock topic source: type and + schema rewrite, untouched topics pass through. +- Backend hook with a fake transform: source-type lookup on subscribe, output + forwarded, failure drops the sample and counts it. +- `cloudini` smoke test (only when compiled in): a synthetic PointCloud2 CDR + buffer transforms without error into a non-empty, smaller output. No + decode/round-trip test — decoding correctness belongs to Cloudini's own + suite. +- End-to-end, manual: PlotJuggler with the compressed-pointcloud parser against + the 4-lidar bag. + +## Acceptance measurement + +Pinned rig from `docs/perf/README.md`, 4-lidar bag (~52 MiB/s), 1 client, +Cloudini off vs on: bridge CPU (% of a core), bytes on the wire, and +client-side per-topic message counts (must be lossless). If encode time +starves ingest, the designed-but-unbuilt fallback is a per-topic worker with a +1-deep latest-wins input slot. + +## Non-goals (v1) + +Per-client transform choice or parameters; client negotiation; transform +chaining; 1→N fan-out; topic renaming; image codecs; output-buffer pooling; +FastDDS/RTI wiring; CDR-level stripper. From 1d4e60774e44ff956ec3abe5965523ab33600636 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 16:19:49 +0200 Subject: [PATCH 02/19] docs: message transforms implementation plan; align spec Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- .../plans/2026-09-20-message-transforms.md | 1282 +++++++++++++++++ .../2026-09-20-message-transforms-design.md | 38 +- 2 files changed, 1301 insertions(+), 19 deletions(-) create mode 100644 docs/superpowers/plans/2026-09-20-message-transforms.md diff --git a/docs/superpowers/plans/2026-09-20-message-transforms.md b/docs/superpowers/plans/2026-09-20-message-transforms.md new file mode 100644 index 0000000..375b753 --- /dev/null +++ b/docs/superpowers/plans/2026-09-20-message-transforms.md @@ -0,0 +1,1282 @@ +# Message Transforms (v1: Cloudini) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Operator-configured, per-topic payload transforms in pj_bridge, with Cloudini point cloud compression and the existing stripper as the two implementations. + +**Architecture:** A `TransformSet` (rules from a JSON profile + name→factory map) is shared by a `TransformingTopicSource` decorator (rewrites advertised type/schema) and by a hook inside `Ros2SubscriptionManager` (runs `apply()` on the rcl buffer before any copy). `BridgeServer` is untouched except for two informational fields. + +**Tech Stack:** C++20, nlohmann/json, tl::expected, spdlog, gtest, Cloudini 1.3.0 (`cloudini_lib`), rclcpp. + +**Spec:** `docs/superpowers/specs/2026-09-20-message-transforms-design.md` + +## Environment (read first) + +All builds and tests run through pixi RoboStack, from the worktree root +`~/ws_plotjuggler/plotjuggler_bridge/.worktrees/message-transforms`: + +```bash +set -o pipefail +mkdir -p install && touch install/setup.sh +B="~/.pixi/bin/pixi run -e humble" +$B colcon build --packages-select pj_bridge --cmake-args -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON 2>&1 | tail -20 +$B bash -c 'source install/setup.bash && build/pj_bridge/pj_bridge_tests --gtest_filter="*"' +``` + +"Build" and "Run tests ``" below mean these two commands. The baseline +is 271+ tests passing. Format with `pre-commit run -a` before every commit. +Commit messages end with the repo's standard `Co-Authored-By` trailer. + +## File map + +| File | Responsibility | +|---|---| +| `app/include/pj_bridge/message_transform.hpp` (new) | `MessageTransform`, `TransformFactory` | +| `app/include/pj_bridge/transform_set.hpp`, `app/src/transform_set.cpp` (new) | Profile parsing, rule matching, per-topic bindings, counters | +| `app/include/pj_bridge/transforming_topic_source.hpp`, `app/src/transforming_topic_source.cpp` (new) | Decorator over `TopicSourceInterface` | +| `app/include/pj_bridge/cloudini_transform.hpp`, `app/src/cloudini_transform.cpp` (new, conditional) | Cloudini factory | +| `ros2/include/pj_bridge_ros2/strip_transform.hpp`, `ros2/src/strip_transform.cpp` (new) | `strip` factory over `MessageStripper` | +| `ros2/src/ros2_subscription_manager.cpp`, its header | The hook; loses its `strip_large_messages` bool | +| `ros2/src/main.cpp` | `transform_profile` param, wiring, stats at shutdown | +| `app/include/pj_bridge/topic_source_interface.hpp` | `TopicInfo::source_type` | +| `app/src/bridge_server.cpp`, `protocol_constants.hpp` | emit `source_type`; `message_transforms` capability | +| `tests/unit/test_transform_set.cpp`, `test_transforming_topic_source.cpp`, `test_cloudini_transform.cpp` (new) | Tests | + +--- + +### Task 1: Move the project to C++20 + +**Files:** Modify `CMakeLists.txt:18` + +- [ ] **Step 1:** Change `set(CMAKE_CXX_STANDARD 17)` to `set(CMAKE_CXX_STANDARD 20)`. Run `grep -rn "cxx_std_17\|CXX_STANDARD 17\|-std=" CMakeLists.txt cmake/ 2>/dev/null` and update any other hit to 20. +- [ ] **Step 2:** Build, then run the whole suite (no filter). Expected: same pass count as before the change. C++20 typically surfaces: implicit `this` capture in `[=]` lambdas (make it `[=, this]`), and `u8""` literal type changes. Fix what the compiler reports, nothing else. +- [ ] **Step 3:** Repeat Step 2 with `-e jazzy`. +- [ ] **Step 4:** FastDDS standalone build still configures and compiles (see CLAUDE.md "FastDDS Backend"); skip if Conan cache is unavailable and say so in the commit body. +- [ ] **Step 5:** Commit: `build: require C++20`. Push the branch and confirm Humble/Jazzy/Kilted/Lyrical CI is green **before** starting Task 2 (so a compiler issue is never confused with the feature). + +--- + +### Task 2: Transform interface + +**Files:** Create `app/include/pj_bridge/message_transform.hpp` + +- [ ] **Step 1:** Write the header (license banner copied from a sibling header): + +```cpp +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace pj_bridge { + +/// One instance per topic, called from a single thread (the ingest executor). +/// Implementations SHOULD keep codec contexts and scratch buffers as members. +class MessageTransform { + public: + virtual ~MessageTransform() = default; + + /// `in` views the middleware's buffer and is valid only during the call. + /// `out` is the final storage later held by MessageBuffer: clear and fill it. + /// On error the caller DROPS the sample. It must never forward `in` instead: + /// the client was told the output type at subscribe time. + virtual tl::expected apply(std::span in, std::vector& out) = 0; +}; + +struct TransformFactory { + /// Can this transform handle messages of `source_type`? + std::function accepts; + /// Reject unknown or malformed params. Called once per rule at startup. + std::function(const nlohmann::json& params)> check_params; + /// Type name advertised to clients in place of `source_type`. + std::function output_type; + /// Schema advertised to clients. `source_schema` is the untransformed one. + std::function output_schema; + std::function(const std::string& source_type, const nlohmann::json& params)> + create; +}; + +} // namespace pj_bridge +``` + +Check the include spelling of `tl/expected.hpp` against `app/include/pj_bridge/whitelist_filter.hpp` and copy it exactly. + +- [ ] **Step 2:** Build (header is not yet included anywhere; this only checks nothing else broke). Commit: `feat(transform): MessageTransform interface`. + +--- + +### Task 3: TransformSet — profile parsing and matching + +**Files:** Create `app/include/pj_bridge/transform_set.hpp`, `app/src/transform_set.cpp`, `tests/unit/test_transform_set.cpp`. Modify `CMakeLists.txt` (add the `.cpp` to `pj_bridge_app`, the test to `${PROJECT_NAME}_tests`). + +- [ ] **Step 1: Header** + +```cpp +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "pj_bridge/message_transform.hpp" + +namespace pj_bridge { + +/// A transform bound to one topic, plus its counters. +/// run() is called from the ingest thread only; counters are read from others. +struct BoundTransform { + std::string source_type; + std::string transform_name; + std::string output_type; + std::unique_ptr transform; + + std::atomic samples{0}; + std::atomic drops{0}; + std::atomic in_bytes{0}; + std::atomic out_bytes{0}; + std::atomic micros{0}; + + /// Applies the transform and updates counters. False = drop the sample. + bool run(const std::string& topic, std::span in, std::vector& out); +}; + +/// Ordered transform rules plus the per-topic bindings created from them. +/// Thread safety: all public methods are safe to call concurrently. +class TransformSet { + public: + using FactoryMap = std::map; + + /// Parse and validate a profile. Any problem is a startup error. + static tl::expected, std::string> create( + const nlohmann::json& profile, FactoryMap factories); + + /// Append a `match_type` rule after the profile's rules (strip_large_messages sugar). + tl::expected append_type_rule(const std::string& match_type, const std::string& transform); + + /// First matching rule wins. Creates and remembers the binding on first sight. + /// Returns nullptr when no rule applies to this topic. + std::shared_ptr bind(const std::string& topic, const std::string& source_type); + + /// Binding previously created by bind(), or nullptr. + std::shared_ptr find(const std::string& topic) const; + + /// Schema to advertise for a bound topic. + std::string output_schema(const BoundTransform& bound, const std::string& source_schema) const; + + /// One line per bound topic that processed at least one sample. + std::string stats_summary() const; + + private: + struct Rule { + std::optional match_type; + std::optional match_topic; + std::string transform; + nlohmann::json params; + }; + + TransformSet() = default; + tl::expected add_rule(Rule rule); + + FactoryMap factories_; + std::vector rules_; + mutable std::mutex mutex_; + std::unordered_map> bindings_; + std::set warned_topics_; +}; + +} // namespace pj_bridge +``` + +- [ ] **Step 2: Failing tests** — `tests/unit/test_transform_set.cpp` + +```cpp +#include + +#include "pj_bridge/transform_set.hpp" + +using namespace pj_bridge; +using nlohmann::json; + +namespace { + +/// Appends one marker byte to the input. `fail` makes apply() return an error. +class FakeTransform : public MessageTransform { + public: + explicit FakeTransform(bool fail) : fail_(fail) {} + tl::expected apply(std::span in, std::vector& out) override { + if (fail_) { + return tl::make_unexpected(std::string("boom")); + } + out.assign(in.begin(), in.end()); + out.push_back(std::byte{0xAB}); + return {}; + } + + private: + bool fail_; +}; + +TransformFactory fake_factory(const std::string& accepted_type) { + TransformFactory f; + f.accepts = [accepted_type](const std::string& t) { return t == accepted_type; }; + f.check_params = [](const json& p) -> tl::expected { + for (auto it = p.begin(); it != p.end(); ++it) { + if (it.key() != "fail") { + return tl::make_unexpected("unknown param '" + it.key() + "'"); + } + } + return {}; + }; + f.output_type = [](const std::string&) { return std::string("pkg/msg/Out"); }; + f.output_schema = [](const std::string&, const std::string& s) { return "OUT:" + s; }; + f.create = [](const std::string&, const json& p) { return std::make_unique(p.value("fail", false)); }; + return f; +} + +TransformSet::FactoryMap factories() { + return {{"fake", fake_factory("pkg/msg/In")}, {"other", fake_factory("pkg/msg/Other")}}; +} + +json rule(json fields) { + return json{{"transforms", json::array({std::move(fields)})}}; +} + +} // namespace + +TEST(TransformSetTest, MatchesByType) { + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + auto bound = (*set)->bind("/a", "pkg/msg/In"); + ASSERT_NE(bound, nullptr); + EXPECT_EQ(bound->source_type, "pkg/msg/In"); + EXPECT_EQ(bound->output_type, "pkg/msg/Out"); + EXPECT_EQ((*set)->bind("/b", "pkg/msg/Unrelated"), nullptr); + EXPECT_EQ((*set)->find("/a"), bound); + EXPECT_EQ((*set)->find("/b"), nullptr); +} + +TEST(TransformSetTest, TopicRegexIsFullMatchAndCombinesWithType) { + auto set = TransformSet::create( + rule({{"match_type", "pkg/msg/In"}, {"match_topic", "/lidar/.*"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + EXPECT_NE((*set)->bind("/lidar/front", "pkg/msg/In"), nullptr); + EXPECT_EQ((*set)->bind("/x/lidar/front", "pkg/msg/In"), nullptr); // full match, not search + EXPECT_EQ((*set)->bind("/lidar/rear", "pkg/msg/Unrelated"), nullptr); +} + +TEST(TransformSetTest, FirstMatchingRuleWinsAndAppendedRulesComeLast) { + json profile = {{"transforms", json::array({{{"match_topic", "/a"}, {"transform", "fake"}, {"params", {{"fail", true}}}}, + {{"match_type", "pkg/msg/In"}, {"transform", "fake"}}})}}; + auto set = TransformSet::create(profile, factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + ASSERT_TRUE((*set)->append_type_rule("pkg/msg/Other", "other").has_value()); + + std::vector in{std::byte{1}}, out; + EXPECT_FALSE((*set)->bind("/a", "pkg/msg/In")->run("/a", in, out)); // rule 0 (fail=true) + EXPECT_TRUE((*set)->bind("/b", "pkg/msg/In")->run("/b", in, out)); // rule 1 + EXPECT_EQ((*set)->bind("/c", "pkg/msg/Other")->transform_name, "other"); +} + +TEST(TransformSetTest, TopicOnlyRuleWithNonAcceptedTypeLeavesTopicUntransformed) { + auto set = TransformSet::create(rule({{"match_topic", "/a"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + EXPECT_EQ((*set)->bind("/a", "pkg/msg/Unrelated"), nullptr); +} + +TEST(TransformSetTest, RebindsWhenSourceTypeChanges) { + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + EXPECT_NE((*set)->bind("/a", "pkg/msg/In"), nullptr); + EXPECT_EQ((*set)->bind("/a", "pkg/msg/Unrelated"), nullptr); + EXPECT_EQ((*set)->find("/a"), nullptr); +} + +TEST(TransformSetTest, RunCountsSamplesBytesAndDrops) { + json profile = {{"transforms", json::array({{{"match_topic", "/bad"}, {"transform", "fake"}, {"params", {{"fail", true}}}}, + {{"match_type", "pkg/msg/In"}, {"transform", "fake"}}})}}; + auto set = TransformSet::create(profile, factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + std::vector in(10, std::byte{7}), out; + + auto good = (*set)->bind("/good", "pkg/msg/In"); + ASSERT_TRUE(good->run("/good", in, out)); + EXPECT_EQ(out.size(), 11u); + EXPECT_EQ(good->samples.load(), 1u); + EXPECT_EQ(good->in_bytes.load(), 10u); + EXPECT_EQ(good->out_bytes.load(), 11u); + EXPECT_EQ(good->drops.load(), 0u); + + auto bad = (*set)->bind("/bad", "pkg/msg/In"); + EXPECT_FALSE(bad->run("/bad", in, out)); + EXPECT_EQ(bad->drops.load(), 1u); + EXPECT_EQ(bad->samples.load(), 0u); +} + +struct BadProfile { + const char* name; + json profile; +}; + +class TransformSetBadProfileTest : public ::testing::TestWithParam {}; + +TEST_P(TransformSetBadProfileTest, IsRejectedAtStartup) { + auto set = TransformSet::create(GetParam().profile, factories()); + EXPECT_FALSE(set.has_value()); +} + +INSTANTIATE_TEST_SUITE_P( + All, TransformSetBadProfileTest, + ::testing::Values( + BadProfile{"not_an_object", json::array()}, + BadProfile{"missing_transforms", json::object()}, + BadProfile{"unknown_top_level_key", json{{"transforms", json::array()}, {"extra", 1}}}, + BadProfile{"rule_without_matcher", rule({{"transform", "fake"}})}, + BadProfile{"rule_without_transform", rule({{"match_type", "pkg/msg/In"}})}, + BadProfile{"unknown_rule_key", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"oops", 1}})}, + BadProfile{"unknown_transform", rule({{"match_type", "pkg/msg/In"}, {"transform", "nope"}})}, + BadProfile{"invalid_regex", rule({{"match_topic", "("}, {"transform", "fake"}})}, + BadProfile{"unknown_param", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"params", {{"q", 1}}}})}, + BadProfile{"type_not_accepted", rule({{"match_type", "pkg/msg/Unrelated"}, {"transform", "fake"}})}), + [](const auto& info) { return std::string(info.param.name); }); +``` + +- [ ] **Step 3:** Add both files to `CMakeLists.txt`, build. Expected: link errors for `TransformSet::*` (not implemented). + +- [ ] **Step 4: Implementation** — `app/src/transform_set.cpp` + +```cpp +#include "pj_bridge/transform_set.hpp" + +#include + +#include +#include + +namespace pj_bridge { + +bool BoundTransform::run(const std::string& topic, std::span in, std::vector& out) { + const auto start = std::chrono::steady_clock::now(); + auto result = transform->apply(in, out); + if (!result) { + const uint64_t n = drops.fetch_add(1) + 1; + if ((n & (n - 1)) == 0) { // 1st, 2nd, 4th, 8th... keeps a broken topic from flooding the log + spdlog::warn("Transform '{}' failed on '{}' ({} drops so far): {}", transform_name, topic, n, result.error()); + } + return false; + } + const auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + samples.fetch_add(1); + in_bytes.fetch_add(in.size()); + out_bytes.fetch_add(out.size()); + micros.fetch_add(static_cast(elapsed.count())); + return true; +} + +tl::expected, std::string> TransformSet::create( + const nlohmann::json& profile, FactoryMap factories) { + if (!profile.is_object() || !profile.contains("transforms") || !profile["transforms"].is_array()) { + return tl::make_unexpected(std::string("transform profile must be an object with a 'transforms' array")); + } + for (auto it = profile.begin(); it != profile.end(); ++it) { + if (it.key() != "transforms") { + return tl::make_unexpected("transform profile: unknown key '" + it.key() + "'"); + } + } + + std::shared_ptr set(new TransformSet()); + set->factories_ = std::move(factories); + + size_t index = 0; + for (const auto& entry : profile["transforms"]) { + const std::string where = "transform profile rule " + std::to_string(index++) + ": "; + if (!entry.is_object()) { + return tl::make_unexpected(where + "must be an object"); + } + Rule rule; + for (auto it = entry.begin(); it != entry.end(); ++it) { + const auto& key = it.key(); + if (key == "match_type" && it->is_string()) { + rule.match_type = it->get(); + } else if (key == "match_topic" && it->is_string()) { + try { + rule.match_topic.emplace(it->get(), std::regex::ECMAScript); + } catch (const std::regex_error& e) { + return tl::make_unexpected(where + "invalid match_topic regex: " + e.what()); + } + } else if (key == "transform" && it->is_string()) { + rule.transform = it->get(); + } else if (key == "params" && it->is_object()) { + rule.params = *it; + } else { + return tl::make_unexpected(where + "unknown or mistyped key '" + key + "'"); + } + } + if (rule.params.is_null()) { + rule.params = nlohmann::json::object(); + } + if (auto added = set->add_rule(std::move(rule)); !added) { + return tl::make_unexpected(where + added.error()); + } + } + return set; +} + +tl::expected TransformSet::add_rule(Rule rule) { + if (!rule.match_type && !rule.match_topic) { + return tl::make_unexpected(std::string("needs match_type and/or match_topic")); + } + if (rule.transform.empty()) { + return tl::make_unexpected(std::string("missing 'transform'")); + } + auto factory = factories_.find(rule.transform); + if (factory == factories_.end()) { + return tl::make_unexpected("unknown transform '" + rule.transform + "' (not built in, or misspelled)"); + } + if (auto ok = factory->second.check_params(rule.params); !ok) { + return tl::make_unexpected("transform '" + rule.transform + "': " + ok.error()); + } + if (rule.match_type && !factory->second.accepts(*rule.match_type)) { + return tl::make_unexpected("transform '" + rule.transform + "' does not accept type '" + *rule.match_type + "'"); + } + std::lock_guard lock(mutex_); + rules_.push_back(std::move(rule)); + return {}; +} + +tl::expected TransformSet::append_type_rule( + const std::string& match_type, const std::string& transform) { + Rule rule; + rule.match_type = match_type; + rule.transform = transform; + rule.params = nlohmann::json::object(); + return add_rule(std::move(rule)); +} + +std::shared_ptr TransformSet::bind(const std::string& topic, const std::string& source_type) { + std::lock_guard lock(mutex_); + if (auto it = bindings_.find(topic); it != bindings_.end()) { + if (it->second->source_type == source_type) { + return it->second; + } + bindings_.erase(it); // the topic's type changed: match again + } + + for (const auto& rule : rules_) { + if (rule.match_type && *rule.match_type != source_type) { + continue; + } + if (rule.match_topic && !std::regex_match(topic, *rule.match_topic)) { + continue; + } + const auto& factory = factories_.at(rule.transform); + if (!factory.accepts(source_type)) { + if (warned_topics_.insert(topic).second) { + spdlog::warn( + "Transform '{}' matched topic '{}' but does not accept type '{}'; leaving it untransformed", + rule.transform, topic, source_type); + } + return nullptr; + } + auto bound = std::make_shared(); + bound->source_type = source_type; + bound->transform_name = rule.transform; + bound->output_type = factory.output_type(source_type); + bound->transform = factory.create(source_type, rule.params); + bindings_[topic] = bound; + spdlog::info("Topic '{}' ({}) -> transform '{}' -> {}", topic, source_type, rule.transform, bound->output_type); + return bound; + } + return nullptr; +} + +std::shared_ptr TransformSet::find(const std::string& topic) const { + std::lock_guard lock(mutex_); + auto it = bindings_.find(topic); + return it == bindings_.end() ? nullptr : it->second; +} + +std::string TransformSet::output_schema(const BoundTransform& bound, const std::string& source_schema) const { + return factories_.at(bound.transform_name).output_schema(bound.source_type, source_schema); +} + +std::string TransformSet::stats_summary() const { + std::lock_guard lock(mutex_); + std::ostringstream os; + for (const auto& [topic, bound] : bindings_) { + const uint64_t n = bound->samples.load(); + if (n == 0 && bound->drops.load() == 0) { + continue; + } + const double in = static_cast(bound->in_bytes.load()); + const double out = static_cast(bound->out_bytes.load()); + os << "\n " << topic << " [" << bound->transform_name << "]: " << n << " samples, " << bound->drops.load() + << " drops, ratio " << (out > 0 ? in / out : 0.0) << ", " << (n ? bound->micros.load() / n : 0) + << " us/sample"; + } + return os.str(); +} + +} // namespace pj_bridge +``` + +`factories_` is written only in `create()` before the object is shared, so reading it without the lock in `output_schema()` is safe. + +- [ ] **Step 5:** Build, run tests `TransformSet*`. Expected: 6 + 10 tests PASS. +- [ ] **Step 6:** Commit: `feat(transform): TransformSet profile parsing, matching and counters`. + +--- + +### Task 4: TransformingTopicSource + informational protocol fields + +**Files:** Create `app/include/pj_bridge/transforming_topic_source.hpp`, `app/src/transforming_topic_source.cpp`, `tests/unit/test_transforming_topic_source.cpp`. Modify `app/include/pj_bridge/topic_source_interface.hpp`, `app/src/bridge_server.cpp:281`, `app/include/pj_bridge/protocol_constants.hpp`, `tests/unit/test_protocol_constants.cpp`, `CMakeLists.txt`. + +- [ ] **Step 1:** Add to `TopicInfo`: + +```cpp + std::string source_type; ///< set only for transformed topics: the type before the transform +``` + +- [ ] **Step 2: Failing test** + +```cpp +#include + +#include "pj_bridge/transforming_topic_source.hpp" + +using namespace pj_bridge; + +namespace { +class StubSource : public TopicSourceInterface { + public: + std::vector get_topics() override { + return {{"/cloud", "pkg/msg/In", ""}, {"/odom", "pkg/msg/Odom", ""}}; + } + std::string get_schema(const std::string& topic) override { + return "schema-of-" + topic; + } + std::string schema_encoding() const override { + return "ros2msg"; + } + bool is_transient_local(const std::string& topic) const override { + return topic == "/cloud"; + } +}; + +class PassThrough : public MessageTransform { + public: + tl::expected apply(std::span in, std::vector& out) override { + out.assign(in.begin(), in.end()); + return {}; + } +}; + +std::shared_ptr make_set() { + TransformFactory f; + f.accepts = [](const std::string& t) { return t == "pkg/msg/In"; }; + f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; + f.output_type = [](const std::string&) { return std::string("pkg/msg/Out"); }; + f.output_schema = [](const std::string&, const std::string& s) { return "OUT:" + s; }; + f.create = [](const std::string&, const nlohmann::json&) { return std::make_unique(); }; + nlohmann::json profile = {{"transforms", {{{"match_type", "pkg/msg/In"}, {"transform", "t"}}}}}; + return TransformSet::create(profile, {{"t", f}}).value(); +} +} // namespace + +TEST(TransformingTopicSourceTest, RewritesTypeAndSchemaOnlyForMatchedTopics) { + TransformingTopicSource source(std::make_shared(), make_set()); + + auto topics = source.get_topics(); + ASSERT_EQ(topics.size(), 2u); + EXPECT_EQ(topics[0].type, "pkg/msg/Out"); + EXPECT_EQ(topics[0].source_type, "pkg/msg/In"); + EXPECT_EQ(topics[1].type, "pkg/msg/Odom"); + EXPECT_TRUE(topics[1].source_type.empty()); + + EXPECT_EQ(source.get_schema("/cloud"), "OUT:schema-of-/cloud"); + EXPECT_EQ(source.get_schema("/odom"), "schema-of-/odom"); + EXPECT_EQ(source.schema_encoding(), "ros2msg"); + EXPECT_TRUE(source.is_transient_local("/cloud")); +} +``` + +- [ ] **Step 3:** Build. Expected: compile error, `transforming_topic_source.hpp` not found. + +- [ ] **Step 4: Implementation** + +Header: + +```cpp +#pragma once + +#include + +#include "pj_bridge/topic_source_interface.hpp" +#include "pj_bridge/transform_set.hpp" + +namespace pj_bridge { + +/// Advertises transformed topics with their OUTPUT type and schema, so clients +/// never see the source type. Thread safety: same as the wrapped source. +class TransformingTopicSource : public TopicSourceInterface { + public: + TransformingTopicSource(std::shared_ptr inner, std::shared_ptr transforms); + + std::vector get_topics() override; + std::string get_schema(const std::string& topic_name) override; + std::string schema_encoding() const override; + bool is_transient_local(const std::string& topic_name) const override; + + private: + std::shared_ptr inner_; + std::shared_ptr transforms_; +}; + +} // namespace pj_bridge +``` + +Source: + +```cpp +#include "pj_bridge/transforming_topic_source.hpp" + +namespace pj_bridge { + +TransformingTopicSource::TransformingTopicSource( + std::shared_ptr inner, std::shared_ptr transforms) + : inner_(std::move(inner)), transforms_(std::move(transforms)) {} + +std::vector TransformingTopicSource::get_topics() { + auto topics = inner_->get_topics(); + for (auto& topic : topics) { + if (auto bound = transforms_->bind(topic.name, topic.type)) { + topic.source_type = topic.type; + topic.type = bound->output_type; + } + } + return topics; +} + +std::string TransformingTopicSource::get_schema(const std::string& topic_name) { + auto schema = inner_->get_schema(topic_name); + if (auto bound = transforms_->find(topic_name)) { + return transforms_->output_schema(*bound, schema); + } + return schema; +} + +std::string TransformingTopicSource::schema_encoding() const { + return inner_->schema_encoding(); +} + +bool TransformingTopicSource::is_transient_local(const std::string& topic_name) const { + return inner_->is_transient_local(topic_name); +} + +} // namespace pj_bridge +``` + +- [ ] **Step 5:** In `app/src/bridge_server.cpp`, right after `topic_entry["type"] = topic.type;` (line ~281): + +```cpp + if (!topic.source_type.empty()) { + topic_entry["source_type"] = topic.source_type; + } +``` + +`topics_changed` entries (line ~824) are built from a name→type map and do not carry `source_type` in v1; say so in `docs/API.md` (Task 8). + +- [ ] **Step 6:** Append to `kServerCapabilities`: `"message_transforms", // topics may be advertised with a transformed type (+ source_type)`. If `test_protocol_constants.cpp` asserts the list or its size, update it. +- [ ] **Step 7:** Any aggregate initialisation `TopicInfo{name, type}` still compiles (the new member is last and defaulted). Build, run the whole suite. Expected: all PASS. +- [ ] **Step 8:** Commit: `feat(transform): advertise transformed type and schema`. + +--- + +### Task 5: ROS2 hook and the `strip` transform + +**Files:** Create `ros2/include/pj_bridge_ros2/strip_transform.hpp`, `ros2/src/strip_transform.cpp`. Modify `ros2/include/pj_bridge_ros2/message_stripper.hpp`, `ros2/src/message_stripper.cpp`, `ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp`, `ros2/src/ros2_subscription_manager.cpp`, `tests/unit/test_ros2_subscription_manager.cpp`, `CMakeLists.txt`. + +- [ ] **Step 1:** Expose the stripper's type list. In `message_stripper.hpp` add to the class: + +```cpp + static const std::unordered_set& strippable_types(); +``` + +(`#include `), and in the `.cpp`: `const std::unordered_set& MessageStripper::strippable_types() { return kStrippableTypes; }`. + +- [ ] **Step 2:** `strip_transform.hpp`: + +```cpp +#pragma once + +#include "pj_bridge/message_transform.hpp" + +namespace pj_bridge { + +/// `strip` transform: MessageStripper behind the MessageTransform interface. +/// Output type and schema equal the input's. Takes no params. +TransformFactory make_strip_transform_factory(); + +} // namespace pj_bridge +``` + +`strip_transform.cpp`: + +```cpp +#include "pj_bridge_ros2/strip_transform.hpp" + +#include + +#include "pj_bridge_ros2/message_stripper.hpp" + +namespace pj_bridge { +namespace { + +class StripTransform : public MessageTransform { + public: + explicit StripTransform(std::string type) : type_(std::move(type)) {} + + tl::expected apply(std::span in, std::vector& out) override { + try { + // MessageStripper needs a SerializedMessage: one copy of the input. + rclcpp::SerializedMessage input(in.size()); + auto& rcl_in = input.get_rcl_serialized_message(); + std::memcpy(rcl_in.buffer, in.data(), in.size()); + rcl_in.buffer_length = in.size(); + + const auto stripped = MessageStripper::strip(type_, input); + const auto& rcl_out = stripped.get_rcl_serialized_message(); + const auto* bytes = reinterpret_cast(rcl_out.buffer); + out.assign(bytes, bytes + rcl_out.buffer_length); + return {}; + } catch (const std::exception& e) { + return tl::make_unexpected(std::string(e.what())); + } + } + + private: + std::string type_; +}; + +} // namespace + +TransformFactory make_strip_transform_factory() { + TransformFactory f; + f.accepts = [](const std::string& type) { return MessageStripper::should_strip(type); }; + f.check_params = [](const nlohmann::json& params) -> tl::expected { + if (!params.empty()) { + return tl::make_unexpected(std::string("takes no params")); + } + return {}; + }; + f.output_type = [](const std::string& type) { return type; }; + f.output_schema = [](const std::string&, const std::string& schema) { return schema; }; + f.create = [](const std::string& type, const nlohmann::json&) { return std::make_unique(type); }; + return f; +} + +} // namespace pj_bridge +``` + +- [ ] **Step 3:** Change `Ros2SubscriptionManager`. Constructor becomes: + +```cpp + explicit Ros2SubscriptionManager( + rclcpp::Node::SharedPtr node, std::shared_ptr transforms = nullptr, size_t min_qos_depth = 1, + size_t max_qos_depth = 100); +``` + +Replace the member `bool strip_large_messages_;` with `std::shared_ptr transforms_;`, include `pj_bridge/transform_set.hpp`, drop the `message_stripper.hpp` include. Update the class comment: "...and applies the topic's transform, if any, on the rcl buffer before copying." + +- [ ] **Step 4: Failing test.** First read `tests/unit/test_ros2_subscription_manager.cpp`: every construction passing `true`/`false` as the second argument must change. Replace the strip-specific tests' setup with a `TransformSet` holding the strip factory, and add one new test. Helper + test (adapt the publisher/spin scaffolding to what that file already uses): + +```cpp +std::shared_ptr strip_everything() { + auto set = pj_bridge::TransformSet::create( + nlohmann::json{{"transforms", nlohmann::json::array()}}, + {{"strip", pj_bridge::make_strip_transform_factory()}}) + .value(); + for (const auto& type : pj_bridge::MessageStripper::strippable_types()) { + EXPECT_TRUE(set->append_type_rule(type, "strip").has_value()); + } + return set; +} + +TEST_F(Ros2SubscriptionManagerTest, SubscribesWithSourceTypeAndForwardsTransformedBytes) { + // A transform whose output type differs from the source type: the manager is + // asked to subscribe with the ADVERTISED type and must use the source type. + pj_bridge::TransformFactory f; + f.accepts = [](const std::string& t) { return t == "std_msgs/msg/String"; }; + f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; + f.output_type = [](const std::string&) { return std::string("fake_msgs/msg/Out"); }; + f.output_schema = [](const std::string&, const std::string& s) { return s; }; + f.create = [](const std::string&, const nlohmann::json&) -> std::unique_ptr { + struct Marker : pj_bridge::MessageTransform { + tl::expected apply(std::span, std::vector& out) override { + out.assign(3, std::byte{0x5A}); + return {}; + } + }; + return std::make_unique(); + }; + auto set = pj_bridge::TransformSet::create( + nlohmann::json{{"transforms", {{{"match_topic", "/transform_me"}, {"transform", "marker"}}}}}, + {{"marker", f}}) + .value(); + ASSERT_NE(set->bind("/transform_me", "std_msgs/msg/String"), nullptr); // what get_topics would have done + + pj_bridge::Ros2SubscriptionManager manager(node_, set); + std::vector received; + manager.set_message_callback( + [&](const std::string&, std::shared_ptr> data, uint64_t) { received = *data; }); + + ASSERT_TRUE(manager.subscribe("/transform_me", "fake_msgs/msg/Out")); + // publish one std_msgs/msg/String on /transform_me and spin until `received` is non-empty, + // using the same publish-and-spin helper the neighbouring tests use. + publish_string_and_spin("/transform_me", "hello", [&] { return !received.empty(); }); + + EXPECT_EQ(received, std::vector(3, std::byte{0x5A})); +} +``` + +If the file has no publish-and-spin helper, add one next to the fixture: + +```cpp + void publish_string_and_spin(const std::string& topic, const std::string& text, const std::function& done) { + auto pub = node_->create_publisher(topic, 10); + std_msgs::msg::String msg; + msg.data = text; + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!done() && std::chrono::steady_clock::now() < deadline) { + pub->publish(msg); + rclcpp::spin_some(node_); + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + } + } +``` + +- [ ] **Step 5:** Build. Expected: FAIL (constructor signature, behaviour). + +- [ ] **Step 6: Implement the hook.** `subscribe()` becomes: + +```cpp +bool Ros2SubscriptionManager::subscribe(const std::string& topic_name, const std::string& topic_type) { + // A transformed topic is advertised with its output type; subscribe with the real one. + std::shared_ptr bound = transforms_ ? transforms_->find(topic_name) : nullptr; + const std::string& source_type = bound ? bound->source_type : topic_type; + + Ros2MessageCallback ros2_callback = + [this, bound]( + const std::string& topic, const std::shared_ptr& msg, uint64_t receive_time_ns) { + const auto& rcl_msg = msg->get_rcl_serialized_message(); + const auto* bytes = reinterpret_cast(rcl_msg.buffer); + + std::shared_ptr> data; + if (bound) { + data = std::make_shared>(); + if (!bound->run(topic, {bytes, rcl_msg.buffer_length}, *data)) { + return; // dropped and counted; never forward the untransformed bytes + } + } else { + data = std::make_shared>(bytes, bytes + rcl_msg.buffer_length); + } + + MessageCallback cb; + { + std::lock_guard lock(callback_mutex_); + cb = callback_; + } + if (cb) { + cb(topic, std::move(data), receive_time_ns); + } + }; + + return inner_manager_.subscribe(topic_name, source_type, ros2_callback); +} +``` + +Constructor: `: inner_manager_(node, min_qos_depth, max_qos_depth), transforms_(std::move(transforms)) {}`. Remove `#include ` if now unused. + +- [ ] **Step 7:** Add `ros2/src/strip_transform.cpp` to `pj_bridge_ros2_lib`. `ros2/src/main.cpp` will not compile yet (it passes a bool): change that call temporarily to pass `nullptr` so the tree builds; Task 6 wires it properly. +- [ ] **Step 8:** Build, run tests `Ros2SubscriptionManager*:MessageStripper*`. Expected: PASS. +- [ ] **Step 9:** Commit: `feat(transform): ROS2 ingest hook; strip becomes a transform`. + +--- + +### Task 6: Wire `transform_profile` and `strip_large_messages` in main + +**Files:** Modify `ros2/src/main.cpp`. + +- [ ] **Step 1:** Next to the other `declare_parameter` calls: `node->declare_parameter("transform_profile", "");` and read it into `std::string transform_profile`. Add it to the "Configuration:" log line. +- [ ] **Step 2:** Add includes: ``, `"pj_bridge/transform_set.hpp"`, `"pj_bridge/transforming_topic_source.hpp"`, `"pj_bridge_ros2/strip_transform.hpp"`, `"pj_bridge_ros2/message_stripper.hpp"`. +- [ ] **Step 3:** Inside the `try`, replace the `topic_source` / `sub_manager` construction with: + +```cpp + std::shared_ptr topic_source = std::make_shared(node); + + std::shared_ptr transforms; + if (!transform_profile.empty() || strip_large_messages) { + nlohmann::json profile = {{"transforms", nlohmann::json::array()}}; + if (!transform_profile.empty()) { + std::ifstream file(transform_profile); + if (!file) { + RCLCPP_ERROR(node->get_logger(), "Cannot open transform_profile '%s'", transform_profile.c_str()); + rclcpp::shutdown(); + return 1; + } + profile = nlohmann::json::parse(file, nullptr, /*allow_exceptions=*/false); + if (profile.is_discarded()) { + RCLCPP_ERROR(node->get_logger(), "transform_profile '%s' is not valid JSON", transform_profile.c_str()); + rclcpp::shutdown(); + return 1; + } + } + + pj_bridge::TransformSet::FactoryMap factories; + factories["strip"] = pj_bridge::make_strip_transform_factory(); + + auto created = pj_bridge::TransformSet::create(profile, std::move(factories)); + if (!created) { + RCLCPP_ERROR(node->get_logger(), "%s", created.error().c_str()); + rclcpp::shutdown(); + return 1; + } + transforms = *created; + + if (strip_large_messages) { // after the profile's rules, so an explicit rule wins + for (const auto& type : pj_bridge::MessageStripper::strippable_types()) { + transforms->append_type_rule(type, "strip"); + } + } + topic_source = std::make_shared(topic_source, transforms); + } + + auto sub_manager = std::make_shared( + node, transforms, static_cast(min_qos_depth), static_cast(max_qos_depth)); +``` + +Check that `BridgeServer`'s constructor takes `std::shared_ptr` (it does today via implicit conversion from `shared_ptr`). + +- [ ] **Step 4:** After the "Final statistics" log: + +```cpp + if (transforms) { + const auto summary = transforms->stats_summary(); + if (!summary.empty()) { + RCLCPP_INFO(node->get_logger(), "Transform statistics:%s", summary.c_str()); + } + } +``` + +- [ ] **Step 5: Manual check.** Build. Then, in three terminals under `pixi run -e humble` with `install/setup.bash` sourced: + 1. `pj_bridge_ros2` with `--ros-args -p strip_large_messages:=true` → log shows no error; with `-p transform_profile:=/nonexistent.json` → exits 1 with "Cannot open"; with a file containing `{"transforms":[{"match_type":"sensor_msgs/msg/Image","transform":"nope"}]}` → exits 1 naming the unknown transform. + 2. Whole test suite still passes. +- [ ] **Step 6:** Commit: `feat(transform): transform_profile parameter; strip_large_messages as sugar`. + +--- + +### Task 7: Cloudini transform + +**Files:** Create `app/include/pj_bridge/cloudini_transform.hpp`, `app/src/cloudini_transform.cpp`, `tests/unit/test_cloudini_transform.cpp`. Modify `CMakeLists.txt`, `ros2/src/main.cpp`, `package.xml`. + +- [ ] **Step 1: CMake.** After the IXWebSocket block: + +```cmake +# Cloudini (optional): point cloud compression transform +option(PJ_BRIDGE_FETCH_CLOUDINI "Fetch Cloudini when it is not installed" ON) +find_package(cloudini_lib QUIET) +if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) + include(FetchContent) + set(CLOUDINI_BUILD_TOOLS OFF CACHE BOOL "" FORCE) + set(CLOUDINI_BUILD_BENCHMARKS OFF CACHE BOOL "" FORCE) + FetchContent_Declare(cloudini + URL https://github.com/facontidavide/cloudini/archive/refs/tags/1.3.0.tar.gz + URL_HASH SHA256=26c9322d5a08694b9b24bbec975b7814b05cf7d4962327f35266b38bb15b5e66 + SOURCE_SUBDIR cloudini_lib + EXCLUDE_FROM_ALL) + # Cloudini switches to a SHARED ament package (install rules, ament_package()) + # when it finds ament_cmake. Hide it for the subdirectory: we want the plain + # static library. Scoped to Cloudini's directory; our own ament state is untouched. + set(BUILD_TESTING_SAVED ${BUILD_TESTING}) + set(BUILD_TESTING OFF) + set(CMAKE_DISABLE_FIND_PACKAGE_ament_cmake ON) + FetchContent_MakeAvailable(cloudini) + set(CMAKE_DISABLE_FIND_PACKAGE_ament_cmake OFF) + set(BUILD_TESTING ${BUILD_TESTING_SAVED}) + if(NOT TARGET cloudini::cloudini_lib) + add_library(cloudini::cloudini_lib ALIAS cloudini_lib) + endif() + set_target_properties(cloudini_lib PROPERTIES POSITION_INDEPENDENT_CODE ON) + set(cloudini_lib_FOUND TRUE) +endif() +``` + +and, after `pj_bridge_app` is defined: + +```cmake +if(cloudini_lib_FOUND) + target_sources(pj_bridge_app PRIVATE app/src/cloudini_transform.cpp) + target_link_libraries(pj_bridge_app PRIVATE cloudini::cloudini_lib) + target_compile_definitions(pj_bridge_app PUBLIC PJ_BRIDGE_HAS_CLOUDINI) + message(STATUS "Cloudini transform: enabled") +else() + message(STATUS "Cloudini transform: disabled (cloudini_lib not found)") +endif() +``` + +The hash above was computed from the 1.3.0 tag tarball on 2026-09-20. After configuring, confirm in the CMake output that Cloudini did **not** take its ament branch (`grep -c "ament" build/pj_bridge/_deps/cloudini-build/CMakeCache.txt` should show no `ament_cmake_DIR` pointing to a real path) and that `libcloudini_lib.a` (static) was produced. The pixi environments ship CMake 4.x, so `EXCLUDE_FROM_ALL` in `FetchContent_Declare` (needs >= 3.28) is fine there; the Humble CI container has an older CMake — if configure fails on that keyword, remove it and wrap `FetchContent_MakeAvailable` with `set(CMAKE_SKIP_INSTALL_RULES ON)` / `OFF`. If the static `cloudini_lib` is linked PRIVATE into the static `pj_bridge_app`, the final executables still need it on their link line — CMake propagates this automatically for static libraries (`$`); confirm `pj_bridge_ros2` links. + +- [ ] **Step 2:** `cloudini_transform.hpp`: + +```cpp +#pragma once + +#include "pj_bridge/message_transform.hpp" + +namespace pj_bridge { + +/// `cloudini` transform: sensor_msgs PointCloud2 -> point_cloud_interfaces +/// CompressedPointCloud2. Works on CDR bytes, so it serves any backend whose +/// PointCloud2 is CDR-encoded. Only declared when Cloudini is available. +/// +/// Params: resolution (float, default 0.001), fields ({name: resolution}, 0 +/// removes the field), viz_preprocessing (bool, default false). +TransformFactory make_cloudini_transform_factory(); + +} // namespace pj_bridge +``` + +- [ ] **Step 3: Failing smoke test** — `tests/unit/test_cloudini_transform.cpp`, whole file inside `#ifdef PJ_BRIDGE_HAS_CLOUDINI`: + +```cpp +#ifdef PJ_BRIDGE_HAS_CLOUDINI + +#include + +#include +#include +#include + +#include "pj_bridge/cloudini_transform.hpp" + +using namespace pj_bridge; + +namespace { +std::vector make_cloud_cdr(size_t points) { + sensor_msgs::msg::PointCloud2 cloud; + cloud.header.frame_id = "lidar"; + sensor_msgs::PointCloud2Modifier modifier(cloud); + modifier.setPointCloud2FieldsByString(1, "xyz"); + modifier.resize(points); + sensor_msgs::PointCloud2Iterator x(cloud, "x"), y(cloud, "y"), z(cloud, "z"); + for (size_t i = 0; i < points; ++i, ++x, ++y, ++z) { + *x = 0.01f * static_cast(i); + *y = 1.0f; + *z = -0.5f; + } + rclcpp::SerializedMessage serialized; + rclcpp::Serialization().serialize_message(&cloud, &serialized); + const auto& rcl = serialized.get_rcl_serialized_message(); + const auto* bytes = reinterpret_cast(rcl.buffer); + return {bytes, bytes + rcl.buffer_length}; +} +} // namespace + +TEST(CloudiniTransformTest, CompressesPointCloud2) { + auto factory = make_cloudini_transform_factory(); + EXPECT_TRUE(factory.accepts("sensor_msgs/msg/PointCloud2")); + EXPECT_FALSE(factory.accepts("sensor_msgs/msg/Image")); + EXPECT_EQ(factory.output_type("sensor_msgs/msg/PointCloud2"), "point_cloud_interfaces/msg/CompressedPointCloud2"); + EXPECT_NE(factory.output_schema("sensor_msgs/msg/PointCloud2", "").find("format"), std::string::npos); + + const nlohmann::json params = {{"resolution", 0.001}}; + ASSERT_TRUE(factory.check_params(params).has_value()); + EXPECT_FALSE(factory.check_params({{"resolutoin", 0.001}}).has_value()); + EXPECT_FALSE(factory.check_params({{"resolution", -1.0}}).has_value()); + + auto transform = factory.create("sensor_msgs/msg/PointCloud2", params); + const auto in = make_cloud_cdr(10000); + std::vector out; + ASSERT_TRUE(transform->apply(in, out).has_value()); + EXPECT_FALSE(out.empty()); + EXPECT_LT(out.size(), in.size()); + + ASSERT_TRUE(transform->apply(in, out).has_value()); // instance is reusable +} + +TEST(CloudiniTransformTest, GarbageInputIsAnErrorNotACrash) { + auto transform = make_cloudini_transform_factory().create("sensor_msgs/msg/PointCloud2", nlohmann::json::object()); + const std::vector in(7, std::byte{0xFF}); + std::vector out; + EXPECT_FALSE(transform->apply(in, out).has_value()); +} + +#endif // PJ_BRIDGE_HAS_CLOUDINI +``` + +Add the file to the test target unconditionally (it is empty without the define). Build: expected link error for `make_cloudini_transform_factory`. + +- [ ] **Step 4: Implementation** — `app/src/cloudini_transform.cpp` + +```cpp +#include "pj_bridge/cloudini_transform.hpp" + +#include +#include +// Defines non-inline `const char*` globals: include in this translation unit ONLY. +#include + +namespace pj_bridge { +namespace { + +constexpr const char* kPointCloud2 = "sensor_msgs/msg/PointCloud2"; + +class CloudiniTransform : public MessageTransform { + public: + explicit CloudiniTransform(const nlohmann::json& params) + : resolution_(params.value("resolution", 0.001f)), viz_preprocessing_(params.value("viz_preprocessing", false)) { + if (params.contains("fields")) { + for (auto it = params["fields"].begin(); it != params["fields"].end(); ++it) { + profile_[it.key()] = it->get(); + } + } + } + + tl::expected apply(std::span in, std::vector& out) override { + try { + const Cloudini::ConstBufferView raw(reinterpret_cast(in.data()), in.size()); + auto cloud = cloudini_ros::getDeserializedPointCloudMessage(raw); + cloudini_ros::applyResolutionProfile(profile_, cloud.fields, resolution_); + if (viz_preprocessing_) { + cloudini_ros::applyVizLossyPreprocessing(cloud); + } + auto info = cloudini_ros::toEncodingInfo(cloud); + // The frame is zstd-compressed by the bridge: a second stage here would compress twice. + info.compression_opt = Cloudini::CompressionOption::NONE; + info.use_threads = false; + + // Cloudini writes CDR into a vector; scratch_ keeps its capacity across calls. + cloudini_ros::convertPointCloud2ToCompressedCloud(cloud, info, scratch_); + const auto* bytes = reinterpret_cast(scratch_.data()); + out.assign(bytes, bytes + scratch_.size()); + return {}; + } catch (const std::exception& e) { + return tl::make_unexpected(std::string(e.what())); + } + } + + private: + float resolution_; + bool viz_preprocessing_; + cloudini_ros::ResolutionProfile profile_; + std::vector scratch_; +}; + +tl::expected check_cloudini_params(const nlohmann::json& params) { + for (auto it = params.begin(); it != params.end(); ++it) { + const auto& key = it.key(); + if (key == "resolution") { + if (!it->is_number() || it->get() <= 0.0) { + return tl::make_unexpected(std::string("'resolution' must be a number > 0")); + } + } else if (key == "viz_preprocessing") { + if (!it->is_boolean()) { + return tl::make_unexpected(std::string("'viz_preprocessing' must be a boolean")); + } + } else if (key == "fields") { + if (!it->is_object()) { + return tl::make_unexpected(std::string("'fields' must be an object of {name: resolution}")); + } + for (auto f = it->begin(); f != it->end(); ++f) { + if (!f->is_number() || f->get() < 0.0) { + return tl::make_unexpected("'fields." + f.key() + "' must be a number >= 0 (0 removes the field)"); + } + } + } else { + return tl::make_unexpected("unknown param '" + key + "'"); + } + } + return {}; +} + +} // namespace + +TransformFactory make_cloudini_transform_factory() { + TransformFactory f; + f.accepts = [](const std::string& type) { return type == kPointCloud2; }; + f.check_params = check_cloudini_params; + f.output_type = [](const std::string&) { return std::string(compressed_schema_name); }; + f.output_schema = [](const std::string&, const std::string&) { return std::string(compressed_schema_data); }; + f.create = [](const std::string&, const nlohmann::json& params) { + return std::make_unique(params); + }; + return f; +} + +} // namespace pj_bridge +``` + +Verify against the installed headers before building: the namespace of `compressed_schema_name` / `compressed_schema_data` in `ros_message_definitions.hpp`, the spelling `Cloudini::CompressionOption::NONE`, and that `toEncodingInfo` leaves `encoding_opt` at `LOSSY`. Known cost, accepted for v1: `convertPointCloud2ToCompressedCloud` builds a `PointcloudEncoder` per call and the result is copied once (compressed size) from `scratch_` into `out`. Removing both needs a small Cloudini API addition (byte-generic output, reusable encoder) — out of scope here. + +- [ ] **Step 5:** Register it in `ros2/src/main.cpp` next to the strip factory: + +```cpp +#ifdef PJ_BRIDGE_HAS_CLOUDINI + factories["cloudini"] = pj_bridge::make_cloudini_transform_factory(); +#endif +``` + +with `#ifdef PJ_BRIDGE_HAS_CLOUDINI` / `#include "pj_bridge/cloudini_transform.hpp"` / `#endif` among the includes. Without Cloudini, a profile naming `cloudini` already fails at startup with "unknown transform 'cloudini' (not built in, or misspelled)". + +- [ ] **Step 6:** Build for Humble and Jazzy, run tests `CloudiniTransform*`. Expected: 2 PASS. Then the whole suite. +- [ ] **Step 7:** `package.xml`: do **not** add `cloudini_lib` until the key resolves on every target distro (check `rosdep resolve cloudini_lib --rosdistro humble|jazzy|lyrical`); record the result in the commit body. +- [ ] **Step 8:** Commit: `feat(transform): Cloudini point cloud compression`. + +--- + +### Task 8: Documentation + +**Files:** Modify `docs/API.md`, `README.md`, `CLAUDE.md`, `CHANGELOG.rst`. Create `docs/transform_profile.example.json`. + +- [ ] **Step 1:** `docs/transform_profile.example.json`: + +```json +{ + "transforms": [ + { + "match_type": "sensor_msgs/msg/PointCloud2", + "transform": "cloudini", + "params": {"resolution": 0.001, "viz_preprocessing": false} + } + ] +} +``` + +- [ ] **Step 2:** `docs/API.md`: new section "Message transforms" covering: operator-only and off by default; a transformed topic is advertised with its **output** type and schema and the optional `source_type` (absent from `topics_changed` entries in this version); the `message_transforms` capability; the profile format, matching rules and first-match-wins; the two transforms and their params; every startup error; runtime failures drop the sample; `strip_large_messages: true` equals strip rules appended after the profile; clients need a `CompressedPointCloud2`-capable parser; Cloudini is lossy at `resolution`. +- [ ] **Step 3:** `README.md` and `CLAUDE.md`: add `transform_profile: ""` to the ROS2 configuration block; CLAUDE.md gets C++20, the new components (TransformSet, TransformingTopicSource, the hook replacing the stripper call), Cloudini as an optional dependency, and the updated test count. +- [ ] **Step 4:** `CHANGELOG.rst`: Forthcoming section listing the feature, C++20, and the `strip` runtime-failure behaviour change (drop instead of forwarding the original). +- [ ] **Step 5:** `pre-commit run -a`; commit: `docs: message transforms`. + +--- + +### Task 9: Acceptance measurement + +Uses the pinned rig and scripts described in `docs/perf/README.md` (bridge on cores 4-7, player 12-19, client 8-11, governor `performance`, 20 s windows from `/proc//stat`). Large temporary files go on disk, never in `/tmp` (tmpfs). + +- [ ] **Step 1:** 4-lidar bag, one client subscribed to all topics, three runs each: (a) no profile, (b) `docs/transform_profile.example.json`, (c) same with `viz_preprocessing: true`. +- [ ] **Step 2:** Record per configuration: bridge CPU (% of one core), wire bytes/s from the client, client-side per-topic message counts, and the shutdown "Transform statistics" (ratio, µs/sample). +- [ ] **Step 3:** Pass criteria: per-topic message counts in (b) and (c) equal (a) within one publish period; no transform drops. If counts fall short, ingest is being starved by encode time: stop and report — the fallback (per-topic worker, 1-deep latest-wins slot) is a separate design step, do not improvise it. +- [ ] **Step 4:** End to end: PlotJuggler built from `pj-official-plugins` branch `feat/compressed-pointcloud` connects, subscribes to a lidar topic and renders the cloud. +- [ ] **Step 5:** Add the numbers to `docs/perf/README.md`; commit: `docs(perf): message transform measurements`. diff --git a/docs/superpowers/specs/2026-09-20-message-transforms-design.md b/docs/superpowers/specs/2026-09-20-message-transforms-design.md index 67b6874..0b3f358 100644 --- a/docs/superpowers/specs/2026-09-20-message-transforms-design.md +++ b/docs/superpowers/specs/2026-09-20-message-transforms-design.md @@ -87,17 +87,14 @@ class MessageTransform { std::span in, std::vector& out) = 0; }; -struct TransformOutputType { - std::string type_name; - std::string schema; -}; - struct TransformFactory { std::function accepts; - std::function output; - std::function, std::string>( - const nlohmann::json& params)> create; + std::function(const nlohmann::json& params)> check_params; + std::function output_type; + std::function output_schema; + std::function(const std::string& source_type, + const nlohmann::json& params)> create; }; ``` @@ -118,13 +115,14 @@ no downstream change) if measurement justifies it. Not built in v1. ### `cloudini` (`app/`, compiled only when Cloudini is available) `getDeserializedPointCloudMessage` → `applyResolutionProfile` → optional -`applyVizLossyPreprocessing` → encode. Encoding uses the lower-level -`PointcloudEncoder::encode(ConstBufferView, BufferView)` + -`writePointCloudHeader` so the result is written directly into the -`std::vector` (the convenience -`convertPointCloud2ToCompressedCloud` writes a `vector`, which would -cost an extra output copy). Second stage `NONE`. The encoder and the -preprocessing buffer are members, reused while the field layout is unchanged. +`applyVizLossyPreprocessing` → `toEncodingInfo` → +`convertPointCloud2ToCompressedCloud`. Cloudini's CDR writer (`nanocdr`) only +targets `std::vector`, so the transform encodes into a member scratch +vector (capacity kept across calls) and copies the result — at compressed size +— into `out`. v1 accepts that copy and the per-call `PointcloudEncoder` +construction inside the convenience function; removing both needs a small +Cloudini API addition (byte-generic output, reusable encoder), tracked +separately. Second stage `NONE`. Output schema comes from Cloudini's embedded `ros_message_definitions.hpp`, so `point_cloud_interfaces` need not be installed. @@ -181,9 +179,11 @@ for uniformity). ## Informational protocol additions (ignored by old clients) - `message_transforms` in the server capabilities list. -- Optional `"source_type"` on `get_topics` entries of transformed topics. -- Per-transform counters in the 5 s stats line: in bytes, out bytes, drops, - mean µs per sample. +- Optional `"source_type"` on `get_topics` entries of transformed topics + (not on `topics_changed` entries in v1). +- Per-topic transform counters (samples, drops, compression ratio, mean µs per + sample) logged with the final statistics at shutdown; the ROS2 entry point + has no periodic stats line today. ## Build From 94eb9447bb1dc1bc4d373fd11fa2eec7c3c9de0a Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:16:43 +0200 Subject: [PATCH 03/19] build: require C++20 std::span is needed by the message transform interface, and cloudini_lib exports cxx_std_20 publicly, so the standard must not depend on whether that optional dependency is found. Humble and Jazzy (pixi) build warning-free, 271/271 tests pass on both. FastDDS standalone not rebuilt here. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5e75a07..56429b5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -15,7 +15,7 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") add_compile_options(-Wall -Wextra -Wpedantic) endif() -set(CMAKE_CXX_STANDARD 17) +set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED ON) # Sanitizer options From 44f5d98e782369cc83ccacb7ddf2ca2ff4f61b87 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:20:37 +0200 Subject: [PATCH 04/19] feat(transform): MessageTransform interface Adds the MessageTransform abstract interface and TransformFactory struct that later tasks build on: TransformSet (profile parsing and matching), the ROS2 ingest hook, and the strip/cloudini transforms. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- app/include/pj_bridge/message_transform.hpp | 60 +++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 app/include/pj_bridge/message_transform.hpp diff --git a/app/include/pj_bridge/message_transform.hpp b/app/include/pj_bridge/message_transform.hpp new file mode 100644 index 0000000..25c08bb --- /dev/null +++ b/app/include/pj_bridge/message_transform.hpp @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +#include "tl/expected.hpp" + +namespace pj_bridge { + +/// One instance per topic, called from a single thread (the ingest executor). +/// Implementations SHOULD keep codec contexts and scratch buffers as members. +class MessageTransform { + public: + virtual ~MessageTransform() = default; + + /// `in` views the middleware's buffer and is valid only during the call. + /// `out` is the final storage later held by MessageBuffer: clear and fill it. + /// On error the caller DROPS the sample. It must never forward `in` instead: + /// the client was told the output type at subscribe time. + virtual tl::expected apply(std::span in, std::vector& out) = 0; +}; + +struct TransformFactory { + /// Can this transform handle messages of `source_type`? + std::function accepts; + /// Reject unknown or malformed params. Called once per rule at startup. + std::function(const nlohmann::json& params)> check_params; + /// Type name advertised to clients in place of `source_type`. + std::function output_type; + /// Schema advertised to clients. `source_schema` is the untransformed one. + std::function output_schema; + std::function(const std::string& source_type, const nlohmann::json& params)> + create; +}; + +} // namespace pj_bridge From 2e5cc43cf7100d94b067dd0f0973695f2f147525 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:23:48 +0200 Subject: [PATCH 05/19] feat(transform): TransformSet profile parsing, matching and counters TransformSet parses a JSON transform profile into ordered match_type / match_topic rules bound to named factories, validating every rule at startup (unknown transform, rejected params, invalid regex, a type the transform declares it doesn't accept). bind() applies first-match-wins and caches the per-topic BoundTransform, rebinding when a topic's source type changes; BoundTransform::run() applies the transform and tracks samples/drops/bytes/latency counters for stats_summary(). Also reformats app/include/pj_bridge/message_transform.hpp per clang-format (pre-commit was not run before the previous commit). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CMakeLists.txt | 2 + app/include/pj_bridge/message_transform.hpp | 3 +- app/include/pj_bridge/transform_set.hpp | 99 ++++++++++ app/src/transform_set.cpp | 190 ++++++++++++++++++++ tests/unit/test_transform_set.cpp | 152 ++++++++++++++++ 5 files changed, 444 insertions(+), 2 deletions(-) create mode 100644 app/include/pj_bridge/transform_set.hpp create mode 100644 app/src/transform_set.cpp create mode 100644 tests/unit/test_transform_set.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 56429b5..9f6bd05 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -113,6 +113,7 @@ add_library(pj_bridge_app STATIC app/src/bridge_server.cpp app/src/standalone_event_loop.cpp app/src/whitelist_filter.cpp + app/src/transform_set.cpp ) target_include_directories(pj_bridge_app PUBLIC @@ -266,6 +267,7 @@ if(BUILD_TESTING AND ament_cmake_FOUND) tests/unit/test_bridge_server.cpp tests/unit/test_protocol_constants.cpp tests/unit/test_whitelist_filter.cpp + tests/unit/test_transform_set.cpp tests/unit/test_topic_discovery.cpp tests/unit/test_schema_extractor.cpp tests/unit/test_generic_subscription_manager.cpp diff --git a/app/include/pj_bridge/message_transform.hpp b/app/include/pj_bridge/message_transform.hpp index 25c08bb..fae1ba5 100644 --- a/app/include/pj_bridge/message_transform.hpp +++ b/app/include/pj_bridge/message_transform.hpp @@ -53,8 +53,7 @@ struct TransformFactory { std::function output_type; /// Schema advertised to clients. `source_schema` is the untransformed one. std::function output_schema; - std::function(const std::string& source_type, const nlohmann::json& params)> - create; + std::function(const std::string& source_type, const nlohmann::json& params)> create; }; } // namespace pj_bridge diff --git a/app/include/pj_bridge/transform_set.hpp b/app/include/pj_bridge/transform_set.hpp new file mode 100644 index 0000000..e8c0322 --- /dev/null +++ b/app/include/pj_bridge/transform_set.hpp @@ -0,0 +1,99 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "pj_bridge/message_transform.hpp" + +namespace pj_bridge { + +/// A transform bound to one topic, plus its counters. +/// run() is called from the ingest thread only; counters are read from others. +struct BoundTransform { + std::string source_type; + std::string transform_name; + std::string output_type; + std::unique_ptr transform; + + std::atomic samples{0}; + std::atomic drops{0}; + std::atomic in_bytes{0}; + std::atomic out_bytes{0}; + std::atomic micros{0}; + + /// Applies the transform and updates counters. False = drop the sample. + bool run(const std::string& topic, std::span in, std::vector& out); +}; + +/// Ordered transform rules plus the per-topic bindings created from them. +/// Thread safety: all public methods are safe to call concurrently. +class TransformSet { + public: + using FactoryMap = std::map; + + /// Parse and validate a profile. Any problem is a startup error. + static tl::expected, std::string> create( + const nlohmann::json& profile, FactoryMap factories); + + /// Append a `match_type` rule after the profile's rules (strip_large_messages sugar). + tl::expected append_type_rule(const std::string& match_type, const std::string& transform); + + /// First matching rule wins. Creates and remembers the binding on first sight. + /// Returns nullptr when no rule applies to this topic. + std::shared_ptr bind(const std::string& topic, const std::string& source_type); + + /// Binding previously created by bind(), or nullptr. + std::shared_ptr find(const std::string& topic) const; + + /// Schema to advertise for a bound topic. + std::string output_schema(const BoundTransform& bound, const std::string& source_schema) const; + + /// One line per bound topic that processed at least one sample. + std::string stats_summary() const; + + private: + struct Rule { + std::optional match_type; + std::optional match_topic; + std::string transform; + nlohmann::json params; + }; + + TransformSet() = default; + tl::expected add_rule(Rule rule); + + FactoryMap factories_; + std::vector rules_; + mutable std::mutex mutex_; + std::unordered_map> bindings_; + std::set warned_topics_; +}; + +} // namespace pj_bridge diff --git a/app/src/transform_set.cpp b/app/src/transform_set.cpp new file mode 100644 index 0000000..fe179ab --- /dev/null +++ b/app/src/transform_set.cpp @@ -0,0 +1,190 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include "pj_bridge/transform_set.hpp" + +#include + +#include +#include + +namespace pj_bridge { + +bool BoundTransform::run(const std::string& topic, std::span in, std::vector& out) { + const auto start = std::chrono::steady_clock::now(); + auto result = transform->apply(in, out); + if (!result) { + const uint64_t n = drops.fetch_add(1) + 1; + if ((n & (n - 1)) == 0) { // 1st, 2nd, 4th, 8th... keeps a broken topic from flooding the log + spdlog::warn("Transform '{}' failed on '{}' ({} drops so far): {}", transform_name, topic, n, result.error()); + } + return false; + } + const auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + samples.fetch_add(1); + in_bytes.fetch_add(in.size()); + out_bytes.fetch_add(out.size()); + micros.fetch_add(static_cast(elapsed.count())); + return true; +} + +tl::expected, std::string> TransformSet::create( + const nlohmann::json& profile, FactoryMap factories) { + if (!profile.is_object() || !profile.contains("transforms") || !profile["transforms"].is_array()) { + return tl::make_unexpected(std::string("transform profile must be an object with a 'transforms' array")); + } + for (auto it = profile.begin(); it != profile.end(); ++it) { + if (it.key() != "transforms") { + return tl::make_unexpected("transform profile: unknown key '" + it.key() + "'"); + } + } + + std::shared_ptr set(new TransformSet()); + set->factories_ = std::move(factories); + + size_t index = 0; + for (const auto& entry : profile["transforms"]) { + const std::string where = "transform profile rule " + std::to_string(index++) + ": "; + if (!entry.is_object()) { + return tl::make_unexpected(where + "must be an object"); + } + Rule rule; + for (auto it = entry.begin(); it != entry.end(); ++it) { + const auto& key = it.key(); + if (key == "match_type" && it->is_string()) { + rule.match_type = it->get(); + } else if (key == "match_topic" && it->is_string()) { + try { + rule.match_topic.emplace(it->get(), std::regex::ECMAScript); + } catch (const std::regex_error& e) { + return tl::make_unexpected(where + "invalid match_topic regex: " + e.what()); + } + } else if (key == "transform" && it->is_string()) { + rule.transform = it->get(); + } else if (key == "params" && it->is_object()) { + rule.params = *it; + } else { + return tl::make_unexpected(where + "unknown or mistyped key '" + key + "'"); + } + } + if (rule.params.is_null()) { + rule.params = nlohmann::json::object(); + } + if (auto added = set->add_rule(std::move(rule)); !added) { + return tl::make_unexpected(where + added.error()); + } + } + return set; +} + +tl::expected TransformSet::add_rule(Rule rule) { + if (!rule.match_type && !rule.match_topic) { + return tl::make_unexpected(std::string("needs match_type and/or match_topic")); + } + if (rule.transform.empty()) { + return tl::make_unexpected(std::string("missing 'transform'")); + } + auto factory = factories_.find(rule.transform); + if (factory == factories_.end()) { + return tl::make_unexpected("unknown transform '" + rule.transform + "' (not built in, or misspelled)"); + } + if (auto ok = factory->second.check_params(rule.params); !ok) { + return tl::make_unexpected("transform '" + rule.transform + "': " + ok.error()); + } + if (rule.match_type && !factory->second.accepts(*rule.match_type)) { + return tl::make_unexpected("transform '" + rule.transform + "' does not accept type '" + *rule.match_type + "'"); + } + std::lock_guard lock(mutex_); + rules_.push_back(std::move(rule)); + return {}; +} + +tl::expected TransformSet::append_type_rule( + const std::string& match_type, const std::string& transform) { + Rule rule; + rule.match_type = match_type; + rule.transform = transform; + rule.params = nlohmann::json::object(); + return add_rule(std::move(rule)); +} + +std::shared_ptr TransformSet::bind(const std::string& topic, const std::string& source_type) { + std::lock_guard lock(mutex_); + if (auto it = bindings_.find(topic); it != bindings_.end()) { + if (it->second->source_type == source_type) { + return it->second; + } + bindings_.erase(it); // the topic's type changed: match again + } + + for (const auto& rule : rules_) { + if (rule.match_type && *rule.match_type != source_type) { + continue; + } + if (rule.match_topic && !std::regex_match(topic, *rule.match_topic)) { + continue; + } + const auto& factory = factories_.at(rule.transform); + if (!factory.accepts(source_type)) { + if (warned_topics_.insert(topic).second) { + spdlog::warn( + "Transform '{}' matched topic '{}' but does not accept type '{}'; leaving it untransformed", rule.transform, + topic, source_type); + } + return nullptr; + } + auto bound = std::make_shared(); + bound->source_type = source_type; + bound->transform_name = rule.transform; + bound->output_type = factory.output_type(source_type); + bound->transform = factory.create(source_type, rule.params); + bindings_[topic] = bound; + spdlog::info("Topic '{}' ({}) -> transform '{}' -> {}", topic, source_type, rule.transform, bound->output_type); + return bound; + } + return nullptr; +} + +std::shared_ptr TransformSet::find(const std::string& topic) const { + std::lock_guard lock(mutex_); + auto it = bindings_.find(topic); + return it == bindings_.end() ? nullptr : it->second; +} + +std::string TransformSet::output_schema(const BoundTransform& bound, const std::string& source_schema) const { + return factories_.at(bound.transform_name).output_schema(bound.source_type, source_schema); +} + +std::string TransformSet::stats_summary() const { + std::lock_guard lock(mutex_); + std::ostringstream os; + for (const auto& [topic, bound] : bindings_) { + const uint64_t n = bound->samples.load(); + if (n == 0 && bound->drops.load() == 0) { + continue; + } + const double in = static_cast(bound->in_bytes.load()); + const double out = static_cast(bound->out_bytes.load()); + os << "\n " << topic << " [" << bound->transform_name << "]: " << n << " samples, " << bound->drops.load() + << " drops, ratio " << (out > 0 ? in / out : 0.0) << ", " << (n ? bound->micros.load() / n : 0) << " us/sample"; + } + return os.str(); +} + +} // namespace pj_bridge diff --git a/tests/unit/test_transform_set.cpp b/tests/unit/test_transform_set.cpp new file mode 100644 index 0000000..942c383 --- /dev/null +++ b/tests/unit/test_transform_set.cpp @@ -0,0 +1,152 @@ +#include + +#include "pj_bridge/transform_set.hpp" + +using namespace pj_bridge; +using nlohmann::json; + +namespace { + +/// Appends one marker byte to the input. `fail` makes apply() return an error. +class FakeTransform : public MessageTransform { + public: + explicit FakeTransform(bool fail) : fail_(fail) {} + tl::expected apply(std::span in, std::vector& out) override { + if (fail_) { + return tl::make_unexpected(std::string("boom")); + } + out.assign(in.begin(), in.end()); + out.push_back(std::byte{0xAB}); + return {}; + } + + private: + bool fail_; +}; + +TransformFactory fake_factory(const std::string& accepted_type) { + TransformFactory f; + f.accepts = [accepted_type](const std::string& t) { return t == accepted_type; }; + f.check_params = [](const json& p) -> tl::expected { + for (auto it = p.begin(); it != p.end(); ++it) { + if (it.key() != "fail") { + return tl::make_unexpected("unknown param '" + it.key() + "'"); + } + } + return {}; + }; + f.output_type = [](const std::string&) { return std::string("pkg/msg/Out"); }; + f.output_schema = [](const std::string&, const std::string& s) { return "OUT:" + s; }; + f.create = [](const std::string&, const json& p) { return std::make_unique(p.value("fail", false)); }; + return f; +} + +TransformSet::FactoryMap factories() { + return {{"fake", fake_factory("pkg/msg/In")}, {"other", fake_factory("pkg/msg/Other")}}; +} + +json rule(json fields) { + return json{{"transforms", json::array({std::move(fields)})}}; +} + +} // namespace + +TEST(TransformSetTest, MatchesByType) { + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + auto bound = (*set)->bind("/a", "pkg/msg/In"); + ASSERT_NE(bound, nullptr); + EXPECT_EQ(bound->source_type, "pkg/msg/In"); + EXPECT_EQ(bound->output_type, "pkg/msg/Out"); + EXPECT_EQ((*set)->bind("/b", "pkg/msg/Unrelated"), nullptr); + EXPECT_EQ((*set)->find("/a"), bound); + EXPECT_EQ((*set)->find("/b"), nullptr); +} + +TEST(TransformSetTest, TopicRegexIsFullMatchAndCombinesWithType) { + auto set = TransformSet::create( + rule({{"match_type", "pkg/msg/In"}, {"match_topic", "/lidar/.*"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + EXPECT_NE((*set)->bind("/lidar/front", "pkg/msg/In"), nullptr); + EXPECT_EQ((*set)->bind("/x/lidar/front", "pkg/msg/In"), nullptr); // full match, not search + EXPECT_EQ((*set)->bind("/lidar/rear", "pkg/msg/Unrelated"), nullptr); +} + +TEST(TransformSetTest, FirstMatchingRuleWinsAndAppendedRulesComeLast) { + json profile = { + {"transforms", json::array( + {{{"match_topic", "/a"}, {"transform", "fake"}, {"params", {{"fail", true}}}}, + {{"match_type", "pkg/msg/In"}, {"transform", "fake"}}})}}; + auto set = TransformSet::create(profile, factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + ASSERT_TRUE((*set)->append_type_rule("pkg/msg/Other", "other").has_value()); + + std::vector in{std::byte{1}}, out; + EXPECT_FALSE((*set)->bind("/a", "pkg/msg/In")->run("/a", in, out)); // rule 0 (fail=true) + EXPECT_TRUE((*set)->bind("/b", "pkg/msg/In")->run("/b", in, out)); // rule 1 + EXPECT_EQ((*set)->bind("/c", "pkg/msg/Other")->transform_name, "other"); +} + +TEST(TransformSetTest, TopicOnlyRuleWithNonAcceptedTypeLeavesTopicUntransformed) { + auto set = TransformSet::create(rule({{"match_topic", "/a"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + EXPECT_EQ((*set)->bind("/a", "pkg/msg/Unrelated"), nullptr); +} + +TEST(TransformSetTest, RebindsWhenSourceTypeChanges) { + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + EXPECT_NE((*set)->bind("/a", "pkg/msg/In"), nullptr); + EXPECT_EQ((*set)->bind("/a", "pkg/msg/Unrelated"), nullptr); + EXPECT_EQ((*set)->find("/a"), nullptr); +} + +TEST(TransformSetTest, RunCountsSamplesBytesAndDrops) { + json profile = { + {"transforms", json::array( + {{{"match_topic", "/bad"}, {"transform", "fake"}, {"params", {{"fail", true}}}}, + {{"match_type", "pkg/msg/In"}, {"transform", "fake"}}})}}; + auto set = TransformSet::create(profile, factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + std::vector in(10, std::byte{7}), out; + + auto good = (*set)->bind("/good", "pkg/msg/In"); + ASSERT_TRUE(good->run("/good", in, out)); + EXPECT_EQ(out.size(), 11u); + EXPECT_EQ(good->samples.load(), 1u); + EXPECT_EQ(good->in_bytes.load(), 10u); + EXPECT_EQ(good->out_bytes.load(), 11u); + EXPECT_EQ(good->drops.load(), 0u); + + auto bad = (*set)->bind("/bad", "pkg/msg/In"); + EXPECT_FALSE(bad->run("/bad", in, out)); + EXPECT_EQ(bad->drops.load(), 1u); + EXPECT_EQ(bad->samples.load(), 0u); +} + +struct BadProfile { + const char* name; + json profile; +}; + +class TransformSetBadProfileTest : public ::testing::TestWithParam {}; + +TEST_P(TransformSetBadProfileTest, IsRejectedAtStartup) { + auto set = TransformSet::create(GetParam().profile, factories()); + EXPECT_FALSE(set.has_value()); +} + +INSTANTIATE_TEST_SUITE_P( + All, TransformSetBadProfileTest, + ::testing::Values( + BadProfile{"not_an_object", json::array()}, BadProfile{"missing_transforms", json::object()}, + BadProfile{"unknown_top_level_key", json{{"transforms", json::array()}, {"extra", 1}}}, + BadProfile{"rule_without_matcher", rule({{"transform", "fake"}})}, + BadProfile{"rule_without_transform", rule({{"match_type", "pkg/msg/In"}})}, + BadProfile{"unknown_rule_key", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"oops", 1}})}, + BadProfile{"unknown_transform", rule({{"match_type", "pkg/msg/In"}, {"transform", "nope"}})}, + BadProfile{"invalid_regex", rule({{"match_topic", "("}, {"transform", "fake"}})}, + BadProfile{ + "unknown_param", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"params", {{"q", 1}}}})}, + BadProfile{"type_not_accepted", rule({{"match_type", "pkg/msg/Unrelated"}, {"transform", "fake"}})}), + [](const auto& info) { return std::string(info.param.name); }); From 6936698185dd7bf395cfe2957084136aadf5ff58 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:27:51 +0200 Subject: [PATCH 06/19] feat(transform): advertise transformed type and schema Adds TransformingTopicSource, a TopicSourceInterface decorator that rewrites a matched topic's advertised type/schema to the transform's output, tagging the original type on TopicInfo::source_type (new, defaulted, last member). BridgeServer emits source_type on get_topics entries when non-empty and advertises the message_transforms capability. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CMakeLists.txt | 2 + app/include/pj_bridge/protocol_constants.hpp | 1 + .../pj_bridge/topic_source_interface.hpp | 5 +- .../pj_bridge/transforming_topic_source.hpp | 45 +++++++++++ app/src/bridge_server.cpp | 3 + app/src/transforming_topic_source.cpp | 55 +++++++++++++ tests/unit/test_bridge_server.cpp | 28 +++++++ tests/unit/test_transforming_topic_source.cpp | 77 +++++++++++++++++++ 8 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 app/include/pj_bridge/transforming_topic_source.hpp create mode 100644 app/src/transforming_topic_source.cpp create mode 100644 tests/unit/test_transforming_topic_source.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 9f6bd05..c477f03 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -114,6 +114,7 @@ add_library(pj_bridge_app STATIC app/src/standalone_event_loop.cpp app/src/whitelist_filter.cpp app/src/transform_set.cpp + app/src/transforming_topic_source.cpp ) target_include_directories(pj_bridge_app PUBLIC @@ -268,6 +269,7 @@ if(BUILD_TESTING AND ament_cmake_FOUND) tests/unit/test_protocol_constants.cpp tests/unit/test_whitelist_filter.cpp tests/unit/test_transform_set.cpp + tests/unit/test_transforming_topic_source.cpp tests/unit/test_topic_discovery.cpp tests/unit/test_schema_extractor.cpp tests/unit/test_generic_subscription_manager.cpp diff --git a/app/include/pj_bridge/protocol_constants.hpp b/app/include/pj_bridge/protocol_constants.hpp index af57f3c..7342c9d 100644 --- a/app/include/pj_bridge/protocol_constants.hpp +++ b/app/include/pj_bridge/protocol_constants.hpp @@ -65,6 +65,7 @@ inline constexpr const char* kServerCapabilities[] = { "topics_changed", // pushed topic advertisement (subscribe_topic_updates) "per_topic_rate_limit", // subscribe entries accept {name, max_rate_hz} "size_class_frames", // large topics isolated into own frames (header flag bit0 = heavy) + "message_transforms", // topics may be advertised with a transformed type (+ source_type) }; } // namespace pj_bridge diff --git a/app/include/pj_bridge/topic_source_interface.hpp b/app/include/pj_bridge/topic_source_interface.hpp index 947cadf..3185056 100644 --- a/app/include/pj_bridge/topic_source_interface.hpp +++ b/app/include/pj_bridge/topic_source_interface.hpp @@ -26,8 +26,9 @@ namespace pj_bridge { /// A discovered topic with its fully-qualified name and message type. struct TopicInfo { - std::string name; ///< e.g. "/sensor/imu" - std::string type; ///< e.g. "sensor_msgs/msg/Imu" + std::string name; ///< e.g. "/sensor/imu" + std::string type; ///< e.g. "sensor_msgs/msg/Imu" + std::string source_type{}; ///< set only for transformed topics: the type before the transform }; /// Abstract interface for discovering topics and retrieving their schemas. diff --git a/app/include/pj_bridge/transforming_topic_source.hpp b/app/include/pj_bridge/transforming_topic_source.hpp new file mode 100644 index 0000000..a556a26 --- /dev/null +++ b/app/include/pj_bridge/transforming_topic_source.hpp @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include + +#include "pj_bridge/topic_source_interface.hpp" +#include "pj_bridge/transform_set.hpp" + +namespace pj_bridge { + +/// Advertises transformed topics with their OUTPUT type and schema, so clients +/// never see the source type. Thread safety: same as the wrapped source. +class TransformingTopicSource : public TopicSourceInterface { + public: + TransformingTopicSource(std::shared_ptr inner, std::shared_ptr transforms); + + std::vector get_topics() override; + std::string get_schema(const std::string& topic_name) override; + std::string schema_encoding() const override; + bool is_transient_local(const std::string& topic_name) const override; + + private: + std::shared_ptr inner_; + std::shared_ptr transforms_; +}; + +} // namespace pj_bridge diff --git a/app/src/bridge_server.cpp b/app/src/bridge_server.cpp index fe3551e..b1fce8f 100644 --- a/app/src/bridge_server.cpp +++ b/app/src/bridge_server.cpp @@ -279,6 +279,9 @@ std::string BridgeServer::handle_get_topics(const std::string& client_id, const json topic_entry; topic_entry["name"] = topic.name; topic_entry["type"] = topic.type; + if (!topic.source_type.empty()) { + topic_entry["source_type"] = topic.source_type; + } attach_latched_badge(topic_entry, topic.name); if (include_schemas) { attach_schema_fields(topic_entry, topic.name); diff --git a/app/src/transforming_topic_source.cpp b/app/src/transforming_topic_source.cpp new file mode 100644 index 0000000..6ff9847 --- /dev/null +++ b/app/src/transforming_topic_source.cpp @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include "pj_bridge/transforming_topic_source.hpp" + +namespace pj_bridge { + +TransformingTopicSource::TransformingTopicSource( + std::shared_ptr inner, std::shared_ptr transforms) + : inner_(std::move(inner)), transforms_(std::move(transforms)) {} + +std::vector TransformingTopicSource::get_topics() { + auto topics = inner_->get_topics(); + for (auto& topic : topics) { + if (auto bound = transforms_->bind(topic.name, topic.type)) { + topic.source_type = topic.type; + topic.type = bound->output_type; + } + } + return topics; +} + +std::string TransformingTopicSource::get_schema(const std::string& topic_name) { + auto schema = inner_->get_schema(topic_name); + if (auto bound = transforms_->find(topic_name)) { + return transforms_->output_schema(*bound, schema); + } + return schema; +} + +std::string TransformingTopicSource::schema_encoding() const { + return inner_->schema_encoding(); +} + +bool TransformingTopicSource::is_transient_local(const std::string& topic_name) const { + return inner_->is_transient_local(topic_name); +} + +} // namespace pj_bridge diff --git a/tests/unit/test_bridge_server.cpp b/tests/unit/test_bridge_server.cpp index 7d0b62e..13d6582 100644 --- a/tests/unit/test_bridge_server.cpp +++ b/tests/unit/test_bridge_server.cpp @@ -2754,6 +2754,34 @@ TEST_F(BridgeServerTest, GetTopicsMarksLatchedTopics) { } } +// GetTopicsCarriesSourceTypeOnlyForTransformedTopics +// +// A topic whose TopicInfo::source_type is set (i.e. it was rewritten by a +// TransformingTopicSource) carries `source_type` in its get_topics entry; +// an untransformed topic (empty source_type) carries no such key. +TEST_F(BridgeServerTest, GetTopicsCarriesSourceTypeOnlyForTransformedTopics) { + ASSERT_TRUE(server_->initialize()); + mock_topic_source_->set_topics( + {{"/cloud", "point_cloud_interfaces/msg/CompressedPointCloud2", "sensor_msgs/msg/PointCloud2"}, + {"/imu", "sensor_msgs/msg/Imu"}}); + + json req; + req["command"] = "get_topics"; + mock_->push_request("client_source_type", req.dump()); + server_->process_requests(); + + auto replies = mock_->get_replies("client_source_type"); + ASSERT_EQ(replies.size(), 1u); + for (const auto& entry : replies[0]["topics"]) { + if (entry["name"] == "/cloud") { + ASSERT_TRUE(entry.contains("source_type")); + EXPECT_EQ(entry["source_type"], "sensor_msgs/msg/PointCloud2"); + } else { + EXPECT_FALSE(entry.contains("source_type")) << entry.dump(); + } + } +} + // TopicsChangedAddedEntriesCarryLatched // // A late-appearing transient-local topic is badged in the topics_changed push diff --git a/tests/unit/test_transforming_topic_source.cpp b/tests/unit/test_transforming_topic_source.cpp new file mode 100644 index 0000000..a4578bd --- /dev/null +++ b/tests/unit/test_transforming_topic_source.cpp @@ -0,0 +1,77 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include + +#include "pj_bridge/transforming_topic_source.hpp" + +using namespace pj_bridge; + +namespace { +class StubSource : public TopicSourceInterface { + public: + std::vector get_topics() override { + return {{"/cloud", "pkg/msg/In", ""}, {"/odom", "pkg/msg/Odom", ""}}; + } + std::string get_schema(const std::string& topic) override { + return "schema-of-" + topic; + } + std::string schema_encoding() const override { + return "ros2msg"; + } + bool is_transient_local(const std::string& topic) const override { + return topic == "/cloud"; + } +}; + +class PassThrough : public MessageTransform { + public: + tl::expected apply(std::span in, std::vector& out) override { + out.assign(in.begin(), in.end()); + return {}; + } +}; + +std::shared_ptr make_set() { + TransformFactory f; + f.accepts = [](const std::string& t) { return t == "pkg/msg/In"; }; + f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; + f.output_type = [](const std::string&) { return std::string("pkg/msg/Out"); }; + f.output_schema = [](const std::string&, const std::string& s) { return "OUT:" + s; }; + f.create = [](const std::string&, const nlohmann::json&) { return std::make_unique(); }; + nlohmann::json profile = {{"transforms", {{{"match_type", "pkg/msg/In"}, {"transform", "t"}}}}}; + return TransformSet::create(profile, {{"t", f}}).value(); +} +} // namespace + +TEST(TransformingTopicSourceTest, RewritesTypeAndSchemaOnlyForMatchedTopics) { + TransformingTopicSource source(std::make_shared(), make_set()); + + auto topics = source.get_topics(); + ASSERT_EQ(topics.size(), 2u); + EXPECT_EQ(topics[0].type, "pkg/msg/Out"); + EXPECT_EQ(topics[0].source_type, "pkg/msg/In"); + EXPECT_EQ(topics[1].type, "pkg/msg/Odom"); + EXPECT_TRUE(topics[1].source_type.empty()); + + EXPECT_EQ(source.get_schema("/cloud"), "OUT:schema-of-/cloud"); + EXPECT_EQ(source.get_schema("/odom"), "schema-of-/odom"); + EXPECT_EQ(source.schema_encoding(), "ros2msg"); + EXPECT_TRUE(source.is_transient_local("/cloud")); +} From fcf25882117d4ba1e2af5878779ec55ed9cf430f Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:29:09 +0200 Subject: [PATCH 07/19] fix(transform): contain transform exceptions; skip non-accepting rules Review findings on TransformSet: an exception from apply() or a factory, or a null transform, now costs a sample / leaves the topic untransformed instead of unwinding into the ingest executor. A topic-only rule whose transform does not accept the type is skipped rather than ending the search, so later rules (the strip_large_messages sugar) still apply. Adds caching, stats, validation and two-thread tests. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- app/include/pj_bridge/transform_set.hpp | 1 + app/src/transform_set.cpp | 59 ++++++++----- .../2026-09-20-message-transforms-design.md | 10 ++- tests/unit/test_transform_set.cpp | 86 +++++++++++++++++++ 4 files changed, 131 insertions(+), 25 deletions(-) diff --git a/app/include/pj_bridge/transform_set.hpp b/app/include/pj_bridge/transform_set.hpp index e8c0322..70774ca 100644 --- a/app/include/pj_bridge/transform_set.hpp +++ b/app/include/pj_bridge/transform_set.hpp @@ -20,6 +20,7 @@ #pragma once #include +#include #include #include #include diff --git a/app/src/transform_set.cpp b/app/src/transform_set.cpp index fe179ab..9cc61e8 100644 --- a/app/src/transform_set.cpp +++ b/app/src/transform_set.cpp @@ -23,12 +23,18 @@ #include #include +#include namespace pj_bridge { bool BoundTransform::run(const std::string& topic, std::span in, std::vector& out) { const auto start = std::chrono::steady_clock::now(); - auto result = transform->apply(in, out); + tl::expected result; + try { + result = transform->apply(in, out); + } catch (const std::exception& e) { // a codec throwing on bad input costs one sample, not the bridge + result = tl::make_unexpected(std::string("exception: ") + e.what()); + } if (!result) { const uint64_t n = drops.fetch_add(1) + 1; if ((n & (n - 1)) == 0) { // 1st, 2nd, 4th, 8th... keeps a broken topic from flooding the log @@ -133,30 +139,41 @@ std::shared_ptr TransformSet::bind(const std::string& topic, con bindings_.erase(it); // the topic's type changed: match again } + // ponytail: unmatched topics re-run every rule on each call (once per topic poll); + // cache the misses too if topics x rules ever shows up in a profile. for (const auto& rule : rules_) { - if (rule.match_type && *rule.match_type != source_type) { - continue; - } - if (rule.match_topic && !std::regex_match(topic, *rule.match_topic)) { - continue; - } - const auto& factory = factories_.at(rule.transform); - if (!factory.accepts(source_type)) { - if (warned_topics_.insert(topic).second) { - spdlog::warn( - "Transform '{}' matched topic '{}' but does not accept type '{}'; leaving it untransformed", rule.transform, - topic, source_type); + try { + if (rule.match_type && *rule.match_type != source_type) { + continue; + } + if (rule.match_topic && !std::regex_match(topic, *rule.match_topic)) { + continue; + } + const auto& factory = factories_.at(rule.transform); + if (!factory.accepts(source_type)) { + if (warned_topics_.insert(topic).second) { + spdlog::warn( + "Transform '{}' matched topic '{}' but does not accept type '{}'; rule skipped", rule.transform, topic, + source_type); + } + continue; // a later rule (e.g. the strip_large_messages sugar) may still apply + } + auto bound = std::make_shared(); + bound->source_type = source_type; + bound->transform_name = rule.transform; + bound->output_type = factory.output_type(source_type); + bound->transform = factory.create(source_type, rule.params); + if (!bound->transform) { + throw std::runtime_error("factory returned no transform"); } + bindings_[topic] = bound; + spdlog::info("Topic '{}' ({}) -> transform '{}' -> {}", topic, source_type, rule.transform, bound->output_type); + return bound; + } catch (const std::exception& e) { + spdlog::error( + "Transform '{}' could not be set up for '{}': {}; leaving it untransformed", rule.transform, topic, e.what()); return nullptr; } - auto bound = std::make_shared(); - bound->source_type = source_type; - bound->transform_name = rule.transform; - bound->output_type = factory.output_type(source_type); - bound->transform = factory.create(source_type, rule.params); - bindings_[topic] = bound; - spdlog::info("Topic '{}' ({}) -> transform '{}' -> {}", topic, source_type, rule.transform, bound->output_type); - return bound; } return nullptr; } diff --git a/docs/superpowers/specs/2026-09-20-message-transforms-design.md b/docs/superpowers/specs/2026-09-20-message-transforms-design.md index 0b3f358..84bda96 100644 --- a/docs/superpowers/specs/2026-09-20-message-transforms-design.md +++ b/docs/superpowers/specs/2026-09-20-message-transforms-design.md @@ -166,11 +166,13 @@ ROS2 parameter `transform_profile` (string, default empty = disabled). **Fail at startup** (clear error, non-zero exit): unreadable or malformed file, unknown top-level/rule/param key, unknown transform name, `cloudini` -named in a build without Cloudini, invalid regex, `create()` returning an -error, a `match_type` the named transform does not `accept()`. A rule without +named in a build without Cloudini, invalid regex, params rejected by the +transform's `check_params`, a `match_type` the named transform does not `accept()`. A rule without `match_type` whose transform does not `accept()` a topic it matched by -`match_topic` is reported once as a warning and the topic is left -untransformed (types are only known at discovery time). +`match_topic` is reported once as a warning and that rule is skipped for the +topic — later rules still apply (types are only known at discovery time). A +transform that cannot be instantiated for a topic (factory throws or returns +nothing) is logged as an error and the topic is left untransformed. **At runtime**, `apply()` failure → drop the sample, bump a counter, throttled warning. This also applies to `strip` (today it forwards the original; changed diff --git a/tests/unit/test_transform_set.cpp b/tests/unit/test_transform_set.cpp index 942c383..4094dec 100644 --- a/tests/unit/test_transform_set.cpp +++ b/tests/unit/test_transform_set.cpp @@ -1,5 +1,9 @@ #include +#include +#include +#include + #include "pj_bridge/transform_set.hpp" using namespace pj_bridge; @@ -60,6 +64,7 @@ TEST(TransformSetTest, MatchesByType) { EXPECT_EQ(bound->output_type, "pkg/msg/Out"); EXPECT_EQ((*set)->bind("/b", "pkg/msg/Unrelated"), nullptr); EXPECT_EQ((*set)->find("/a"), bound); + EXPECT_EQ((*set)->bind("/a", "pkg/msg/In"), bound); // cached: counters and codec state survive topic polls EXPECT_EQ((*set)->find("/b"), nullptr); } @@ -93,6 +98,84 @@ TEST(TransformSetTest, TopicOnlyRuleWithNonAcceptedTypeLeavesTopicUntransformed) EXPECT_EQ((*set)->bind("/a", "pkg/msg/Unrelated"), nullptr); } +TEST(TransformSetTest, NonAcceptingRuleIsSkippedSoLaterRulesStillApply) { + auto set = TransformSet::create(rule({{"match_topic", "/lidar/.*"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + ASSERT_TRUE((*set)->append_type_rule("pkg/msg/Other", "other").has_value()); + auto bound = (*set)->bind("/lidar/image", "pkg/msg/Other"); + ASSERT_NE(bound, nullptr); + EXPECT_EQ(bound->transform_name, "other"); +} + +TEST(TransformSetTest, FactoryReturningNullOrThrowingLeavesTopicUntransformed) { + auto broken = fake_factory("pkg/msg/In"); + broken.create = [](const std::string&, const json& p) -> std::unique_ptr { + if (p.value("fail", false)) { + throw std::runtime_error("no codec"); + } + return nullptr; + }; + json profile = { + {"transforms", json::array( + {{{"match_topic", "/throws"}, {"transform", "broken"}, {"params", {{"fail", true}}}}, + {{"match_type", "pkg/msg/In"}, {"transform", "broken"}}})}}; + auto set = TransformSet::create(profile, {{"broken", broken}}); + ASSERT_TRUE(set.has_value()) << set.error(); + EXPECT_EQ((*set)->bind("/throws", "pkg/msg/In"), nullptr); + EXPECT_EQ((*set)->bind("/null", "pkg/msg/In"), nullptr); + EXPECT_EQ((*set)->find("/null"), nullptr); +} + +TEST(TransformSetTest, ThrowingApplyIsADropNotACrash) { + struct Thrower : MessageTransform { + tl::expected apply(std::span, std::vector&) override { + throw std::length_error("corrupt size field"); + } + }; + auto f = fake_factory("pkg/msg/In"); + f.create = [](const std::string&, const json&) { return std::make_unique(); }; + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "t"}}), {{"t", f}}); + ASSERT_TRUE(set.has_value()) << set.error(); + auto bound = (*set)->bind("/a", "pkg/msg/In"); + std::vector in(4), out; + EXPECT_FALSE(bound->run("/a", in, out)); + EXPECT_EQ(bound->drops.load(), 1u); +} + +TEST(TransformSetTest, StatsSummaryListsOnlyTopicsThatProcessedSamples) { + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + std::vector in(4), out; + (*set)->bind("/busy", "pkg/msg/In")->run("/busy", in, out); + (*set)->bind("/idle", "pkg/msg/In"); + const auto summary = (*set)->stats_summary(); + EXPECT_NE(summary.find("/busy"), std::string::npos); + EXPECT_EQ(summary.find("/idle"), std::string::npos); +} + +// Gives TSAN the real threading model: run() on one thread, everything else on another. +TEST(TransformSetTest, RunIsSafeConcurrentlyWithBindFindAndStats) { + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); + ASSERT_TRUE(set.has_value()) << set.error(); + auto bound = (*set)->bind("/a", "pkg/msg/In"); + std::atomic stop{false}; + std::thread ingest([&] { + std::vector in(64), out; + while (!stop) { + bound->run("/a", in, out); + } + }); + for (int i = 0; i < 2000; ++i) { + (*set)->bind("/a", "pkg/msg/In"); + (*set)->bind("/other" + std::to_string(i % 8), "pkg/msg/In"); + (*set)->find("/a"); + (*set)->stats_summary(); + } + stop = true; + ingest.join(); + EXPECT_GT(bound->samples.load(), 0u); +} + TEST(TransformSetTest, RebindsWhenSourceTypeChanges) { auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); ASSERT_TRUE(set.has_value()) << set.error(); @@ -141,6 +224,9 @@ INSTANTIATE_TEST_SUITE_P( ::testing::Values( BadProfile{"not_an_object", json::array()}, BadProfile{"missing_transforms", json::object()}, BadProfile{"unknown_top_level_key", json{{"transforms", json::array()}, {"extra", 1}}}, + BadProfile{"transforms_not_an_array", json{{"transforms", 5}}}, + BadProfile{"rule_not_an_object", json{{"transforms", json::array({5})}}}, + BadProfile{"mistyped_rule_key", rule({{"match_type", 5}, {"transform", "fake"}})}, BadProfile{"rule_without_matcher", rule({{"transform", "fake"}})}, BadProfile{"rule_without_transform", rule({{"match_type", "pkg/msg/In"}})}, BadProfile{"unknown_rule_key", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"oops", 1}})}, From ab92dca95f1d38b71f32cc11881fd484ac7242b9 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:34:07 +0200 Subject: [PATCH 08/19] feat(transform): ROS2 ingest hook; strip becomes a transform Ros2SubscriptionManager now takes a std::shared_ptr instead of a strip_large_messages bool. subscribe() looks up the topic's bound transform (created earlier via TransformSet::bind(), e.g. by TransformingTopicSource::get_topics()), subscribes with the transform's source_type instead of the advertised output type, and runs the transform on a span over the rcl buffer directly into a fresh output vector -- no copy of the input. On transform failure the sample is DROPPED instead of forwarding the original bytes, since the client was already told the output type at subscribe time. `strip` is now one MessageTransform, adapting MessageStripper. main.cpp wires it up in the next commit; for now it passes nullptr so the tree builds. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CMakeLists.txt | 1 + .../pj_bridge_ros2/message_stripper.hpp | 4 + .../ros2_subscription_manager.hpp | 11 ++- .../pj_bridge_ros2/strip_transform.hpp | 30 ++++++ ros2/src/main.cpp | 2 +- ros2/src/message_stripper.cpp | 4 + ros2/src/ros2_subscription_manager.cpp | 36 ++++---- ros2/src/strip_transform.cpp | 72 +++++++++++++++ tests/unit/test_ros2_subscription_manager.cpp | 92 +++++++++++++++++-- 9 files changed, 218 insertions(+), 34 deletions(-) create mode 100644 ros2/include/pj_bridge_ros2/strip_transform.hpp create mode 100644 ros2/src/strip_transform.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index c477f03..1b71d73 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -144,6 +144,7 @@ if(ament_cmake_FOUND) ros2/src/schema_extractor.cpp ros2/src/generic_subscription_manager.cpp ros2/src/message_stripper.cpp + ros2/src/strip_transform.cpp ros2/src/ros2_topic_source.cpp ros2/src/ros2_subscription_manager.cpp ) diff --git a/ros2/include/pj_bridge_ros2/message_stripper.hpp b/ros2/include/pj_bridge_ros2/message_stripper.hpp index b418a5e..c6f38ab 100644 --- a/ros2/include/pj_bridge_ros2/message_stripper.hpp +++ b/ros2/include/pj_bridge_ros2/message_stripper.hpp @@ -21,6 +21,7 @@ #include #include +#include namespace pj_bridge { @@ -40,6 +41,9 @@ class MessageStripper { public: static bool should_strip(const std::string& message_type); static rclcpp::SerializedMessage strip(const std::string& message_type, const rclcpp::SerializedMessage& input); + + /// The message types should_strip() accepts. + static const std::unordered_set& strippable_types(); }; } // namespace pj_bridge diff --git a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp index 6e1c8ef..a6b92d9 100644 --- a/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp +++ b/ros2/include/pj_bridge_ros2/ros2_subscription_manager.hpp @@ -19,12 +19,13 @@ #pragma once +#include #include #include #include "pj_bridge/subscription_manager_interface.hpp" +#include "pj_bridge/transform_set.hpp" #include "pj_bridge_ros2/generic_subscription_manager.hpp" -#include "pj_bridge_ros2/message_stripper.hpp" namespace pj_bridge { @@ -32,13 +33,13 @@ namespace pj_bridge { * @brief ROS2 implementation of SubscriptionManagerInterface * * Wraps GenericSubscriptionManager to provide backend-agnostic subscription - * management. Handles conversion from rclcpp::SerializedMessage to - * shared_ptr> and optional message stripping. + * management. Converts rclcpp::SerializedMessage to shared_ptr> + * and applies the topic's transform, if any, on the rcl buffer before copying. */ class Ros2SubscriptionManager : public SubscriptionManagerInterface { public: explicit Ros2SubscriptionManager( - rclcpp::Node::SharedPtr node, bool strip_large_messages = false, size_t min_qos_depth = 1, + rclcpp::Node::SharedPtr node, std::shared_ptr transforms = nullptr, size_t min_qos_depth = 1, size_t max_qos_depth = 100); Ros2SubscriptionManager(const Ros2SubscriptionManager&) = delete; @@ -54,7 +55,7 @@ class Ros2SubscriptionManager : public SubscriptionManagerInterface { private: GenericSubscriptionManager inner_manager_; - bool strip_large_messages_; + std::shared_ptr transforms_; std::mutex callback_mutex_; MessageCallback callback_; diff --git a/ros2/include/pj_bridge_ros2/strip_transform.hpp b/ros2/include/pj_bridge_ros2/strip_transform.hpp new file mode 100644 index 0000000..2b9021b --- /dev/null +++ b/ros2/include/pj_bridge_ros2/strip_transform.hpp @@ -0,0 +1,30 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include "pj_bridge/message_transform.hpp" + +namespace pj_bridge { + +/// `strip` transform: MessageStripper behind the MessageTransform interface. +/// Output type and schema equal the input's. Takes no params. +TransformFactory make_strip_transform_factory(); + +} // namespace pj_bridge diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 1de482e..fbe125c 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -145,7 +145,7 @@ int main(int argc, char** argv) { // Create backend components auto topic_source = std::make_shared(node); auto sub_manager = std::make_shared( - node, strip_large_messages, static_cast(min_qos_depth), static_cast(max_qos_depth)); + node, nullptr, static_cast(min_qos_depth), static_cast(max_qos_depth)); std::optional tls_config; if (tls_enabled) { tls_config = pj_bridge::TlsConfig{certfile, keyfile}; diff --git a/ros2/src/message_stripper.cpp b/ros2/src/message_stripper.cpp index 09af4cb..61c0cf9 100644 --- a/ros2/src/message_stripper.cpp +++ b/ros2/src/message_stripper.cpp @@ -54,6 +54,10 @@ bool MessageStripper::should_strip(const std::string& message_type) { return kStrippableTypes.find(message_type) != kStrippableTypes.end(); } +const std::unordered_set& MessageStripper::strippable_types() { + return kStrippableTypes; +} + rclcpp::SerializedMessage MessageStripper::strip( const std::string& message_type, const rclcpp::SerializedMessage& input) { if (message_type == "sensor_msgs/msg/Image") { diff --git a/ros2/src/ros2_subscription_manager.cpp b/ros2/src/ros2_subscription_manager.cpp index 3f806b9..4b3f710 100644 --- a/ros2/src/ros2_subscription_manager.cpp +++ b/ros2/src/ros2_subscription_manager.cpp @@ -22,13 +22,12 @@ #include #include -#include namespace pj_bridge { Ros2SubscriptionManager::Ros2SubscriptionManager( - rclcpp::Node::SharedPtr node, bool strip_large_messages, size_t min_qos_depth, size_t max_qos_depth) - : inner_manager_(node, min_qos_depth, max_qos_depth), strip_large_messages_(strip_large_messages) {} + rclcpp::Node::SharedPtr node, std::shared_ptr transforms, size_t min_qos_depth, size_t max_qos_depth) + : inner_manager_(node, min_qos_depth, max_qos_depth), transforms_(std::move(transforms)) {} void Ros2SubscriptionManager::set_message_callback(MessageCallback callback) { std::lock_guard lock(callback_mutex_); @@ -36,27 +35,26 @@ void Ros2SubscriptionManager::set_message_callback(MessageCallback callback) { } bool Ros2SubscriptionManager::subscribe(const std::string& topic_name, const std::string& topic_type) { - bool needs_stripping = strip_large_messages_ && MessageStripper::should_strip(topic_type); + // A transformed topic is advertised with its output type; subscribe with the real one. + std::shared_ptr bound = transforms_ ? transforms_->find(topic_name) : nullptr; + const std::string& source_type = bound ? bound->source_type : topic_type; Ros2MessageCallback ros2_callback = - [this, topic_type, needs_stripping]( + [this, bound]( const std::string& topic, const std::shared_ptr& msg, uint64_t receive_time_ns) { - const rclcpp::SerializedMessage* msg_to_use = msg.get(); - std::optional stripped_msg; - - if (needs_stripping) { - try { - stripped_msg.emplace(MessageStripper::strip(topic_type, *msg)); - msg_to_use = &*stripped_msg; - } catch (const std::exception& e) { - spdlog::warn("Failed to strip message on topic '{}': {}. Forwarding original.", topic, e.what()); + const auto& rcl_msg = msg->get_rcl_serialized_message(); + const auto* bytes = reinterpret_cast(rcl_msg.buffer); + + std::shared_ptr> data; + if (bound) { + data = std::make_shared>(); + if (!bound->run(topic, {bytes, rcl_msg.buffer_length}, *data)) { + return; // dropped and counted; never forward the untransformed bytes } + } else { + data = std::make_shared>(bytes, bytes + rcl_msg.buffer_length); } - const auto& rcl_msg = msg_to_use->get_rcl_serialized_message(); - const auto* bytes = reinterpret_cast(rcl_msg.buffer); - auto data = std::make_shared>(bytes, bytes + rcl_msg.buffer_length); - MessageCallback cb; { std::lock_guard lock(callback_mutex_); @@ -67,7 +65,7 @@ bool Ros2SubscriptionManager::subscribe(const std::string& topic_name, const std } }; - return inner_manager_.subscribe(topic_name, topic_type, ros2_callback); + return inner_manager_.subscribe(topic_name, source_type, ros2_callback); } bool Ros2SubscriptionManager::unsubscribe(const std::string& topic_name) { diff --git a/ros2/src/strip_transform.cpp b/ros2/src/strip_transform.cpp new file mode 100644 index 0000000..ee64b16 --- /dev/null +++ b/ros2/src/strip_transform.cpp @@ -0,0 +1,72 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include "pj_bridge_ros2/strip_transform.hpp" + +#include + +#include "pj_bridge_ros2/message_stripper.hpp" + +namespace pj_bridge { +namespace { + +class StripTransform : public MessageTransform { + public: + explicit StripTransform(std::string type) : type_(std::move(type)) {} + + tl::expected apply(std::span in, std::vector& out) override { + try { + // MessageStripper needs a SerializedMessage: one copy of the input. + rclcpp::SerializedMessage input(in.size()); + auto& rcl_in = input.get_rcl_serialized_message(); + std::memcpy(rcl_in.buffer, in.data(), in.size()); + rcl_in.buffer_length = in.size(); + + const auto stripped = MessageStripper::strip(type_, input); + const auto& rcl_out = stripped.get_rcl_serialized_message(); + const auto* bytes = reinterpret_cast(rcl_out.buffer); + out.assign(bytes, bytes + rcl_out.buffer_length); + return {}; + } catch (const std::exception& e) { + return tl::make_unexpected(std::string(e.what())); + } + } + + private: + std::string type_; +}; + +} // namespace + +TransformFactory make_strip_transform_factory() { + TransformFactory f; + f.accepts = [](const std::string& type) { return MessageStripper::should_strip(type); }; + f.check_params = [](const nlohmann::json& params) -> tl::expected { + if (!params.empty()) { + return tl::make_unexpected(std::string("takes no params")); + } + return {}; + }; + f.output_type = [](const std::string& type) { return type; }; + f.output_schema = [](const std::string&, const std::string& schema) { return schema; }; + f.create = [](const std::string& type, const nlohmann::json&) { return std::make_unique(type); }; + return f; +} + +} // namespace pj_bridge diff --git a/tests/unit/test_ros2_subscription_manager.cpp b/tests/unit/test_ros2_subscription_manager.cpp index b21fb53..4f80d37 100644 --- a/tests/unit/test_ros2_subscription_manager.cpp +++ b/tests/unit/test_ros2_subscription_manager.cpp @@ -23,21 +23,37 @@ #include #include #include +#include #include +#include "pj_bridge/transform_set.hpp" +#include "pj_bridge_ros2/message_stripper.hpp" #include "pj_bridge_ros2/ros2_subscription_manager.hpp" +#include "pj_bridge_ros2/strip_transform.hpp" using namespace pj_bridge; namespace { +// A TransformSet with the `strip` rule appended for every strippable type, +// as strip_large_messages=true wires it up in main.cpp. +std::shared_ptr strip_everything() { + auto set = TransformSet::create( + nlohmann::json{{"transforms", nlohmann::json::array()}}, {{"strip", make_strip_transform_factory()}}) + .value(); + for (const auto& type : MessageStripper::strippable_types()) { + EXPECT_TRUE(set->append_type_rule(type, "strip").has_value()); + } + return set; +} + // Publish an Image with a large data payload through the manager and return // the size of the serialized message delivered to the bridge callback. -size_t roundtrip_image_bytes(bool strip_large_messages, bool use_default_config) { - auto node = std::make_shared( - use_default_config ? "test_strip_default" : (strip_large_messages ? "test_strip_on" : "test_strip_off")); +// `transforms` is nullptr for "no transform configured". +size_t roundtrip_image_bytes(std::shared_ptr transforms, bool use_default_config) { + auto node = std::make_shared(use_default_config ? "test_strip_default" : "test_strip_on"); auto manager = use_default_config ? std::make_shared(node) - : std::make_shared(node, strip_large_messages); + : std::make_shared(node, transforms); std::atomic received_size{0}; manager->set_message_callback( @@ -46,6 +62,10 @@ size_t roundtrip_image_bytes(bool strip_large_messages, bool use_default_config) }); const std::string topic = "/strip_test_image_" + std::string(node->get_name()); + if (transforms) { + // ASSERT_* requires a void-returning function; this helper returns size_t. + EXPECT_NE(transforms->bind(topic, "sensor_msgs/msg/Image"), nullptr); // what get_topics would have done + } auto publisher = node->create_publisher(topic, rclcpp::QoS(10)); EXPECT_TRUE(manager->subscribe(topic, "sensor_msgs/msg/Image")); @@ -86,20 +106,74 @@ class Ros2SubscriptionManagerTest : public ::testing::Test { }; // --------------------------------------------------------------------------- -// Stripping is opt-in: by default, large data fields are forwarded intact. +// Stripping is opt-in: by default, no transform is configured and large data +// fields are forwarded intact. // --------------------------------------------------------------------------- TEST_F(Ros2SubscriptionManagerTest, DataFieldsIncludedByDefault) { - size_t received = roundtrip_image_bytes(false, /*use_default_config=*/true); + size_t received = roundtrip_image_bytes(/*transforms=*/nullptr, /*use_default_config=*/true); ASSERT_GT(received, 0u) << "no message received"; EXPECT_GE(received, kImagePayloadBytes) << "image data was stripped despite default (opt-in) configuration"; } // --------------------------------------------------------------------------- -// Opting in still strips: with strip_large_messages=true the payload is -// removed and only the metadata remains. +// Opting in still strips: with a `strip` transform bound to the topic, the +// payload is removed and only the metadata remains. // --------------------------------------------------------------------------- TEST_F(Ros2SubscriptionManagerTest, OptInStrippingRemovesData) { - size_t received = roundtrip_image_bytes(/*strip_large_messages=*/true, /*use_default_config=*/false); + size_t received = roundtrip_image_bytes(strip_everything(), /*use_default_config=*/false); ASSERT_GT(received, 0u) << "no message received"; EXPECT_LT(received, kImagePayloadBytes) << "opt-in stripping did not remove the data payload"; } + +// --------------------------------------------------------------------------- +// The hook subscribes with the topic's SOURCE type (not the advertised +// output type) and forwards whatever bytes the bound transform produces. +// --------------------------------------------------------------------------- +TEST_F(Ros2SubscriptionManagerTest, SubscribesWithSourceTypeAndForwardsTransformedBytes) { + // A transform whose output type differs from the source type: the manager is + // asked to subscribe with the ADVERTISED type and must use the source type. + TransformFactory f; + f.accepts = [](const std::string& t) { return t == "std_msgs/msg/String"; }; + f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; + f.output_type = [](const std::string&) { return std::string("fake_msgs/msg/Out"); }; + f.output_schema = [](const std::string&, const std::string& s) { return s; }; + f.create = [](const std::string&, const nlohmann::json&) -> std::unique_ptr { + struct Marker : MessageTransform { + tl::expected apply(std::span, std::vector& out) override { + out.assign(3, std::byte{0x5A}); + return {}; + } + }; + return std::make_unique(); + }; + auto set = TransformSet::create( + nlohmann::json{{"transforms", {{{"match_topic", "/transform_me"}, {"transform", "marker"}}}}}, + {{"marker", f}}) + .value(); + ASSERT_NE(set->bind("/transform_me", "std_msgs/msg/String"), nullptr); // what get_topics would have done + + auto node = std::make_shared("test_transform_hook"); + Ros2SubscriptionManager manager(node, set); + std::vector received; + manager.set_message_callback( + [&](const std::string&, std::shared_ptr> data, uint64_t) { received = *data; }); + + // subscribe() is called with the ADVERTISED (output) type, as BridgeServer does. + ASSERT_TRUE(manager.subscribe("/transform_me", "fake_msgs/msg/Out")); + + auto publisher = node->create_publisher("/transform_me", rclcpp::QoS(10)); + std_msgs::msg::String msg; + msg.data = "hello"; + + rclcpp::executors::SingleThreadedExecutor executor; + executor.add_node(node); + + auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (received.empty() && std::chrono::steady_clock::now() < deadline) { + publisher->publish(msg); + executor.spin_some(); + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + + EXPECT_EQ(received, std::vector(3, std::byte{0x5A})); +} From 65aabaa0788c40204478cec27170b60a5c6cc9ce Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:36:42 +0200 Subject: [PATCH 09/19] feat(transform): transform_profile parameter; strip_large_messages as sugar New `transform_profile` string parameter (default "", meaning no profile). If set, main.cpp reads and parses it into a TransformSet (built-in factory: "strip"), wraps Ros2TopicSource in a TransformingTopicSource, and passes the TransformSet to Ros2SubscriptionManager. Any parse or validation failure exits 1 with a descriptive message. `strip_large_messages: true` is now sugar: after the profile's rules (if any) are loaded, one `strip` match_type rule is appended per MessageStripper::strippable_types(), so an explicit profile rule for a type always wins. Transform statistics are logged at shutdown. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- ros2/src/main.cpp | 69 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 6 deletions(-) diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index fbe125c..0a70a36 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -29,9 +30,13 @@ #include "pj_bridge/bridge_server.hpp" #include "pj_bridge/middleware/websocket_middleware.hpp" +#include "pj_bridge/transform_set.hpp" +#include "pj_bridge/transforming_topic_source.hpp" #include "pj_bridge/whitelist_filter.hpp" +#include "pj_bridge_ros2/message_stripper.hpp" #include "pj_bridge_ros2/ros2_subscription_manager.hpp" #include "pj_bridge_ros2/ros2_topic_source.hpp" +#include "pj_bridge_ros2/strip_transform.hpp" int main(int argc, char** argv) { rclcpp::init(argc, argv); @@ -45,6 +50,7 @@ int main(int argc, char** argv) { node->declare_parameter("publish_rate", 50.0); node->declare_parameter("session_timeout", 10.0); node->declare_parameter("strip_large_messages", false); + node->declare_parameter("transform_profile", ""); node->declare_parameter>("topic_whitelist", {".*"}); node->declare_parameter("min_qos_depth", 10); node->declare_parameter("ingest_poll_interval_ms", 5.0); @@ -60,6 +66,7 @@ int main(int argc, char** argv) { double publish_rate = node->get_parameter("publish_rate").as_double(); double session_timeout = node->get_parameter("session_timeout").as_double(); bool strip_large_messages = node->get_parameter("strip_large_messages").as_bool(); + std::string transform_profile = node->get_parameter("transform_profile").as_string(); std::vector topic_whitelist = node->get_parameter("topic_whitelist").as_string_array(); int64_t min_qos_depth = node->get_parameter("min_qos_depth").as_int(); double ingest_poll_interval_ms = node->get_parameter("ingest_poll_interval_ms").as_double(); @@ -74,10 +81,11 @@ int main(int argc, char** argv) { RCLCPP_INFO( node->get_logger(), "Configuration: port=%d, publish_rate=%.1f Hz, session_timeout=%.1f s, strip_large_messages=%s, " - "min_qos_depth=%ld, max_qos_depth=%ld, ingest_poll_interval_ms=%.1f, topic_poll_interval=%.1f s, " - "client_backlog_size=%ld, tls=%s", - port, publish_rate, session_timeout, strip_large_messages ? "true" : "false", min_qos_depth, max_qos_depth, - ingest_poll_interval_ms, topic_poll_interval, client_backlog_size, tls_enabled ? "true" : "false"); + "transform_profile='%s', min_qos_depth=%ld, max_qos_depth=%ld, ingest_poll_interval_ms=%.1f, " + "topic_poll_interval=%.1f s, client_backlog_size=%ld, tls=%s", + port, publish_rate, session_timeout, strip_large_messages ? "true" : "false", transform_profile.c_str(), + min_qos_depth, max_qos_depth, ingest_poll_interval_ms, topic_poll_interval, client_backlog_size, + tls_enabled ? "true" : "false"); if (tls_enabled && (certfile.empty() || keyfile.empty())) { RCLCPP_ERROR(node->get_logger(), "tls=true requires both 'certfile' and 'keyfile' parameters to be set"); @@ -143,9 +151,51 @@ int main(int argc, char** argv) { try { // Create backend components - auto topic_source = std::make_shared(node); + std::shared_ptr topic_source = std::make_shared(node); + + std::shared_ptr transforms; + if (!transform_profile.empty() || strip_large_messages) { + nlohmann::json profile = {{"transforms", nlohmann::json::array()}}; + if (!transform_profile.empty()) { + std::ifstream file(transform_profile); + if (!file) { + RCLCPP_ERROR(node->get_logger(), "Cannot open transform_profile '%s'", transform_profile.c_str()); + rclcpp::shutdown(); + return 1; + } + profile = nlohmann::json::parse(file, nullptr, /*allow_exceptions=*/false); + if (profile.is_discarded()) { + RCLCPP_ERROR(node->get_logger(), "transform_profile '%s' is not valid JSON", transform_profile.c_str()); + rclcpp::shutdown(); + return 1; + } + } + + pj_bridge::TransformSet::FactoryMap factories; + factories["strip"] = pj_bridge::make_strip_transform_factory(); + + auto created = pj_bridge::TransformSet::create(profile, std::move(factories)); + if (!created) { + RCLCPP_ERROR(node->get_logger(), "%s", created.error().c_str()); + rclcpp::shutdown(); + return 1; + } + transforms = *created; + + if (strip_large_messages) { // after the profile's rules, so an explicit rule wins + for (const auto& type : pj_bridge::MessageStripper::strippable_types()) { + // append_type_rule() is [[nodiscard]]; failure here would mean "strip" no longer + // accepts one of its own strippable_types(), which should never happen. + if (auto added = transforms->append_type_rule(type, "strip"); !added) { + RCLCPP_ERROR(node->get_logger(), "strip_large_messages: %s", added.error().c_str()); + } + } + } + topic_source = std::make_shared(topic_source, transforms); + } + auto sub_manager = std::make_shared( - node, nullptr, static_cast(min_qos_depth), static_cast(max_qos_depth)); + node, transforms, static_cast(min_qos_depth), static_cast(max_qos_depth)); std::optional tls_config; if (tls_enabled) { tls_config = pj_bridge::TlsConfig{certfile, keyfile}; @@ -269,6 +319,13 @@ int main(int argc, char** argv) { "%lu heavy frames shed (congestion)", total_messages, total_bytes, middleware->dropped_frame_count(), middleware->heavy_shed_count()); + if (transforms) { + const auto summary = transforms->stats_summary(); + if (!summary.empty()) { + RCLCPP_INFO(node->get_logger(), "Transform statistics:%s", summary.c_str()); + } + } + RCLCPP_INFO(node->get_logger(), "Bridge server shutdown complete"); } catch (const std::exception& e) { From c46f8a45de8b036817ea68d1851b50447fdc0e99 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:41:37 +0200 Subject: [PATCH 10/19] test(transform): failed transform drops the sample on the ROS2 path Also from review: strip rejects an empty message before touching the rcl buffer, and a factory that cannot set up a topic is logged once, not on every topic poll. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- app/src/transform_set.cpp | 7 ++- ros2/src/strip_transform.cpp | 3 ++ tests/unit/test_ros2_subscription_manager.cpp | 47 +++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/app/src/transform_set.cpp b/app/src/transform_set.cpp index 9cc61e8..de1db77 100644 --- a/app/src/transform_set.cpp +++ b/app/src/transform_set.cpp @@ -170,8 +170,11 @@ std::shared_ptr TransformSet::bind(const std::string& topic, con spdlog::info("Topic '{}' ({}) -> transform '{}' -> {}", topic, source_type, rule.transform, bound->output_type); return bound; } catch (const std::exception& e) { - spdlog::error( - "Transform '{}' could not be set up for '{}': {}; leaving it untransformed", rule.transform, topic, e.what()); + if (warned_topics_.insert(topic).second) { // bind() runs on every topic poll: log once + spdlog::error( + "Transform '{}' could not be set up for '{}': {}; leaving it untransformed", rule.transform, topic, + e.what()); + } return nullptr; } } diff --git a/ros2/src/strip_transform.cpp b/ros2/src/strip_transform.cpp index ee64b16..836090f 100644 --- a/ros2/src/strip_transform.cpp +++ b/ros2/src/strip_transform.cpp @@ -31,6 +31,9 @@ class StripTransform : public MessageTransform { explicit StripTransform(std::string type) : type_(std::move(type)) {} tl::expected apply(std::span in, std::vector& out) override { + if (in.empty()) { + return tl::make_unexpected(std::string("empty message")); + } try { // MessageStripper needs a SerializedMessage: one copy of the input. rclcpp::SerializedMessage input(in.size()); diff --git a/tests/unit/test_ros2_subscription_manager.cpp b/tests/unit/test_ros2_subscription_manager.cpp index 4f80d37..84b684a 100644 --- a/tests/unit/test_ros2_subscription_manager.cpp +++ b/tests/unit/test_ros2_subscription_manager.cpp @@ -177,3 +177,50 @@ TEST_F(Ros2SubscriptionManagerTest, SubscribesWithSourceTypeAndForwardsTransform EXPECT_EQ(received, std::vector(3, std::byte{0x5A})); } + +// The client was told the output type at subscribe time, so a sample whose +// transform fails must be dropped, never forwarded untransformed. +TEST_F(Ros2SubscriptionManagerTest, FailedTransformDropsTheSample) { + TransformFactory f; + f.accepts = [](const std::string& t) { return t == "std_msgs/msg/String"; }; + f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; + f.output_type = [](const std::string& t) { return t; }; + f.output_schema = [](const std::string&, const std::string& s) { return s; }; + f.create = [](const std::string&, const nlohmann::json&) -> std::unique_ptr { + struct Failing : MessageTransform { + tl::expected apply(std::span, std::vector&) override { + return tl::make_unexpected(std::string("boom")); + } + }; + return std::make_unique(); + }; + auto set = TransformSet::create( + nlohmann::json{{"transforms", {{{"match_topic", "/always_fails"}, {"transform", "failing"}}}}}, + {{"failing", f}}) + .value(); + auto bound = set->bind("/always_fails", "std_msgs/msg/String"); + ASSERT_NE(bound, nullptr); + + auto node = std::make_shared("test_transform_hook_failure"); + Ros2SubscriptionManager manager(node, set); + std::atomic forwarded{0}; + manager.set_message_callback( + [&](const std::string&, std::shared_ptr>, uint64_t) { forwarded++; }); + ASSERT_TRUE(manager.subscribe("/always_fails", "std_msgs/msg/String")); + + auto publisher = node->create_publisher("/always_fails", rclcpp::QoS(10)); + std_msgs::msg::String msg; + msg.data = "hello"; + rclcpp::executors::SingleThreadedExecutor executor; + executor.add_node(node); + + auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (bound->drops.load() < 3 && std::chrono::steady_clock::now() < deadline) { + publisher->publish(msg); + executor.spin_some(); + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + + EXPECT_GE(bound->drops.load(), 3u); + EXPECT_EQ(forwarded.load(), 0); +} From 4b91f93c30d3fa8ce73ceac271742d0ca2992989 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:41:37 +0200 Subject: [PATCH 11/19] feat(transform): Cloudini point cloud compression sensor_msgs/PointCloud2 -> point_cloud_interfaces/CompressedPointCloud2 on the CDR bytes, second stage NONE (the frame is zstd-compressed already). cloudini_lib is optional: find_package first, otherwise fetched (1.3.0, pinned hash) and built as a plain static library. Its ament branch, its own zstd lookup and its install rules are kept out of this project. package.xml: no cloudini_lib dependency yet (rosdep key availability per distro not checked). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CMakeLists.txt | 57 ++++++++++ app/include/pj_bridge/cloudini_transform.hpp | 34 ++++++ app/src/cloudini_transform.cpp | 114 +++++++++++++++++++ ros2/src/main.cpp | 6 + tests/unit/test_cloudini_transform.cpp | 83 ++++++++++++++ 5 files changed, 294 insertions(+) create mode 100644 app/include/pj_bridge/cloudini_transform.hpp create mode 100644 app/src/cloudini_transform.cpp create mode 100644 tests/unit/test_cloudini_transform.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 1b71d73..1b67652 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -88,6 +88,53 @@ else() message(STATUS "Using system IXWebSocket") endif() +# Cloudini (optional) — point cloud compression transform +option(PJ_BRIDGE_FETCH_CLOUDINI "Fetch Cloudini when cloudini_lib is not installed" ON) +find_package(cloudini_lib QUIET) +if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) + message(STATUS "cloudini_lib not found — fetching via FetchContent") + FetchContent_Declare( + cloudini + URL https://github.com/facontidavide/cloudini/archive/refs/tags/1.3.0.tar.gz + URL_HASH SHA256=26c9322d5a08694b9b24bbec975b7814b05cf7d4962327f35266b38bb15b5e66 + ) + set(CLOUDINI_BUILD_TOOLS OFF CACHE BOOL "" FORCE) + set(CLOUDINI_BUILD_BENCHMARKS OFF CACHE BOOL "" FORCE) + # Use the zstd found above instead of a second, vendored copy in the same binary. + # FindZSTD already defined zstd::libzstd_static; Cloudini's own + # find_package(zstd CONFIG) would clash with those targets, so it is disabled. + set(CLOUDINI_FORCE_VENDORED_DEPS OFF CACHE BOOL "" FORCE) + set(CMAKE_DISABLE_FIND_PACKAGE_zstd ON) + # Cloudini becomes a SHARED ament package (install rules, ament_package()) when + # it finds ament_cmake. Hide it while its directory is processed: we want the + # plain static library, and EXCLUDE_FROM_ALL keeps its install rules out of ours. + set(_pj_build_testing ${BUILD_TESTING}) + set(BUILD_TESTING OFF) + set(CMAKE_DISABLE_FIND_PACKAGE_ament_cmake ON) + set(_pj_ament_cmake_found ${ament_cmake_FOUND}) + set(ament_cmake_FOUND FALSE) # the disabled find_package() leaves our earlier result in place + set(_pj_build_shared_libs ${BUILD_SHARED_LIBS}) + set(BUILD_SHARED_LIBS OFF) + FetchContent_GetProperties(cloudini) + if(NOT cloudini_POPULATED) + if(POLICY CMP0169) + cmake_policy(SET CMP0169 OLD) # FetchContent_Populate: needed for add_subdirectory(EXCLUDE_FROM_ALL) before CMake 3.28 + endif() + FetchContent_Populate(cloudini) + add_subdirectory(${cloudini_SOURCE_DIR}/cloudini_lib ${cloudini_BINARY_DIR} EXCLUDE_FROM_ALL) + endif() + set(BUILD_SHARED_LIBS ${_pj_build_shared_libs}) + set(ament_cmake_FOUND ${_pj_ament_cmake_found}) + set(CMAKE_DISABLE_FIND_PACKAGE_zstd OFF) + set(CMAKE_DISABLE_FIND_PACKAGE_ament_cmake OFF) + set(BUILD_TESTING ${_pj_build_testing}) + set_target_properties(cloudini_lib PROPERTIES POSITION_INDEPENDENT_CODE ON) + if(NOT TARGET cloudini::cloudini_lib) + add_library(cloudini::cloudini_lib ALIAS cloudini_lib) + endif() + set(cloudini_lib_FOUND TRUE) +endif() + # spdlog — available via rosdep (libspdlog-dev) or pixi (conda-forge) find_package(spdlog REQUIRED) @@ -129,6 +176,15 @@ target_link_libraries(pj_bridge_app PUBLIC nlohmann_json::nlohmann_json ) +if(cloudini_lib_FOUND) + target_sources(pj_bridge_app PRIVATE app/src/cloudini_transform.cpp) + target_link_libraries(pj_bridge_app PRIVATE cloudini::cloudini_lib) + target_compile_definitions(pj_bridge_app PUBLIC PJ_BRIDGE_HAS_CLOUDINI) + message(STATUS "Cloudini transform: enabled") +else() + message(STATUS "Cloudini transform: disabled (cloudini_lib not found)") +endif() + # ============================================================================ # pj_bridge_ros2 — ROS2 backend (only if ament_cmake found) # ============================================================================ @@ -276,6 +332,7 @@ if(BUILD_TESTING AND ament_cmake_FOUND) tests/unit/test_generic_subscription_manager.cpp tests/unit/test_ros2_subscription_manager.cpp tests/unit/test_message_stripper.cpp + tests/unit/test_cloudini_transform.cpp ) target_link_libraries(${PROJECT_NAME}_tests diff --git a/app/include/pj_bridge/cloudini_transform.hpp b/app/include/pj_bridge/cloudini_transform.hpp new file mode 100644 index 0000000..247e70b --- /dev/null +++ b/app/include/pj_bridge/cloudini_transform.hpp @@ -0,0 +1,34 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include "pj_bridge/message_transform.hpp" + +namespace pj_bridge { + +/// `cloudini` transform: sensor_msgs PointCloud2 -> point_cloud_interfaces +/// CompressedPointCloud2. Works on CDR bytes, so it serves any backend whose +/// PointCloud2 is CDR-encoded. Only declared when Cloudini is available. +/// +/// Params: resolution (float, default 0.001), fields ({name: resolution}, 0 +/// removes the field), viz_preprocessing (bool, default false). +TransformFactory make_cloudini_transform_factory(); + +} // namespace pj_bridge diff --git a/app/src/cloudini_transform.cpp b/app/src/cloudini_transform.cpp new file mode 100644 index 0000000..9fa6f99 --- /dev/null +++ b/app/src/cloudini_transform.cpp @@ -0,0 +1,114 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#include "pj_bridge/cloudini_transform.hpp" + +#include +#include +// Defines non-inline `const char*` globals: include in this translation unit ONLY. +#include + +namespace pj_bridge { +namespace { + +constexpr const char* kPointCloud2 = "sensor_msgs/msg/PointCloud2"; + +class CloudiniTransform : public MessageTransform { + public: + explicit CloudiniTransform(const nlohmann::json& params) + : resolution_(params.value("resolution", 0.001f)), viz_preprocessing_(params.value("viz_preprocessing", false)) { + if (params.contains("fields")) { + for (auto it = params["fields"].begin(); it != params["fields"].end(); ++it) { + profile_[it.key()] = it->get(); + } + } + } + + tl::expected apply(std::span in, std::vector& out) override { + try { + const Cloudini::ConstBufferView raw(reinterpret_cast(in.data()), in.size()); + auto cloud = cloudini_ros::getDeserializedPointCloudMessage(raw); + cloudini_ros::applyResolutionProfile(profile_, cloud.fields, resolution_); + if (viz_preprocessing_) { + cloudini_ros::applyVizLossyPreprocessing(cloud); + } + auto info = cloudini_ros::toEncodingInfo(cloud); + // The frame is zstd-compressed by the bridge: a second stage here would compress twice. + info.compression_opt = Cloudini::CompressionOption::NONE; + info.use_threads = false; + + // Cloudini writes CDR into a vector; scratch_ keeps its capacity across calls. + cloudini_ros::convertPointCloud2ToCompressedCloud(cloud, info, scratch_); + const auto* bytes = reinterpret_cast(scratch_.data()); + out.assign(bytes, bytes + scratch_.size()); + return {}; + } catch (const std::exception& e) { + return tl::make_unexpected(std::string(e.what())); + } + } + + private: + float resolution_; + bool viz_preprocessing_; + cloudini_ros::ResolutionProfile profile_; + std::vector scratch_; +}; + +tl::expected check_cloudini_params(const nlohmann::json& params) { + for (auto it = params.begin(); it != params.end(); ++it) { + const auto& key = it.key(); + if (key == "resolution") { + if (!it->is_number() || it->get() <= 0.0) { + return tl::make_unexpected(std::string("'resolution' must be a number > 0")); + } + } else if (key == "viz_preprocessing") { + if (!it->is_boolean()) { + return tl::make_unexpected(std::string("'viz_preprocessing' must be a boolean")); + } + } else if (key == "fields") { + if (!it->is_object()) { + return tl::make_unexpected(std::string("'fields' must be an object of {name: resolution}")); + } + for (auto f = it->begin(); f != it->end(); ++f) { + if (!f->is_number() || f->get() < 0.0) { + return tl::make_unexpected("'fields." + f.key() + "' must be a number >= 0 (0 removes the field)"); + } + } + } else { + return tl::make_unexpected("unknown param '" + key + "'"); + } + } + return {}; +} + +} // namespace + +TransformFactory make_cloudini_transform_factory() { + TransformFactory f; + f.accepts = [](const std::string& type) { return type == kPointCloud2; }; + f.check_params = check_cloudini_params; + f.output_type = [](const std::string&) { return std::string(compressed_schema_name); }; + f.output_schema = [](const std::string&, const std::string&) { return std::string(compressed_schema_data); }; + f.create = [](const std::string&, const nlohmann::json& params) { + return std::make_unique(params); + }; + return f; +} + +} // namespace pj_bridge diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 0a70a36..2537e54 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -30,6 +30,9 @@ #include "pj_bridge/bridge_server.hpp" #include "pj_bridge/middleware/websocket_middleware.hpp" +#ifdef PJ_BRIDGE_HAS_CLOUDINI +#include "pj_bridge/cloudini_transform.hpp" +#endif #include "pj_bridge/transform_set.hpp" #include "pj_bridge/transforming_topic_source.hpp" #include "pj_bridge/whitelist_filter.hpp" @@ -173,6 +176,9 @@ int main(int argc, char** argv) { pj_bridge::TransformSet::FactoryMap factories; factories["strip"] = pj_bridge::make_strip_transform_factory(); +#ifdef PJ_BRIDGE_HAS_CLOUDINI + factories["cloudini"] = pj_bridge::make_cloudini_transform_factory(); +#endif auto created = pj_bridge::TransformSet::create(profile, std::move(factories)); if (!created) { diff --git a/tests/unit/test_cloudini_transform.cpp b/tests/unit/test_cloudini_transform.cpp new file mode 100644 index 0000000..8cad8fc --- /dev/null +++ b/tests/unit/test_cloudini_transform.cpp @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#ifdef PJ_BRIDGE_HAS_CLOUDINI + +#include + +#include +#include +#include +#include + +#include "pj_bridge/cloudini_transform.hpp" + +using namespace pj_bridge; + +namespace { +std::vector make_cloud_cdr(size_t points) { + sensor_msgs::msg::PointCloud2 cloud; + cloud.header.frame_id = "lidar"; + sensor_msgs::PointCloud2Modifier modifier(cloud); + modifier.setPointCloud2FieldsByString(1, "xyz"); + modifier.resize(points); + sensor_msgs::PointCloud2Iterator x(cloud, "x"), y(cloud, "y"), z(cloud, "z"); + for (size_t i = 0; i < points; ++i, ++x, ++y, ++z) { + *x = 0.01f * static_cast(i); + *y = 1.0f; + *z = -0.5f; + } + rclcpp::SerializedMessage serialized; + rclcpp::Serialization().serialize_message(&cloud, &serialized); + const auto& rcl = serialized.get_rcl_serialized_message(); + const auto* bytes = reinterpret_cast(rcl.buffer); + return {bytes, bytes + rcl.buffer_length}; +} +} // namespace + +TEST(CloudiniTransformTest, CompressesPointCloud2) { + auto factory = make_cloudini_transform_factory(); + EXPECT_TRUE(factory.accepts("sensor_msgs/msg/PointCloud2")); + EXPECT_FALSE(factory.accepts("sensor_msgs/msg/Image")); + EXPECT_EQ(factory.output_type("sensor_msgs/msg/PointCloud2"), "point_cloud_interfaces/msg/CompressedPointCloud2"); + EXPECT_NE(factory.output_schema("sensor_msgs/msg/PointCloud2", "").find("format"), std::string::npos); + + const nlohmann::json params = {{"resolution", 0.001}}; + ASSERT_TRUE(factory.check_params(params).has_value()); + EXPECT_FALSE(factory.check_params({{"resolutoin", 0.001}}).has_value()); + EXPECT_FALSE(factory.check_params({{"resolution", -1.0}}).has_value()); + + auto transform = factory.create("sensor_msgs/msg/PointCloud2", params); + const auto in = make_cloud_cdr(10000); + std::vector out; + ASSERT_TRUE(transform->apply(in, out).has_value()); + EXPECT_FALSE(out.empty()); + EXPECT_LT(out.size(), in.size()); + + ASSERT_TRUE(transform->apply(in, out).has_value()); // instance is reusable +} + +TEST(CloudiniTransformTest, GarbageInputIsAnErrorNotACrash) { + auto transform = make_cloudini_transform_factory().create("sensor_msgs/msg/PointCloud2", nlohmann::json::object()); + const std::vector in(7, std::byte{0xFF}); + std::vector out; + EXPECT_FALSE(transform->apply(in, out).has_value()); +} + +#endif // PJ_BRIDGE_HAS_CLOUDINI From ff10ea22286ff3a8742d9484075573aa0a0ff18a Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:46:18 +0200 Subject: [PATCH 12/19] docs: message transforms Document the message-transform feature (transform_profile, strip and cloudini transforms, wire-protocol additions, startup/runtime error behavior) in docs/API.md, README.md, and CLAUDE.md; add a Forthcoming section to CHANGELOG.rst and an example transform_profile.example.json. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CHANGELOG.rst | 31 +++++ CLAUDE.md | 15 ++- README.md | 2 + docs/API.md | 170 +++++++++++++++++++++++++++- docs/transform_profile.example.json | 9 ++ 5 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 docs/transform_profile.example.json diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 914a3d6..d4b9547 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,37 @@ Changelog for package pj_ros_bridge ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* Message transforms (ROS2 only): operator-configured, off by default, a + topic is transformed for all subscribers or none — no per-client + negotiation. New ``transform_profile`` parameter points to a JSON file of + ordered per-topic rules (``match_type`` exact and/or ``match_topic`` + full-match regex, first match wins). A transformed topic is advertised + with its transform's output type/schema, plus an optional ``source_type`` + on ``get_topics`` entries (not yet on ``topics_changed`` entries); new + ``message_transforms`` server capability. +* New ``cloudini`` transform: ``sensor_msgs/msg/PointCloud2`` -> + ``point_cloud_interfaces/msg/CompressedPointCloud2`` using `Cloudini + `_ point cloud compression + (lossy at a configurable resolution; its own second compression stage is + disabled since the bridge already ZSTD-compresses every frame). Only + built when ``cloudini_lib`` is available (``find_package``, else fetched + at configure time unless ``-DPJ_BRIDGE_FETCH_CLOUDINI=OFF``); a profile + naming ``cloudini`` on a build without it fails at startup. +* ``strip_large_messages`` is now sugar for a ``strip`` transform rule + appended after the profile's own rules (an explicit profile rule for the + same type wins). +* **Behavior change:** a transform (including ``strip``) that fails on a + message now drops the sample instead of forwarding it — previously + ``strip_large_messages`` forwarded the original, unstripped message on + failure. +* Per-topic transform statistics (samples, drops, compression ratio, + microseconds/sample) logged with the final statistics at shutdown. +* The project now requires **C++20** (``std::span``, and ``cloudini_lib``'s + PUBLIC ``cxx_std_20`` requirement). +* 301 unit tests passing. + 0.10.0 (2026-09-20) ------------------- * WebSocket permessage-deflate declined server-side (redundant with our own diff --git a/CLAUDE.md b/CLAUDE.md index 5bf8581..8f28904 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -69,6 +69,7 @@ make -j$(nproc) ### Code Quality +- **C++ Standard**: C++20 (required for `std::span`, and because `cloudini_lib` exports `cxx_std_20` as a PUBLIC requirement) - **Thread Safety**: Use mutexes, document thread safety in class comments - **Testing**: Unit tests required for all core components (gtest) - **Code Formatting**: `pre-commit run -a` before committing (clang-format) @@ -123,7 +124,7 @@ BridgeServer does NOT own timers. The entry point (`main.cpp`) drives the event 6. **Ros2TopicSource** (`ros2/`) — Wraps `TopicDiscovery` + `SchemaExtractor`. Schema encoding: `"ros2msg"`. -7. **Ros2SubscriptionManager** (`ros2/`) — Wraps `GenericSubscriptionManager` + optional `MessageStripper`. Converts `rclcpp::SerializedMessage` → `shared_ptr>` via memcpy. +7. **Ros2SubscriptionManager** (`ros2/`) — Wraps `GenericSubscriptionManager`. On `subscribe()`, resolves the topic's `TransformSet` binding (if any) to find the real source type and subscribes with it; in the message callback, a bound transform's `apply()` runs on the rcl buffer before forwarding (replacing the old inline `MessageStripper` call — `strip` is now a transform, see item 13). Converts `rclcpp::SerializedMessage` → `shared_ptr>` via memcpy (or via the transform's output when one is bound). 8. **RtiTopicSource** (`rti/`) — Wraps `DdsTopicDiscovery`. Schema encoding: `"omgidl"`. (Build disabled) @@ -133,6 +134,10 @@ BridgeServer does NOT own timers. The entry point (`main.cpp`) drives the event 11. **FastDdsSubscriptionManager** (`fastdds/`) — Directly implements `SubscriptionManagerInterface`. Creates `DataReader`s with `DynamicPubSubType`, deserializes into `DynamicData` and re-serializes to extract CDR bytes. +12. **TransformSet** (`app/`) — Owns the ordered `transform_profile` rules, the transform name → `TransformFactory` map, and the per-topic `BoundTransform` instances (created lazily on first match, `first match wins`). Thread-safe: written from the request thread (`get_topics`/`subscribe`), read from the ingest thread. Also tracks per-topic transform statistics (samples, drops, compression ratio, µs/sample), logged at shutdown. + +13. **TransformingTopicSource** (`app/`) — Decorator over a `TopicSourceInterface`. Rewrites `get_topics()`/`get_schema()` to the transform's output type/schema for matched topics (recording `source_type`) and passes everything else through unchanged. `strip` and `cloudini` are the two built-in transforms (`ros2/src/strip_transform.cpp`, `app/src/cloudini_transform.cpp`); `strip` replaces the old inline `MessageStripper` call in `Ros2SubscriptionManager` (see item 7). + ### Communication Pattern **WebSocket** (single port, default 9090): @@ -181,6 +186,9 @@ Then, for each message in the (compressed) payload: - **nlohmann/json** — JSON (`find_package(REQUIRED)`) - **tl::expected** — error handling (header-only, vendored in 3rdparty/) +### Core (optional) +- **Cloudini** (`cloudini_lib`) — point cloud compression for the `cloudini` transform. `find_package(cloudini_lib)` first, else fetched at configure time (pinned tag `1.3.0`, static) unless `-DPJ_BRIDGE_FETCH_CLOUDINI=OFF`. A build with neither simply lacks the `cloudini` transform (`PJ_BRIDGE_HAS_CLOUDINI` undefined); `strip` and the rest of the transform machinery are unaffected. + ### ROS2 Backend - `rclcpp`, `ament_index_cpp`, `ament_cmake` - `sensor_msgs`, `nav_msgs` (for message stripper) @@ -197,7 +205,7 @@ Then, for each message in the (compressed) payload: ## Testing -### Test Count: 231 unit tests across 11 test suites +### Test Count: 301 unit tests ### Commands ```bash @@ -233,6 +241,7 @@ port: 9090 # WebSocket port publish_rate: 50.0 # Hz session_timeout: 10.0 # seconds strip_large_messages: false # Opt-in: strip Image/PointCloud2/etc data fields +transform_profile: "" # Path to a per-topic message-transform profile JSON (e.g. Cloudini); empty disables topic_whitelist: [".*"] # Full-match regex patterns restricting visible/subscribable topics min_qos_depth: 10 # Minimum KEEP_LAST subscription depth after aggregating publisher depths max_qos_depth: 100 # Maximum KEEP_LAST subscription depth after aggregating publisher depths @@ -282,5 +291,5 @@ identity/capabilities object, and TLS setup). **Last Updated**: 2026-07-06 **Project Phase**: Unified multi-backend architecture -**Test Status**: 231 unit tests passing (all sanitizers clean) +**Test Status**: 301 unit tests passing (all sanitizers clean) **Executables**: `pj_bridge_ros2` (ROS2), `pj_bridge_rti` (RTI DDS, disabled), `pj_bridge_fastdds` (FastDDS) diff --git a/README.md b/README.md index c066cdb..debbc90 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,7 @@ independently. - **Multi-Client Support**: Multiple clients can connect simultaneously with shared subscriptions - **Runtime Schema Discovery**: Automatic extraction of message schemas from installed ROS2 packages on the server side. - **Large Message Stripping** (opt-in): Optional stripping of large array fields (Image, PointCloud2, LaserScan, OccupancyGrid) to reduce bandwidth while preserving metadata. Disabled by default — full message data is forwarded; enable with `strip_large_messages:=true` for low-bandwidth links +- **Message Transforms** (opt-in, ROS2 only): Operator-configured per-topic transforms (e.g. `cloudini` point cloud compression) applied before buffering, replacing a topic's payload and advertised type/schema for all subscribers. Off by default (`transform_profile:=""`); see [docs/API.md](docs/API.md#message-transforms-ros2-only) - **Topic Whitelist**: Restrict which topics are visible/subscribable via full-match regex patterns (`topic_whitelist` / `--topic-whitelist`), mirroring foxglove_bridge's option of the same name - **QoS Depth Heuristics** (ROS2 only): KEEP_LAST subscription depth is derived from the discovered publishers' depths and clamped to a configurable `[min_qos_depth, max_qos_depth]` range - **Pushed Topic Advertisement** (opt-in): Clients can subscribe to a `topics_changed` notification instead of polling `get_topics`, at a configurable `topic_poll_interval` @@ -49,6 +50,7 @@ independently. | `publish_rate` | double | 50.0 | Aggregation publish rate in Hz | | `session_timeout` | double | 10.0 | Client timeout duration in seconds | | `strip_large_messages` | bool | false | Opt-in: strip large arrays from Image, PointCloud2, LaserScan, OccupancyGrid messages | +| `transform_profile` | string | `""` | ROS2 only: path to a JSON profile of per-topic message transforms (e.g. Cloudini point cloud compression); empty disables the feature | | `topic_whitelist` | string array | `[".*"]` | Full-match regex patterns (ECMAScript) restricting visible/subscribable topics | | `min_qos_depth` | int | 10 | ROS2 only: minimum KEEP_LAST subscription depth after aggregating publisher depths | | `max_qos_depth` | int | 100 | ROS2 only: maximum KEEP_LAST subscription depth after aggregating publisher depths | diff --git a/docs/API.md b/docs/API.md index f9beeaf..053c429 100644 --- a/docs/API.md +++ b/docs/API.md @@ -92,6 +92,15 @@ fully match any whitelist pattern are omitted from the response entirely (see } ``` +A topic rewritten by a [message transform](#message-transforms-ros2-only) is +advertised with its transform **output** type, plus an extra `source_type` +field carrying the original type: + +```json +{"name": "/lidar/points", "type": "point_cloud_interfaces/msg/CompressedPointCloud2", + "source_type": "sensor_msgs/msg/PointCloud2"} +``` + ### Server identity and capabilities (`server`) Every `get_topics` response carries a `server` object: @@ -100,7 +109,8 @@ Every `get_topics` response carries a `server` object: {"server": {"name": "pj_bridge", "version": "0.10.0", "capabilities": ["include_schemas", "latched_badge", "latched_replay", "topics_changed", - "per_topic_rate_limit", "size_class_frames"]}} + "per_topic_rate_limit", "size_class_frames", + "message_transforms"]}} ``` Compatibility policy for clients: @@ -243,6 +253,160 @@ the reader depth (`min_qos_depth`) covers a burst. Publish/request/session-timeout timers run on their own executor thread so a long publish (zstd-compression) cycle never delays ingest. +## Message Transforms (ROS2 only) + +The server operator can configure per-topic transforms that replace a +topic's payload — and, optionally, its type — before it is buffered and +sent. This is operator-only configuration: there is no per-client +negotiation, and a transformed topic is transformed for **all** subscribers +or none. With no profile configured and `strip_large_messages: false`, no +transform machinery is created at all and behavior/wire output are +byte-identical to a server without this feature. + +- **ROS2**: string parameter `transform_profile`, default `""` (disabled), + pointing to a JSON profile file. + +Advertised by the `message_transforms` capability (always present in +`server.capabilities`, whether or not a profile is configured). + +### Wire protocol + +Unchanged. A transformed topic is simply advertised with its transform's +**output** type instead of its real one: + +- `get_topics` entries carry the output `type`, plus an optional + `source_type` field with the original type (see + [Get Topics](#get-topics)). +- `subscribe` returns the output schema (`encoding` + `definition`) for that + topic — a client never sees the source schema. +- `topics_changed` `added` entries carry the output `type` only; they do + **not** carry `source_type` in this version (see + [Pushed Topic Advertisement](#pushed-topic-advertisement-topics_changed)). + +Everything downstream — latched replay, size-class framing, rate limiting, +the binary frame format — operates on the transformed bytes with no special +case; a transform is invisible past the subscription layer. + +Client requirement: a client that subscribes to a transformed topic needs a +parser for the **output** type. In particular, older PlotJuggler clients +without a `CompressedPointCloud2` parser see a topic they cannot decode — +enabling the `cloudini` transform on a topic is the operator's call to make +for their fleet of clients. + +### Profile format + +```json +{ + "transforms": [ + { + "match_type": "sensor_msgs/msg/PointCloud2", + "match_topic": "/lidar/.*", + "transform": "cloudini", + "params": {"resolution": 0.001, "fields": {"intensity": 0.01}, "viz_preprocessing": true} + } + ] +} +``` + +See also `docs/transform_profile.example.json`. + +Each rule in `transforms` needs at least one of: + +- `match_type` — the topic's type, matched **exactly**. +- `match_topic` — full-match ECMAScript regex against the topic name, same + semantics as [`topic_whitelist`](#topic-whitelist) (`std::regex_match`, + not a substring search). + +When both are present, **both** must match. Rules are evaluated in order +and the **first match wins** — later rules are never consulted for a topic +once one has matched (successfully or not; see below). + +A rule that matches a topic by `match_topic` only (no `match_type`) whose +named transform does not accept that topic's actual type is **skipped**: a +one-time warning is logged for that topic and evaluation continues with the +next rule. A `match_type` rule can never hit this case — `TransformSet` +rejects a rule at startup whose transform does not accept its declared +`match_type` (see below). + +### `strip_large_messages` interaction + +`strip_large_messages: true` is sugar for one `strip` rule per strippable +type (`sensor_msgs/msg/Image`, `sensor_msgs/msg/CompressedImage`, +`sensor_msgs/msg/PointCloud2`, `sensor_msgs/msg/LaserScan`, +`nav_msgs/msg/OccupancyGrid`), appended **after** the profile's own rules — +so an explicit rule in `transform_profile` for one of those types always +wins over the boolean. + +### Startup errors + +The server refuses to start (exits with a non-zero status) on any of: + +- `transform_profile` names a file that cannot be opened/read. +- The file is not valid JSON. +- An unknown top-level key (anything other than `transforms`). +- An unknown or mistyped rule key (e.g. `params` that isn't an object). +- A rule with neither `match_type` nor `match_topic`. +- A rule missing `transform`. +- A rule naming an unknown transform — including `cloudini` in a build + without Cloudini compiled in (see `CLAUDE.md` / `README.md` for the build + configuration). +- An invalid `match_topic` regex. +- Params rejected by the transform (unknown key, wrong type, out-of-range + value). +- A `match_type` rule whose transform does not accept that type. + +### Runtime behavior + +If `apply()` fails for a message (bad/unexpected input, codec exception), +the sample is **dropped** — it is never forwarded untransformed, because +the client was told the output type/schema at subscribe time. Drops are +counted per topic and logged with a throttled warning (1st, 2nd, 4th, 8th… +occurrence). + +**Behavior change:** this also applies to `strip` messages. Previously, +a stripping failure forwarded the original, unstripped message; as a +transform, a `strip` failure now drops the sample instead (see +`CHANGELOG.rst`). + +Per-topic transform statistics (sample count, drop count, compression +ratio, mean microseconds/sample) are logged together with the server's +final statistics at shutdown. + +### Transforms + +#### `strip` + +Thin wrapper around the pre-existing message-stripping behavior +(`strip_large_messages`): output type and schema equal the input's, no +params accepted. + +#### `cloudini` + +`sensor_msgs/msg/PointCloud2` → `point_cloud_interfaces/msg/CompressedPointCloud2`, +using [Cloudini](https://github.com/facontidavide/cloudini) point cloud +compression. **Lossy** at the configured resolution. Only registered when +the server was built with Cloudini available; naming it in a profile on a +build without it is a startup error. The output schema is embedded in +Cloudini's own headers, so the robot does **not** need +`point_cloud_interfaces` installed. + +Cloudini's own second compression stage is disabled — the bridge already +ZSTD-compresses every outgoing frame, so a second codec-level compression +stage would waste CPU for no benefit. + +Params (all optional): + +| Key | Default | Meaning | +|---|---|---| +| `resolution` | `0.001` | Quantization resolution (metres for `x`/`y`/`z`); must be `> 0` | +| `fields` | `{}` | Per-field resolution override, `{name: resolution}`; `0` removes that field entirely | +| `viz_preprocessing` | `false` | Drops NaN/inf points, voxel-deduplicates at the `xyz` resolution, and quantizes `FLOAT64` fields to 1 µs — a lossy preprocessing pass aimed at visualization, not lossless round-tripping | + +### Backend support + +Only wired for the ROS2 backend in this version; FastDDS and RTI do not +apply transforms yet. + ## Subscribe Subscribe to one or more topics. **Breaking change:** Subscribe now uses an additive model - it only adds topics without removing existing subscriptions. Use the `unsubscribe` command to remove topics. @@ -486,6 +650,10 @@ additionally carries `encoding` + `definition` (the same fields as when at least one of them is non-empty. - A topic whose type changes (same name, different type) is reported as both removed and added. +- `added` entries do **not** carry `source_type`, unlike `get_topics` + (see [Message Transforms](#message-transforms-ros2-only)): the poll that + builds them tracks a plain name → type map. Clients that need the source + type of a transformed topic should call `get_topics`. - **Per-topic schema failure never drops a topic** (same as `get_topics`): an `added` entry whose schema cannot be extracted is delivered with `name` + `type` only, no `encoding`/`definition`. Clients must treat the schema diff --git a/docs/transform_profile.example.json b/docs/transform_profile.example.json new file mode 100644 index 0000000..d00b2c8 --- /dev/null +++ b/docs/transform_profile.example.json @@ -0,0 +1,9 @@ +{ + "transforms": [ + { + "match_type": "sensor_msgs/msg/PointCloud2", + "transform": "cloudini", + "params": {"resolution": 0.001, "viz_preprocessing": false} + } + ] +} From 17196041845bd7986fee6c52012b1daa48a6924b Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 18:51:13 +0200 Subject: [PATCH 13/19] docs: message transform measurements Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- .../2026-09-20-message-transforms-design.md | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/specs/2026-09-20-message-transforms-design.md b/docs/superpowers/specs/2026-09-20-message-transforms-design.md index 84bda96..5c072bc 100644 --- a/docs/superpowers/specs/2026-09-20-message-transforms-design.md +++ b/docs/superpowers/specs/2026-09-20-message-transforms-design.md @@ -1,6 +1,6 @@ # Message Transforms (v1: Cloudini) — Design -Date: 2026-09-20. Status: approved design, not implemented. +Date: 2026-09-20. Status: implemented (ROS2 backend); see "Measured results". Supersedes the exploratory `docs/MESSAGE_TRANSFORMS.md` (PR #14). ## Goal @@ -229,6 +229,33 @@ client-side per-topic message counts (must be lossless). If encode time starves ingest, the designed-but-unbuilt fallback is a per-topic worker with a 1-deep latest-wins input slot. +## Measured results (2026-09-20) + +Pinned rig (bridge on cores 4-7, player 12-19, client 8-11, governor +`performance`), 90 s cut of the 4-lidar bag (4 x PointCloud2 at 10 Hz, ~52 MiB/s +raw, plus /tf, /imu, /odom), one client subscribed to everything, CPU from +`/proc//stat` over 20 s, three runs each. + +| Configuration | Bridge CPU (% of a core) | Wire | Encode | Pre-zstd ratio | +|---|---|---|---|---| +| no profile | 16.7 / 18.9 / 19.3 | 31.0 MB/s | — | — | +| `cloudini`, 1 mm | 16.7 / 14.0 / 16.8 | 14.6 MB/s | 1.4 ms/cloud | 2.7–2.9 | +| `cloudini`, 1 mm, `viz_preprocessing` | 18.3 / 17.9 / 18.3 | 13.8 MB/s | 2.9 ms/cloud | 3.1–3.5 | + +Lossless in every run: 300/300 clouds per lidar in 30 s, zero transform drops, +other topics at their nominal rates. Bandwidth halves at equal or slightly +lower CPU: the ~6% of a core spent encoding on the ingest thread is paid back +by zstd having a third of the bytes to compress. Run-to-run CPU noise is about +±2 points, so "CPU-neutral" is the defensible claim. `viz_preprocessing` doubles +the encode time for ~5% fewer bytes on this data, which is why it defaults to +off. Synchronous encoding in the ingest callback is sufficient; the per-topic +worker fallback was not needed. + +Cloudini-side check: reusing the `PointcloudEncoder` across messages instead of +the one-shot `convertPointCloud2ToCompressedCloud` measured 0% faster with +second stage `NONE` and ~5% with `ZSTD`, so the scratch-and-copy approach above +costs nothing measurable. + ## Non-goals (v1) Per-client transform choice or parameters; client negotiation; transform From 26ec88bb2a613cb367e26c34c74a0237fd7a8497 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 19:02:40 +0200 Subject: [PATCH 14/19] fix(transform): validate clouds before encoding; harden the Cloudini CMake block Final review findings: - a PointCloud2 whose width*height*point_step disagrees with its data, or with a field past point_step, is dropped instead of reaching an encoder that trusts those values (over-read / undecodable frame) - link whichever target an installed cloudini_lib exports (cloudini:: for a standalone install, cloudini_lib:: for an ament one) and require >= 1.2 - keep PCL and Cloudini's forced CMAKE_BUILD_TYPE out of this project; give data_path.hpp its own directory (Cloudini writes one to the same path) - docs: resolution applies to every FLOAT32 field, limitations, offline builds Verified: 302 tests with Cloudini, 299 with -DPJ_BRIDGE_FETCH_CLOUDINI=OFF. Not verified: the ament-installed cloudini_lib path (none available here). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CHANGELOG.rst | 2 +- CLAUDE.md | 4 ++-- CMakeLists.txt | 22 ++++++++++++++++++---- README.md | 2 ++ app/src/cloudini_transform.cpp | 20 ++++++++++++++++++++ docs/API.md | 21 ++++++++++++++++++--- tests/unit/test_cloudini_transform.cpp | 23 ++++++++++++++++++++++- 7 files changed, 83 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d4b9547..cae614c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -31,7 +31,7 @@ Forthcoming microseconds/sample) logged with the final statistics at shutdown. * The project now requires **C++20** (``std::span``, and ``cloudini_lib``'s PUBLIC ``cxx_std_20`` requirement). -* 301 unit tests passing. +* 302 unit tests passing. 0.10.0 (2026-09-20) ------------------- diff --git a/CLAUDE.md b/CLAUDE.md index 8f28904..9326dab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -205,7 +205,7 @@ Then, for each message in the (compressed) payload: ## Testing -### Test Count: 301 unit tests +### Test Count: 302 unit tests ### Commands ```bash @@ -291,5 +291,5 @@ identity/capabilities object, and TLS setup). **Last Updated**: 2026-07-06 **Project Phase**: Unified multi-backend architecture -**Test Status**: 301 unit tests passing (all sanitizers clean) +**Test Status**: 302 unit tests passing (all sanitizers clean) **Executables**: `pj_bridge_ros2` (ROS2), `pj_bridge_rti` (RTI DDS, disabled), `pj_bridge_fastdds` (FastDDS) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1b67652..4ab6c1f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -90,7 +90,8 @@ endif() # Cloudini (optional) — point cloud compression transform option(PJ_BRIDGE_FETCH_CLOUDINI "Fetch Cloudini when cloudini_lib is not installed" ON) -find_package(cloudini_lib QUIET) +# 1.2 introduced the v5 codec and applyVizLossyPreprocessing used by the transform. +find_package(cloudini_lib 1.2 QUIET) if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) message(STATUS "cloudini_lib not found — fetching via FetchContent") FetchContent_Declare( @@ -115,6 +116,8 @@ if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) set(ament_cmake_FOUND FALSE) # the disabled find_package() leaves our earlier result in place set(_pj_build_shared_libs ${BUILD_SHARED_LIBS}) set(BUILD_SHARED_LIBS OFF) + set(CMAKE_DISABLE_FIND_PACKAGE_PCL ON) # optional in Cloudini; would become a PUBLIC link dependency of ours + set(_pj_build_type "${CMAKE_BUILD_TYPE}") FetchContent_GetProperties(cloudini) if(NOT cloudini_POPULATED) if(POLICY CMP0169) @@ -123,6 +126,10 @@ if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) FetchContent_Populate(cloudini) add_subdirectory(${cloudini_SOURCE_DIR}/cloudini_lib ${cloudini_BINARY_DIR} EXCLUDE_FROM_ALL) endif() + if(NOT _pj_build_type) # Cloudini forces Release into the cache when no build type is set + unset(CMAKE_BUILD_TYPE CACHE) + endif() + set(CMAKE_DISABLE_FIND_PACKAGE_PCL OFF) set(BUILD_SHARED_LIBS ${_pj_build_shared_libs}) set(ament_cmake_FOUND ${_pj_ament_cmake_found}) set(CMAKE_DISABLE_FIND_PACKAGE_zstd OFF) @@ -145,9 +152,10 @@ find_package(nlohmann_json REQUIRED) # Data path for tests # ============================================================================ get_filename_component(DATA_PATH "${CMAKE_CURRENT_LIST_DIR}/DATA" ABSOLUTE) -configure_file(data_path.hpp.in "${CMAKE_BINARY_DIR}/include/data_path.hpp" @ONLY) +# Own directory: Cloudini writes a data_path.hpp of its own to ${CMAKE_BINARY_DIR}/include. +configure_file(data_path.hpp.in "${CMAKE_BINARY_DIR}/pj_include/data_path.hpp" @ONLY) add_library(data_path INTERFACE IMPORTED) -target_include_directories(data_path INTERFACE ${CMAKE_BINARY_DIR}/include) +target_include_directories(data_path INTERFACE ${CMAKE_BINARY_DIR}/pj_include) # ============================================================================ # pj_bridge_app — backend-agnostic core library @@ -177,8 +185,14 @@ target_link_libraries(pj_bridge_app PUBLIC ) if(cloudini_lib_FOUND) + # A standalone install exports cloudini::cloudini_lib, an ament install cloudini_lib::cloudini_lib. + if(TARGET cloudini::cloudini_lib) + set(_pj_cloudini_target cloudini::cloudini_lib) + else() + set(_pj_cloudini_target cloudini_lib::cloudini_lib) + endif() target_sources(pj_bridge_app PRIVATE app/src/cloudini_transform.cpp) - target_link_libraries(pj_bridge_app PRIVATE cloudini::cloudini_lib) + target_link_libraries(pj_bridge_app PRIVATE ${_pj_cloudini_target}) target_compile_definitions(pj_bridge_app PUBLIC PJ_BRIDGE_HAS_CLOUDINI) message(STATUS "Cloudini transform: enabled") else() diff --git a/README.md b/README.md index debbc90..a5bec4b 100644 --- a/README.md +++ b/README.md @@ -115,6 +115,8 @@ All dependencies (spdlog, nlohmann_json, ZSTD) are provided by the dependency ma TLS (`wss://`) support depends on IXWebSocket being built with OpenSSL. The CMake option `PJ_BRIDGE_TLS` (default `ON`) controls this for the FetchContent path (`-DPJ_BRIDGE_TLS=OFF` to disable); a system/conda-provided IXWebSocket must likewise have been built with TLS. See [docs/API.md](docs/API.md#tls--wss) for details. +Cloudini (the `cloudini` message transform) is optional: `find_package(cloudini_lib)` is tried first (version >= 1.2), otherwise it is fetched at configure time and linked statically. Builds without network access must either have `cloudini_lib` installed or pass `-DPJ_BRIDGE_FETCH_CLOUDINI=OFF`, which builds the bridge without that transform. + ### ROS2 — Pixi [Pixi](https://pixi.sh) manages the full toolchain including ROS2 via [RoboStack](https://robostack.github.io/). diff --git a/app/src/cloudini_transform.cpp b/app/src/cloudini_transform.cpp index 9fa6f99..66a7cf5 100644 --- a/app/src/cloudini_transform.cpp +++ b/app/src/cloudini_transform.cpp @@ -29,6 +29,23 @@ namespace { constexpr const char* kPointCloud2 = "sensor_msgs/msg/PointCloud2"; +// The encoder trusts field offsets, and the output header repeats width/height: +// a cloud whose metadata disagrees with its payload must not reach it. +tl::expected check_cloud(const cloudini_ros::RosPointCloud2& cloud) { + const uint64_t declared = uint64_t{cloud.width} * cloud.height * cloud.point_step; + if (declared != cloud.data.size()) { + return tl::make_unexpected( + "width*height*point_step = " + std::to_string(declared) + " but data has " + std::to_string(cloud.data.size()) + + " bytes"); + } + for (const auto& field : cloud.fields) { + if (uint64_t{field.offset} + static_cast(Cloudini::SizeOf(field.type)) > cloud.point_step) { + return tl::make_unexpected("field '" + field.name + "' extends past point_step"); + } + } + return {}; +} + class CloudiniTransform : public MessageTransform { public: explicit CloudiniTransform(const nlohmann::json& params) @@ -44,6 +61,9 @@ class CloudiniTransform : public MessageTransform { try { const Cloudini::ConstBufferView raw(reinterpret_cast(in.data()), in.size()); auto cloud = cloudini_ros::getDeserializedPointCloudMessage(raw); + if (auto valid = check_cloud(cloud); !valid) { + return valid; + } cloudini_ros::applyResolutionProfile(profile_, cloud.fields, resolution_); if (viz_preprocessing_) { cloudini_ros::applyVizLossyPreprocessing(cloud); diff --git a/docs/API.md b/docs/API.md index 053c429..1ea9cbc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -318,8 +318,7 @@ Each rule in `transforms` needs at least one of: not a substring search). When both are present, **both** must match. Rules are evaluated in order -and the **first match wins** — later rules are never consulted for a topic -once one has matched (successfully or not; see below). +and the **first match wins** — with the one exception described next. A rule that matches a topic by `match_topic` only (no `match_type`) whose named transform does not accept that topic's actual type is **skipped**: a @@ -398,10 +397,26 @@ Params (all optional): | Key | Default | Meaning | |---|---|---| -| `resolution` | `0.001` | Quantization resolution (metres for `x`/`y`/`z`); must be `> 0` | +| `resolution` | `0.001` | Quantization resolution applied to **every** `FLOAT32` field not listed in `fields` — metres for `x`/`y`/`z`, but also e.g. `intensity` or a `FLOAT32` time field; must be `> 0` | | `fields` | `{}` | Per-field resolution override, `{name: resolution}`; `0` removes that field entirely | | `viz_preprocessing` | `false` | Drops NaN/inf points, voxel-deduplicates at the `xyz` resolution, and quantizes `FLOAT64` fields to 1 µs — a lossy preprocessing pass aimed at visualization, not lossless round-tripping | +Things to know before enabling it: + +- A coarse `resolution` also coarsens non-geometry `FLOAT32` fields; give + those their own value in `fields` (for example `{"intensity": 0.001}`). +- A field removed with resolution `0` keeps its place in the point layout and + decodes as zeros. +- `viz_preprocessing` returns an unorganized cloud (`height = 1`, `width` = + points kept), even when the input was organized. +- A cloud whose `width * height * point_step` does not equal its data size, or + with a field extending past `point_step`, is dropped like any other + transform failure. +- Big-endian clouds are not supported (the flag is not inspected; they would + decode as garbage). `PointField.count` is written as 1. +- The payload uses Cloudini encoding v5: the client needs a Cloudini decoder + >= 1.2.0. + ### Backend support Only wired for the ROS2 backend in this version; FastDDS and RTI do not diff --git a/tests/unit/test_cloudini_transform.cpp b/tests/unit/test_cloudini_transform.cpp index 8cad8fc..0f44bdd 100644 --- a/tests/unit/test_cloudini_transform.cpp +++ b/tests/unit/test_cloudini_transform.cpp @@ -21,6 +21,7 @@ #include +#include #include #include #include @@ -31,7 +32,8 @@ using namespace pj_bridge; namespace { -std::vector make_cloud_cdr(size_t points) { +std::vector make_cloud_cdr( + size_t points, const std::function& corrupt = nullptr) { sensor_msgs::msg::PointCloud2 cloud; cloud.header.frame_id = "lidar"; sensor_msgs::PointCloud2Modifier modifier(cloud); @@ -43,6 +45,9 @@ std::vector make_cloud_cdr(size_t points) { *y = 1.0f; *z = -0.5f; } + if (corrupt) { + corrupt(cloud); + } rclcpp::SerializedMessage serialized; rclcpp::Serialization().serialize_message(&cloud, &serialized); const auto& rcl = serialized.get_rcl_serialized_message(); @@ -80,4 +85,20 @@ TEST(CloudiniTransformTest, GarbageInputIsAnErrorNotACrash) { EXPECT_FALSE(transform->apply(in, out).has_value()); } +// Any DDS peer can publish a PointCloud2 whose metadata lies about its payload. +// The encoder trusts field offsets and the header repeats width/height, so these +// must be rejected here: a drop, not an over-read or an undecodable frame. +TEST(CloudiniTransformTest, InconsistentCloudMetadataIsRejected) { + auto transform = make_cloudini_transform_factory().create("sensor_msgs/msg/PointCloud2", nlohmann::json::object()); + std::vector out; + + const auto field_past_point = make_cloud_cdr(100, [](auto& c) { c.fields[2].offset = c.point_step; }); + EXPECT_FALSE(transform->apply(field_past_point, out).has_value()); + + const auto width_lies = make_cloud_cdr(100, [](auto& c) { c.width = 1000; }); + EXPECT_FALSE(transform->apply(width_lies, out).has_value()); + + EXPECT_TRUE(transform->apply(make_cloud_cdr(100), out).has_value()); // and a sane one still works +} + #endif // PJ_BRIDGE_HAS_CLOUDINI From 30aded74bde4e69fc74b2b5ad458ca4926b6f5b8 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 20:45:26 +0200 Subject: [PATCH 15/19] test(transform): wait for the ingest thread in the concurrency test On a CI runner the main thread's 2000 iterations finished before the ingest thread had run once, so the final samples > 0 check failed. The loop now continues until the two threads have actually overlapped. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- tests/unit/test_transform_set.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_transform_set.cpp b/tests/unit/test_transform_set.cpp index 4094dec..c184c46 100644 --- a/tests/unit/test_transform_set.cpp +++ b/tests/unit/test_transform_set.cpp @@ -165,7 +165,9 @@ TEST(TransformSetTest, RunIsSafeConcurrentlyWithBindFindAndStats) { bound->run("/a", in, out); } }); - for (int i = 0; i < 2000; ++i) { + // Keep going until the ingest thread has really overlapped with us: on a busy + // machine 2000 iterations can finish before it is even scheduled. + for (int i = 0; i < 2000 || bound->samples.load() < 100; ++i) { (*set)->bind("/a", "pkg/msg/In"); (*set)->bind("/other" + std::to_string(i % 8), "pkg/msg/In"); (*set)->find("/a"); From 352cafb057c738a6ddcb0611e1aaba33083c441a Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 21:06:32 +0200 Subject: [PATCH 16/19] refactor(transform): simplify after /simplify review; fix log-once keying Refactors (behaviour-preserving, 302 existing tests unchanged in intent): - output_type/output_schema move from TransformFactory to virtuals on MessageTransform with identity defaults; the factory is accepts/check_params/ create. Removes the identity lambdas in strip and the tests, and an unused source_type argument. - Rule::params defaults to {}; bind() guards only what can throw. - main.cpp: load_transforms() throws into the existing handler instead of three copies of log/shutdown/return; append_strip_rules() replaces the loop that main.cpp and a test both wrote. - CMake: the Cloudini fetch is a function(), so nothing needs restoring. - tests: shared fake_transform.hpp and one publish_until() helper. Fix, test first (SkippedRuleWarningDoesNotHideALaterSetupError): the log-once set was keyed by topic, so a 'does not accept' warning swallowed a later 'could not be set up' error for the same topic. Now keyed by (topic, transform) and cleared when a topic is rebound. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CHANGELOG.rst | 2 +- CLAUDE.md | 4 +- CMakeLists.txt | 47 ++++---- app/include/pj_bridge/message_transform.hpp | 14 ++- app/include/pj_bridge/transform_set.hpp | 9 +- app/src/cloudini_transform.cpp | 10 +- app/src/transform_set.cpp | 92 +++++++++------- app/src/transforming_topic_source.cpp | 2 +- .../2026-09-20-message-transforms-design.md | 6 +- .../pj_bridge_ros2/strip_transform.hpp | 6 +- ros2/src/main.cpp | 85 +++++++-------- ros2/src/strip_transform.cpp | 11 +- tests/unit/fake_transform.hpp | 80 ++++++++++++++ tests/unit/test_cloudini_transform.cpp | 5 +- tests/unit/test_ros2_subscription_manager.cpp | 100 ++++++------------ tests/unit/test_transform_set.cpp | 72 ++++++------- tests/unit/test_transforming_topic_source.cpp | 21 +--- 17 files changed, 313 insertions(+), 253 deletions(-) create mode 100644 tests/unit/fake_transform.hpp diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cae614c..7f16be9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -31,7 +31,7 @@ Forthcoming microseconds/sample) logged with the final statistics at shutdown. * The project now requires **C++20** (``std::span``, and ``cloudini_lib``'s PUBLIC ``cxx_std_20`` requirement). -* 302 unit tests passing. +* 303 unit tests passing. 0.10.0 (2026-09-20) ------------------- diff --git a/CLAUDE.md b/CLAUDE.md index 9326dab..813dd5c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -205,7 +205,7 @@ Then, for each message in the (compressed) payload: ## Testing -### Test Count: 302 unit tests +### Test Count: 303 unit tests ### Commands ```bash @@ -291,5 +291,5 @@ identity/capabilities object, and TLS setup). **Last Updated**: 2026-07-06 **Project Phase**: Unified multi-backend architecture -**Test Status**: 302 unit tests passing (all sanitizers clean) +**Test Status**: 303 unit tests passing (all sanitizers clean) **Executables**: `pj_bridge_ros2` (ROS2), `pj_bridge_rti` (RTI DDS, disabled), `pj_bridge_fastdds` (FastDDS) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4ab6c1f..3be8c3c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -92,8 +92,10 @@ endif() option(PJ_BRIDGE_FETCH_CLOUDINI "Fetch Cloudini when cloudini_lib is not installed" ON) # 1.2 introduced the v5 codec and applyVizLossyPreprocessing used by the transform. find_package(cloudini_lib 1.2 QUIET) -if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) - message(STATUS "cloudini_lib not found — fetching via FetchContent") + +# Builds Cloudini as a plain static library inside this project. A function, so +# that everything set() below is scoped to Cloudini's directory and needs no restoring. +function(pj_bridge_fetch_cloudini) FetchContent_Declare( cloudini URL https://github.com/facontidavide/cloudini/archive/refs/tags/1.3.0.tar.gz @@ -107,38 +109,35 @@ if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) set(CLOUDINI_FORCE_VENDORED_DEPS OFF CACHE BOOL "" FORCE) set(CMAKE_DISABLE_FIND_PACKAGE_zstd ON) # Cloudini becomes a SHARED ament package (install rules, ament_package()) when - # it finds ament_cmake. Hide it while its directory is processed: we want the - # plain static library, and EXCLUDE_FROM_ALL keeps its install rules out of ours. - set(_pj_build_testing ${BUILD_TESTING}) - set(BUILD_TESTING OFF) + # it finds ament_cmake: hide it. The disabled find_package() leaves our own + # earlier result in place, hence ament_cmake_FOUND as well. set(CMAKE_DISABLE_FIND_PACKAGE_ament_cmake ON) - set(_pj_ament_cmake_found ${ament_cmake_FOUND}) - set(ament_cmake_FOUND FALSE) # the disabled find_package() leaves our earlier result in place - set(_pj_build_shared_libs ${BUILD_SHARED_LIBS}) + set(ament_cmake_FOUND FALSE) set(BUILD_SHARED_LIBS OFF) + set(BUILD_TESTING OFF) set(CMAKE_DISABLE_FIND_PACKAGE_PCL ON) # optional in Cloudini; would become a PUBLIC link dependency of ours - set(_pj_build_type "${CMAKE_BUILD_TYPE}") - FetchContent_GetProperties(cloudini) - if(NOT cloudini_POPULATED) - if(POLICY CMP0169) - cmake_policy(SET CMP0169 OLD) # FetchContent_Populate: needed for add_subdirectory(EXCLUDE_FROM_ALL) before CMake 3.28 - endif() - FetchContent_Populate(cloudini) - add_subdirectory(${cloudini_SOURCE_DIR}/cloudini_lib ${cloudini_BINARY_DIR} EXCLUDE_FROM_ALL) + set(build_type_was_set "${CMAKE_BUILD_TYPE}") + + # FetchContent_Populate + add_subdirectory(EXCLUDE_FROM_ALL) keeps Cloudini's + # install rules out of ours; FetchContent_Declare(EXCLUDE_FROM_ALL) needs CMake 3.28. + if(POLICY CMP0169) + cmake_policy(SET CMP0169 OLD) endif() - if(NOT _pj_build_type) # Cloudini forces Release into the cache when no build type is set + FetchContent_Populate(cloudini) + add_subdirectory(${cloudini_SOURCE_DIR}/cloudini_lib ${cloudini_BINARY_DIR} EXCLUDE_FROM_ALL) + + if(NOT build_type_was_set) # Cloudini forces Release into the cache when no build type is set unset(CMAKE_BUILD_TYPE CACHE) endif() - set(CMAKE_DISABLE_FIND_PACKAGE_PCL OFF) - set(BUILD_SHARED_LIBS ${_pj_build_shared_libs}) - set(ament_cmake_FOUND ${_pj_ament_cmake_found}) - set(CMAKE_DISABLE_FIND_PACKAGE_zstd OFF) - set(CMAKE_DISABLE_FIND_PACKAGE_ament_cmake OFF) - set(BUILD_TESTING ${_pj_build_testing}) set_target_properties(cloudini_lib PROPERTIES POSITION_INDEPENDENT_CODE ON) if(NOT TARGET cloudini::cloudini_lib) add_library(cloudini::cloudini_lib ALIAS cloudini_lib) endif() +endfunction() + +if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) + message(STATUS "cloudini_lib not found — fetching via FetchContent") + pj_bridge_fetch_cloudini() set(cloudini_lib_FOUND TRUE) endif() diff --git a/app/include/pj_bridge/message_transform.hpp b/app/include/pj_bridge/message_transform.hpp index fae1ba5..bfdc32e 100644 --- a/app/include/pj_bridge/message_transform.hpp +++ b/app/include/pj_bridge/message_transform.hpp @@ -42,6 +42,16 @@ class MessageTransform { /// On error the caller DROPS the sample. It must never forward `in` instead: /// the client was told the output type at subscribe time. virtual tl::expected apply(std::span in, std::vector& out) = 0; + + /// Type name advertised to clients in place of `source_type`. Default: unchanged. + virtual std::string output_type(const std::string& source_type) const { + return source_type; + } + + /// Schema advertised to clients, given the untransformed one. Default: unchanged. + virtual std::string output_schema(const std::string& source_schema) const { + return source_schema; + } }; struct TransformFactory { @@ -49,10 +59,6 @@ struct TransformFactory { std::function accepts; /// Reject unknown or malformed params. Called once per rule at startup. std::function(const nlohmann::json& params)> check_params; - /// Type name advertised to clients in place of `source_type`. - std::function output_type; - /// Schema advertised to clients. `source_schema` is the untransformed one. - std::function output_schema; std::function(const std::string& source_type, const nlohmann::json& params)> create; }; diff --git a/app/include/pj_bridge/transform_set.hpp b/app/include/pj_bridge/transform_set.hpp index 70774ca..e9e7a4d 100644 --- a/app/include/pj_bridge/transform_set.hpp +++ b/app/include/pj_bridge/transform_set.hpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include "pj_bridge/message_transform.hpp" @@ -73,9 +74,6 @@ class TransformSet { /// Binding previously created by bind(), or nullptr. std::shared_ptr find(const std::string& topic) const; - /// Schema to advertise for a bound topic. - std::string output_schema(const BoundTransform& bound, const std::string& source_schema) const; - /// One line per bound topic that processed at least one sample. std::string stats_summary() const; @@ -84,17 +82,18 @@ class TransformSet { std::optional match_type; std::optional match_topic; std::string transform; - nlohmann::json params; + nlohmann::json params = nlohmann::json::object(); }; TransformSet() = default; tl::expected add_rule(Rule rule); + bool log_once(const std::string& topic, const std::string& transform); FactoryMap factories_; std::vector rules_; mutable std::mutex mutex_; std::unordered_map> bindings_; - std::set warned_topics_; + std::set> logged_; // (topic, transform) already reported }; } // namespace pj_bridge diff --git a/app/src/cloudini_transform.cpp b/app/src/cloudini_transform.cpp index 66a7cf5..b5d2176 100644 --- a/app/src/cloudini_transform.cpp +++ b/app/src/cloudini_transform.cpp @@ -83,6 +83,14 @@ class CloudiniTransform : public MessageTransform { } } + std::string output_type(const std::string&) const override { + return compressed_schema_name; + } + + std::string output_schema(const std::string&) const override { + return compressed_schema_data; + } + private: float resolution_; bool viz_preprocessing_; @@ -123,8 +131,6 @@ TransformFactory make_cloudini_transform_factory() { TransformFactory f; f.accepts = [](const std::string& type) { return type == kPointCloud2; }; f.check_params = check_cloudini_params; - f.output_type = [](const std::string&) { return std::string(compressed_schema_name); }; - f.output_schema = [](const std::string&, const std::string&) { return std::string(compressed_schema_data); }; f.create = [](const std::string&, const nlohmann::json& params) { return std::make_unique(params); }; diff --git a/app/src/transform_set.cpp b/app/src/transform_set.cpp index de1db77..c756c9a 100644 --- a/app/src/transform_set.cpp +++ b/app/src/transform_set.cpp @@ -23,10 +23,20 @@ #include #include -#include namespace pj_bridge { +namespace { +// regex_match can throw at match time (error_complexity / error_stack): treat as no match. +bool topic_matches(const std::string& topic, const std::regex& pattern) { + try { + return std::regex_match(topic, pattern); + } catch (const std::regex_error&) { + return false; + } +} +} // namespace + bool BoundTransform::run(const std::string& topic, std::span in, std::vector& out) { const auto start = std::chrono::steady_clock::now(); tl::expected result; @@ -89,9 +99,6 @@ tl::expected, std::string> TransformSet::create( return tl::make_unexpected(where + "unknown or mistyped key '" + key + "'"); } } - if (rule.params.is_null()) { - rule.params = nlohmann::json::object(); - } if (auto added = set->add_rule(std::move(rule)); !added) { return tl::make_unexpected(where + added.error()); } @@ -126,57 +133,63 @@ tl::expected TransformSet::append_type_rule( Rule rule; rule.match_type = match_type; rule.transform = transform; - rule.params = nlohmann::json::object(); return add_rule(std::move(rule)); } +// bind() runs for every topic on every topic poll: each (topic, transform) problem is reported once. +bool TransformSet::log_once(const std::string& topic, const std::string& transform) { + return logged_.emplace(topic, transform).second; +} + std::shared_ptr TransformSet::bind(const std::string& topic, const std::string& source_type) { std::lock_guard lock(mutex_); if (auto it = bindings_.find(topic); it != bindings_.end()) { if (it->second->source_type == source_type) { return it->second; } - bindings_.erase(it); // the topic's type changed: match again + bindings_.erase(it); // the topic's type changed: match again, and let it be reported again + std::erase_if(logged_, [&](const auto& entry) { return entry.first == topic; }); } // ponytail: unmatched topics re-run every rule on each call (once per topic poll); // cache the misses too if topics x rules ever shows up in a profile. for (const auto& rule : rules_) { - try { - if (rule.match_type && *rule.match_type != source_type) { - continue; - } - if (rule.match_topic && !std::regex_match(topic, *rule.match_topic)) { - continue; - } - const auto& factory = factories_.at(rule.transform); - if (!factory.accepts(source_type)) { - if (warned_topics_.insert(topic).second) { - spdlog::warn( - "Transform '{}' matched topic '{}' but does not accept type '{}'; rule skipped", rule.transform, topic, - source_type); - } - continue; // a later rule (e.g. the strip_large_messages sugar) may still apply + if (rule.match_type && *rule.match_type != source_type) { + continue; + } + if (rule.match_topic && !topic_matches(topic, *rule.match_topic)) { + continue; + } + const auto& factory = factories_.at(rule.transform); + if (!factory.accepts(source_type)) { + if (log_once(topic, rule.transform)) { + spdlog::warn( + "Transform '{}' matched topic '{}' but does not accept type '{}'; rule skipped", rule.transform, topic, + source_type); } - auto bound = std::make_shared(); - bound->source_type = source_type; - bound->transform_name = rule.transform; - bound->output_type = factory.output_type(source_type); + continue; // a later rule (e.g. the strip_large_messages sugar) may still apply + } + + auto bound = std::make_shared(); + std::string error = "factory returned no transform"; + try { bound->transform = factory.create(source_type, rule.params); - if (!bound->transform) { - throw std::runtime_error("factory returned no transform"); - } - bindings_[topic] = bound; - spdlog::info("Topic '{}' ({}) -> transform '{}' -> {}", topic, source_type, rule.transform, bound->output_type); - return bound; } catch (const std::exception& e) { - if (warned_topics_.insert(topic).second) { // bind() runs on every topic poll: log once + error = e.what(); + } + if (!bound->transform) { + if (log_once(topic, rule.transform)) { spdlog::error( - "Transform '{}' could not be set up for '{}': {}; leaving it untransformed", rule.transform, topic, - e.what()); + "Transform '{}' could not be set up for '{}': {}; leaving it untransformed", rule.transform, topic, error); } return nullptr; } + bound->source_type = source_type; + bound->transform_name = rule.transform; + bound->output_type = bound->transform->output_type(source_type); + bindings_[topic] = bound; + spdlog::info("Topic '{}' ({}) -> transform '{}' -> {}", topic, source_type, rule.transform, bound->output_type); + return bound; } return nullptr; } @@ -187,22 +200,19 @@ std::shared_ptr TransformSet::find(const std::string& topic) con return it == bindings_.end() ? nullptr : it->second; } -std::string TransformSet::output_schema(const BoundTransform& bound, const std::string& source_schema) const { - return factories_.at(bound.transform_name).output_schema(bound.source_type, source_schema); -} - std::string TransformSet::stats_summary() const { std::lock_guard lock(mutex_); std::ostringstream os; for (const auto& [topic, bound] : bindings_) { const uint64_t n = bound->samples.load(); - if (n == 0 && bound->drops.load() == 0) { + const uint64_t dropped = bound->drops.load(); + if (n == 0 && dropped == 0) { continue; } const double in = static_cast(bound->in_bytes.load()); const double out = static_cast(bound->out_bytes.load()); - os << "\n " << topic << " [" << bound->transform_name << "]: " << n << " samples, " << bound->drops.load() - << " drops, ratio " << (out > 0 ? in / out : 0.0) << ", " << (n ? bound->micros.load() / n : 0) << " us/sample"; + os << "\n " << topic << " [" << bound->transform_name << "]: " << n << " samples, " << dropped << " drops, ratio " + << (out > 0 ? in / out : 0.0) << ", " << (n ? bound->micros.load() / n : 0) << " us/sample"; } return os.str(); } diff --git a/app/src/transforming_topic_source.cpp b/app/src/transforming_topic_source.cpp index 6ff9847..c87c7dd 100644 --- a/app/src/transforming_topic_source.cpp +++ b/app/src/transforming_topic_source.cpp @@ -39,7 +39,7 @@ std::vector TransformingTopicSource::get_topics() { std::string TransformingTopicSource::get_schema(const std::string& topic_name) { auto schema = inner_->get_schema(topic_name); if (auto bound = transforms_->find(topic_name)) { - return transforms_->output_schema(*bound, schema); + return bound->transform->output_schema(schema); } return schema; } diff --git a/docs/superpowers/specs/2026-09-20-message-transforms-design.md b/docs/superpowers/specs/2026-09-20-message-transforms-design.md index 5c072bc..1bcce8f 100644 --- a/docs/superpowers/specs/2026-09-20-message-transforms-design.md +++ b/docs/superpowers/specs/2026-09-20-message-transforms-design.md @@ -85,14 +85,14 @@ class MessageTransform { /// the client was told the output type at subscribe time. virtual tl::expected apply( std::span in, std::vector& out) = 0; + /// What clients are told they receive. Defaults: unchanged (as for `strip`). + virtual std::string output_type(const std::string& source_type) const; + virtual std::string output_schema(const std::string& source_schema) const; }; struct TransformFactory { std::function accepts; std::function(const nlohmann::json& params)> check_params; - std::function output_type; - std::function output_schema; std::function(const std::string& source_type, const nlohmann::json& params)> create; }; diff --git a/ros2/include/pj_bridge_ros2/strip_transform.hpp b/ros2/include/pj_bridge_ros2/strip_transform.hpp index 2b9021b..d4a042d 100644 --- a/ros2/include/pj_bridge_ros2/strip_transform.hpp +++ b/ros2/include/pj_bridge_ros2/strip_transform.hpp @@ -19,7 +19,7 @@ #pragma once -#include "pj_bridge/message_transform.hpp" +#include "pj_bridge/transform_set.hpp" namespace pj_bridge { @@ -27,4 +27,8 @@ namespace pj_bridge { /// Output type and schema equal the input's. Takes no params. TransformFactory make_strip_transform_factory(); +/// What `strip_large_messages: true` means: one `strip` rule per strippable +/// type, appended after whatever rules `transforms` already holds. +tl::expected append_strip_rules(TransformSet& transforms); + } // namespace pj_bridge diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 2537e54..18cae76 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -36,11 +37,50 @@ #include "pj_bridge/transform_set.hpp" #include "pj_bridge/transforming_topic_source.hpp" #include "pj_bridge/whitelist_filter.hpp" -#include "pj_bridge_ros2/message_stripper.hpp" #include "pj_bridge_ros2/ros2_subscription_manager.hpp" #include "pj_bridge_ros2/ros2_topic_source.hpp" #include "pj_bridge_ros2/strip_transform.hpp" +namespace { + +// The rules of `profile_path` followed by the strip_large_messages sugar, or +// nullptr when neither is set. Throws on any profile error: main() exits 1. +std::shared_ptr load_transforms(const std::string& profile_path, bool strip_large_messages) { + if (profile_path.empty() && !strip_large_messages) { + return nullptr; + } + nlohmann::json profile = {{"transforms", nlohmann::json::array()}}; + if (!profile_path.empty()) { + std::ifstream file(profile_path); + if (!file) { + throw std::runtime_error("Cannot open transform_profile '" + profile_path + "'"); + } + profile = nlohmann::json::parse(file, nullptr, /*allow_exceptions=*/false); + if (profile.is_discarded()) { + throw std::runtime_error("transform_profile '" + profile_path + "' is not valid JSON"); + } + } + + pj_bridge::TransformSet::FactoryMap factories; + factories["strip"] = pj_bridge::make_strip_transform_factory(); +#ifdef PJ_BRIDGE_HAS_CLOUDINI + factories["cloudini"] = pj_bridge::make_cloudini_transform_factory(); +#endif + + auto transforms = pj_bridge::TransformSet::create(profile, std::move(factories)); + if (!transforms) { + throw std::runtime_error(transforms.error()); + } + if (strip_large_messages) { // after the profile's rules, so an explicit rule wins + if (auto added = pj_bridge::append_strip_rules(**transforms); !added) { + throw std::runtime_error(added.error()); + } + } + return *transforms; +} + +} // namespace + int main(int argc, char** argv) { rclcpp::init(argc, argv); @@ -156,47 +196,8 @@ int main(int argc, char** argv) { // Create backend components std::shared_ptr topic_source = std::make_shared(node); - std::shared_ptr transforms; - if (!transform_profile.empty() || strip_large_messages) { - nlohmann::json profile = {{"transforms", nlohmann::json::array()}}; - if (!transform_profile.empty()) { - std::ifstream file(transform_profile); - if (!file) { - RCLCPP_ERROR(node->get_logger(), "Cannot open transform_profile '%s'", transform_profile.c_str()); - rclcpp::shutdown(); - return 1; - } - profile = nlohmann::json::parse(file, nullptr, /*allow_exceptions=*/false); - if (profile.is_discarded()) { - RCLCPP_ERROR(node->get_logger(), "transform_profile '%s' is not valid JSON", transform_profile.c_str()); - rclcpp::shutdown(); - return 1; - } - } - - pj_bridge::TransformSet::FactoryMap factories; - factories["strip"] = pj_bridge::make_strip_transform_factory(); -#ifdef PJ_BRIDGE_HAS_CLOUDINI - factories["cloudini"] = pj_bridge::make_cloudini_transform_factory(); -#endif - - auto created = pj_bridge::TransformSet::create(profile, std::move(factories)); - if (!created) { - RCLCPP_ERROR(node->get_logger(), "%s", created.error().c_str()); - rclcpp::shutdown(); - return 1; - } - transforms = *created; - - if (strip_large_messages) { // after the profile's rules, so an explicit rule wins - for (const auto& type : pj_bridge::MessageStripper::strippable_types()) { - // append_type_rule() is [[nodiscard]]; failure here would mean "strip" no longer - // accepts one of its own strippable_types(), which should never happen. - if (auto added = transforms->append_type_rule(type, "strip"); !added) { - RCLCPP_ERROR(node->get_logger(), "strip_large_messages: %s", added.error().c_str()); - } - } - } + auto transforms = load_transforms(transform_profile, strip_large_messages); + if (transforms) { topic_source = std::make_shared(topic_source, transforms); } diff --git a/ros2/src/strip_transform.cpp b/ros2/src/strip_transform.cpp index 836090f..36df419 100644 --- a/ros2/src/strip_transform.cpp +++ b/ros2/src/strip_transform.cpp @@ -66,10 +66,17 @@ TransformFactory make_strip_transform_factory() { } return {}; }; - f.output_type = [](const std::string& type) { return type; }; - f.output_schema = [](const std::string&, const std::string& schema) { return schema; }; f.create = [](const std::string& type, const nlohmann::json&) { return std::make_unique(type); }; return f; } +tl::expected append_strip_rules(TransformSet& transforms) { + for (const auto& type : MessageStripper::strippable_types()) { + if (auto added = transforms.append_type_rule(type, "strip"); !added) { + return added; + } + } + return {}; +} + } // namespace pj_bridge diff --git a/tests/unit/fake_transform.hpp b/tests/unit/fake_transform.hpp new file mode 100644 index 0000000..6a117fc --- /dev/null +++ b/tests/unit/fake_transform.hpp @@ -0,0 +1,80 @@ +/* + * Copyright (C) 2026 Davide Faconti + * + * This file is part of pj_bridge. + * + * pj_bridge is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * pj_bridge is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with pj_bridge. If not, see . + */ + +#pragma once + +#include +#include + +#include "pj_bridge/transform_set.hpp" + +namespace pj_bridge::test_helpers { + +/// Appends one marker byte (0xAB) to the input; advertises `output_type` and +/// "OUT:" + schema. With param {"fail": true}, apply() returns an error. +class FakeTransform : public MessageTransform { + public: + FakeTransform(bool fail, std::string output_type) : fail_(fail), output_type_(std::move(output_type)) {} + + tl::expected apply(std::span in, std::vector& out) override { + if (fail_) { + return tl::make_unexpected(std::string("boom")); + } + out.assign(in.begin(), in.end()); + out.push_back(std::byte{0xAB}); + return {}; + } + + std::string output_type(const std::string&) const override { + return output_type_; + } + + std::string output_schema(const std::string& source_schema) const override { + return "OUT:" + source_schema; + } + + private: + bool fail_; + std::string output_type_; +}; + +/// Factory accepting exactly `accepted_type`; its only param is "fail". +inline TransformFactory fake_factory(const std::string& accepted_type, const std::string& output_type = "pkg/msg/Out") { + TransformFactory f; + f.accepts = [accepted_type](const std::string& t) { return t == accepted_type; }; + f.check_params = [](const nlohmann::json& p) -> tl::expected { + for (auto it = p.begin(); it != p.end(); ++it) { + if (it.key() != "fail") { + return tl::make_unexpected("unknown param '" + it.key() + "'"); + } + } + return {}; + }; + f.create = [output_type](const std::string&, const nlohmann::json& p) { + return std::make_unique(p.value("fail", false), output_type); + }; + return f; +} + +/// Profile holding a single rule. +inline nlohmann::json rule(nlohmann::json fields) { + return nlohmann::json{{"transforms", nlohmann::json::array({std::move(fields)})}}; +} + +} // namespace pj_bridge::test_helpers diff --git a/tests/unit/test_cloudini_transform.cpp b/tests/unit/test_cloudini_transform.cpp index 0f44bdd..5db1ad1 100644 --- a/tests/unit/test_cloudini_transform.cpp +++ b/tests/unit/test_cloudini_transform.cpp @@ -60,15 +60,14 @@ TEST(CloudiniTransformTest, CompressesPointCloud2) { auto factory = make_cloudini_transform_factory(); EXPECT_TRUE(factory.accepts("sensor_msgs/msg/PointCloud2")); EXPECT_FALSE(factory.accepts("sensor_msgs/msg/Image")); - EXPECT_EQ(factory.output_type("sensor_msgs/msg/PointCloud2"), "point_cloud_interfaces/msg/CompressedPointCloud2"); - EXPECT_NE(factory.output_schema("sensor_msgs/msg/PointCloud2", "").find("format"), std::string::npos); - const nlohmann::json params = {{"resolution", 0.001}}; ASSERT_TRUE(factory.check_params(params).has_value()); EXPECT_FALSE(factory.check_params({{"resolutoin", 0.001}}).has_value()); EXPECT_FALSE(factory.check_params({{"resolution", -1.0}}).has_value()); auto transform = factory.create("sensor_msgs/msg/PointCloud2", params); + EXPECT_EQ(transform->output_type("sensor_msgs/msg/PointCloud2"), "point_cloud_interfaces/msg/CompressedPointCloud2"); + EXPECT_NE(transform->output_schema("").find("format"), std::string::npos); const auto in = make_cloud_cdr(10000); std::vector out; ASSERT_TRUE(transform->apply(in, out).has_value()); diff --git a/tests/unit/test_ros2_subscription_manager.cpp b/tests/unit/test_ros2_subscription_manager.cpp index 84b684a..f66259e 100644 --- a/tests/unit/test_ros2_subscription_manager.cpp +++ b/tests/unit/test_ros2_subscription_manager.cpp @@ -21,13 +21,14 @@ #include #include +#include #include #include #include #include +#include "fake_transform.hpp" #include "pj_bridge/transform_set.hpp" -#include "pj_bridge_ros2/message_stripper.hpp" #include "pj_bridge_ros2/ros2_subscription_manager.hpp" #include "pj_bridge_ros2/strip_transform.hpp" @@ -41,12 +42,25 @@ std::shared_ptr strip_everything() { auto set = TransformSet::create( nlohmann::json{{"transforms", nlohmann::json::array()}}, {{"strip", make_strip_transform_factory()}}) .value(); - for (const auto& type : MessageStripper::strippable_types()) { - EXPECT_TRUE(set->append_type_rule(type, "strip").has_value()); - } + EXPECT_TRUE(append_strip_rules(*set).has_value()); return set; } +// Publish `msg` and spin until `done()` or 5 s have passed. +template +void publish_until( + const rclcpp::Node::SharedPtr& node, const typename rclcpp::Publisher::SharedPtr& publisher, const MsgT& msg, + const std::function& done) { + rclcpp::executors::SingleThreadedExecutor executor; + executor.add_node(node); + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!done() && std::chrono::steady_clock::now() < deadline) { + publisher->publish(msg); + executor.spin_some(); + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } +} + // Publish an Image with a large data payload through the manager and return // the size of the serialized message delivered to the bridge callback. // `transforms` is nullptr for "no transform configured". @@ -76,15 +90,7 @@ size_t roundtrip_image_bytes(std::shared_ptr transforms, bool use_ img.encoding = "rgb8"; img.data.assign(static_cast(img.height) * img.step, 0xAB); - rclcpp::executors::SingleThreadedExecutor executor; - executor.add_node(node); - - auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); - while (received_size.load() == 0 && std::chrono::steady_clock::now() < deadline) { - publisher->publish(img); - executor.spin_some(); - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - } + publish_until(node, publisher, img, [&] { return received_size.load() != 0; }); manager->unsubscribe_all(); return received_size.load(); @@ -130,25 +136,11 @@ TEST_F(Ros2SubscriptionManagerTest, OptInStrippingRemovesData) { // output type) and forwards whatever bytes the bound transform produces. // --------------------------------------------------------------------------- TEST_F(Ros2SubscriptionManagerTest, SubscribesWithSourceTypeAndForwardsTransformedBytes) { - // A transform whose output type differs from the source type: the manager is - // asked to subscribe with the ADVERTISED type and must use the source type. - TransformFactory f; - f.accepts = [](const std::string& t) { return t == "std_msgs/msg/String"; }; - f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; - f.output_type = [](const std::string&) { return std::string("fake_msgs/msg/Out"); }; - f.output_schema = [](const std::string&, const std::string& s) { return s; }; - f.create = [](const std::string&, const nlohmann::json&) -> std::unique_ptr { - struct Marker : MessageTransform { - tl::expected apply(std::span, std::vector& out) override { - out.assign(3, std::byte{0x5A}); - return {}; - } - }; - return std::make_unique(); - }; + // The output type differs from the source type: the manager is asked to + // subscribe with the ADVERTISED type and must use the source type. auto set = TransformSet::create( - nlohmann::json{{"transforms", {{{"match_topic", "/transform_me"}, {"transform", "marker"}}}}}, - {{"marker", f}}) + test_helpers::rule({{"match_topic", "/transform_me"}, {"transform", "fake"}}), + {{"fake", test_helpers::fake_factory("std_msgs/msg/String", "fake_msgs/msg/Out")}}) .value(); ASSERT_NE(set->bind("/transform_me", "std_msgs/msg/String"), nullptr); // what get_topics would have done @@ -164,40 +156,20 @@ TEST_F(Ros2SubscriptionManagerTest, SubscribesWithSourceTypeAndForwardsTransform auto publisher = node->create_publisher("/transform_me", rclcpp::QoS(10)); std_msgs::msg::String msg; msg.data = "hello"; + publish_until(node, publisher, msg, [&] { return !received.empty(); }); - rclcpp::executors::SingleThreadedExecutor executor; - executor.add_node(node); - - auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); - while (received.empty() && std::chrono::steady_clock::now() < deadline) { - publisher->publish(msg); - executor.spin_some(); - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - } - - EXPECT_EQ(received, std::vector(3, std::byte{0x5A})); + ASSERT_FALSE(received.empty()); + EXPECT_EQ(received.back(), std::byte{0xAB}); // FakeTransform's marker: the transform ran } // The client was told the output type at subscribe time, so a sample whose // transform fails must be dropped, never forwarded untransformed. TEST_F(Ros2SubscriptionManagerTest, FailedTransformDropsTheSample) { - TransformFactory f; - f.accepts = [](const std::string& t) { return t == "std_msgs/msg/String"; }; - f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; - f.output_type = [](const std::string& t) { return t; }; - f.output_schema = [](const std::string&, const std::string& s) { return s; }; - f.create = [](const std::string&, const nlohmann::json&) -> std::unique_ptr { - struct Failing : MessageTransform { - tl::expected apply(std::span, std::vector&) override { - return tl::make_unexpected(std::string("boom")); - } - }; - return std::make_unique(); - }; - auto set = TransformSet::create( - nlohmann::json{{"transforms", {{{"match_topic", "/always_fails"}, {"transform", "failing"}}}}}, - {{"failing", f}}) - .value(); + auto set = + TransformSet::create( + test_helpers::rule({{"match_topic", "/always_fails"}, {"transform", "fake"}, {"params", {{"fail", true}}}}), + {{"fake", test_helpers::fake_factory("std_msgs/msg/String", "std_msgs/msg/String")}}) + .value(); auto bound = set->bind("/always_fails", "std_msgs/msg/String"); ASSERT_NE(bound, nullptr); @@ -211,15 +183,7 @@ TEST_F(Ros2SubscriptionManagerTest, FailedTransformDropsTheSample) { auto publisher = node->create_publisher("/always_fails", rclcpp::QoS(10)); std_msgs::msg::String msg; msg.data = "hello"; - rclcpp::executors::SingleThreadedExecutor executor; - executor.add_node(node); - - auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); - while (bound->drops.load() < 3 && std::chrono::steady_clock::now() < deadline) { - publisher->publish(msg); - executor.spin_some(); - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - } + publish_until(node, publisher, msg, [&] { return bound->drops.load() >= 3; }); EXPECT_GE(bound->drops.load(), 3u); EXPECT_EQ(forwarded.load(), 0); diff --git a/tests/unit/test_transform_set.cpp b/tests/unit/test_transform_set.cpp index c184c46..827a6b2 100644 --- a/tests/unit/test_transform_set.cpp +++ b/tests/unit/test_transform_set.cpp @@ -1,58 +1,25 @@ #include +#include +#include #include +#include #include #include +#include "fake_transform.hpp" #include "pj_bridge/transform_set.hpp" using namespace pj_bridge; +using namespace pj_bridge::test_helpers; using nlohmann::json; namespace { -/// Appends one marker byte to the input. `fail` makes apply() return an error. -class FakeTransform : public MessageTransform { - public: - explicit FakeTransform(bool fail) : fail_(fail) {} - tl::expected apply(std::span in, std::vector& out) override { - if (fail_) { - return tl::make_unexpected(std::string("boom")); - } - out.assign(in.begin(), in.end()); - out.push_back(std::byte{0xAB}); - return {}; - } - - private: - bool fail_; -}; - -TransformFactory fake_factory(const std::string& accepted_type) { - TransformFactory f; - f.accepts = [accepted_type](const std::string& t) { return t == accepted_type; }; - f.check_params = [](const json& p) -> tl::expected { - for (auto it = p.begin(); it != p.end(); ++it) { - if (it.key() != "fail") { - return tl::make_unexpected("unknown param '" + it.key() + "'"); - } - } - return {}; - }; - f.output_type = [](const std::string&) { return std::string("pkg/msg/Out"); }; - f.output_schema = [](const std::string&, const std::string& s) { return "OUT:" + s; }; - f.create = [](const std::string&, const json& p) { return std::make_unique(p.value("fail", false)); }; - return f; -} - TransformSet::FactoryMap factories() { return {{"fake", fake_factory("pkg/msg/In")}, {"other", fake_factory("pkg/msg/Other")}}; } -json rule(json fields) { - return json{{"transforms", json::array({std::move(fields)})}}; -} - } // namespace TEST(TransformSetTest, MatchesByType) { @@ -178,6 +145,35 @@ TEST(TransformSetTest, RunIsSafeConcurrentlyWithBindFindAndStats) { EXPECT_GT(bound->samples.load(), 0u); } +// One log-once set serves two conditions. A "does not accept" warning for a +// topic must not swallow the later, more serious "could not be set up" error. +TEST(TransformSetTest, SkippedRuleWarningDoesNotHideALaterSetupError) { + std::ostringstream captured; + auto previous = spdlog::default_logger(); + spdlog::set_default_logger( + std::make_shared("capture", std::make_shared(captured))); + + auto broken = fake_factory("pkg/msg/In"); + broken.create = [](const std::string&, const json&) -> std::unique_ptr { + throw std::runtime_error("no codec"); + }; + json profile = { + {"transforms", json::array( + {{{"match_topic", "/a"}, {"transform", "other"}}, // accepts Other only + {{"match_type", "pkg/msg/In"}, {"transform", "broken"}}})}}; + auto set = TransformSet::create(profile, {{"other", fake_factory("pkg/msg/Other")}, {"broken", broken}}); + ASSERT_TRUE(set.has_value()) << set.error(); + + EXPECT_EQ((*set)->bind("/a", "pkg/msg/In"), nullptr); + (*set)->bind("/a", "pkg/msg/In"); // second poll: nothing new may be logged + spdlog::set_default_logger(previous); + + const std::string log = captured.str(); + EXPECT_NE(log.find("does not accept"), std::string::npos) << log; + EXPECT_NE(log.find("could not be set up"), std::string::npos) << log; + EXPECT_EQ(log.find("could not be set up"), log.rfind("could not be set up")) << "logged more than once:\n" << log; +} + TEST(TransformSetTest, RebindsWhenSourceTypeChanges) { auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}}), factories()); ASSERT_TRUE(set.has_value()) << set.error(); diff --git a/tests/unit/test_transforming_topic_source.cpp b/tests/unit/test_transforming_topic_source.cpp index a4578bd..7fd8e90 100644 --- a/tests/unit/test_transforming_topic_source.cpp +++ b/tests/unit/test_transforming_topic_source.cpp @@ -19,6 +19,7 @@ #include +#include "fake_transform.hpp" #include "pj_bridge/transforming_topic_source.hpp" using namespace pj_bridge; @@ -40,23 +41,11 @@ class StubSource : public TopicSourceInterface { } }; -class PassThrough : public MessageTransform { - public: - tl::expected apply(std::span in, std::vector& out) override { - out.assign(in.begin(), in.end()); - return {}; - } -}; - std::shared_ptr make_set() { - TransformFactory f; - f.accepts = [](const std::string& t) { return t == "pkg/msg/In"; }; - f.check_params = [](const nlohmann::json&) -> tl::expected { return {}; }; - f.output_type = [](const std::string&) { return std::string("pkg/msg/Out"); }; - f.output_schema = [](const std::string&, const std::string& s) { return "OUT:" + s; }; - f.create = [](const std::string&, const nlohmann::json&) { return std::make_unique(); }; - nlohmann::json profile = {{"transforms", {{{"match_type", "pkg/msg/In"}, {"transform", "t"}}}}}; - return TransformSet::create(profile, {{"t", f}}).value(); + return TransformSet::create( + test_helpers::rule({{"match_type", "pkg/msg/In"}, {"transform", "t"}}), + {{"t", test_helpers::fake_factory("pkg/msg/In")}}) + .value(); } } // namespace From a61d665f43d5fb857dd115e8c8075f99a6d66ca1 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 22:12:21 +0200 Subject: [PATCH 17/19] build: Cloudini 1.3.1; drop the embedding workarounds and duplicated validation Cloudini 1.3.1 (facontidavide/cloudini#141, #142) can be embedded as-is and validates malformed clouds itself, so this removes what compensated for that: the scoped fetch function (hidden ament_cmake, disabled zstd/PCL lookups, undone CMAKE_BUILD_TYPE, FetchContent_Populate + EXCLUDE_FROM_ALL), the data_path.hpp relocation, and check_cloud() in the transform. The InconsistentCloudMetadataIsRejected test stays and now covers the upstream check. Clean rebuild against the released tarball: 303 tests, static link, no PCL, nothing of Cloudini installed. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CLAUDE.md | 2 +- CMakeLists.txt | 57 +++++++--------------------------- README.md | 2 +- app/src/cloudini_transform.cpp | 20 ------------ 4 files changed, 13 insertions(+), 68 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 813dd5c..79d6c4b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -187,7 +187,7 @@ Then, for each message in the (compressed) payload: - **tl::expected** — error handling (header-only, vendored in 3rdparty/) ### Core (optional) -- **Cloudini** (`cloudini_lib`) — point cloud compression for the `cloudini` transform. `find_package(cloudini_lib)` first, else fetched at configure time (pinned tag `1.3.0`, static) unless `-DPJ_BRIDGE_FETCH_CLOUDINI=OFF`. A build with neither simply lacks the `cloudini` transform (`PJ_BRIDGE_HAS_CLOUDINI` undefined); `strip` and the rest of the transform machinery are unaffected. +- **Cloudini** (`cloudini_lib`) — point cloud compression for the `cloudini` transform. `find_package(cloudini_lib)` first, else fetched at configure time (pinned tag `1.3.1`, static) unless `-DPJ_BRIDGE_FETCH_CLOUDINI=OFF`. A build with neither simply lacks the `cloudini` transform (`PJ_BRIDGE_HAS_CLOUDINI` undefined); `strip` and the rest of the transform machinery are unaffected. ### ROS2 Backend - `rclcpp`, `ament_index_cpp`, `ament_cmake` diff --git a/CMakeLists.txt b/CMakeLists.txt index 3be8c3c..4da834c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -90,54 +90,20 @@ endif() # Cloudini (optional) — point cloud compression transform option(PJ_BRIDGE_FETCH_CLOUDINI "Fetch Cloudini when cloudini_lib is not installed" ON) -# 1.2 introduced the v5 codec and applyVizLossyPreprocessing used by the transform. -find_package(cloudini_lib 1.2 QUIET) - -# Builds Cloudini as a plain static library inside this project. A function, so -# that everything set() below is scoped to Cloudini's directory and needs no restoring. -function(pj_bridge_fetch_cloudini) +# 1.3.1 validates malformed clouds itself and can be embedded without workarounds. +find_package(cloudini_lib 1.3.1 QUIET) +if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) + message(STATUS "cloudini_lib not found — fetching via FetchContent") FetchContent_Declare( cloudini - URL https://github.com/facontidavide/cloudini/archive/refs/tags/1.3.0.tar.gz - URL_HASH SHA256=26c9322d5a08694b9b24bbec975b7814b05cf7d4962327f35266b38bb15b5e66 + URL https://github.com/facontidavide/cloudini/archive/refs/tags/1.3.1.tar.gz + URL_HASH SHA256=d8c0e265bb841f59002e996bc957b4f0845b5399deb62b3247dc11115b48b4d9 + SOURCE_SUBDIR cloudini_lib ) - set(CLOUDINI_BUILD_TOOLS OFF CACHE BOOL "" FORCE) - set(CLOUDINI_BUILD_BENCHMARKS OFF CACHE BOOL "" FORCE) - # Use the zstd found above instead of a second, vendored copy in the same binary. - # FindZSTD already defined zstd::libzstd_static; Cloudini's own - # find_package(zstd CONFIG) would clash with those targets, so it is disabled. + # Reuse the zstd found above instead of a second, vendored copy in the same binary. set(CLOUDINI_FORCE_VENDORED_DEPS OFF CACHE BOOL "" FORCE) - set(CMAKE_DISABLE_FIND_PACKAGE_zstd ON) - # Cloudini becomes a SHARED ament package (install rules, ament_package()) when - # it finds ament_cmake: hide it. The disabled find_package() leaves our own - # earlier result in place, hence ament_cmake_FOUND as well. - set(CMAKE_DISABLE_FIND_PACKAGE_ament_cmake ON) - set(ament_cmake_FOUND FALSE) - set(BUILD_SHARED_LIBS OFF) - set(BUILD_TESTING OFF) - set(CMAKE_DISABLE_FIND_PACKAGE_PCL ON) # optional in Cloudini; would become a PUBLIC link dependency of ours - set(build_type_was_set "${CMAKE_BUILD_TYPE}") - - # FetchContent_Populate + add_subdirectory(EXCLUDE_FROM_ALL) keeps Cloudini's - # install rules out of ours; FetchContent_Declare(EXCLUDE_FROM_ALL) needs CMake 3.28. - if(POLICY CMP0169) - cmake_policy(SET CMP0169 OLD) - endif() - FetchContent_Populate(cloudini) - add_subdirectory(${cloudini_SOURCE_DIR}/cloudini_lib ${cloudini_BINARY_DIR} EXCLUDE_FROM_ALL) - - if(NOT build_type_was_set) # Cloudini forces Release into the cache when no build type is set - unset(CMAKE_BUILD_TYPE CACHE) - endif() + FetchContent_MakeAvailable(cloudini) set_target_properties(cloudini_lib PROPERTIES POSITION_INDEPENDENT_CODE ON) - if(NOT TARGET cloudini::cloudini_lib) - add_library(cloudini::cloudini_lib ALIAS cloudini_lib) - endif() -endfunction() - -if(NOT cloudini_lib_FOUND AND PJ_BRIDGE_FETCH_CLOUDINI) - message(STATUS "cloudini_lib not found — fetching via FetchContent") - pj_bridge_fetch_cloudini() set(cloudini_lib_FOUND TRUE) endif() @@ -151,10 +117,9 @@ find_package(nlohmann_json REQUIRED) # Data path for tests # ============================================================================ get_filename_component(DATA_PATH "${CMAKE_CURRENT_LIST_DIR}/DATA" ABSOLUTE) -# Own directory: Cloudini writes a data_path.hpp of its own to ${CMAKE_BINARY_DIR}/include. -configure_file(data_path.hpp.in "${CMAKE_BINARY_DIR}/pj_include/data_path.hpp" @ONLY) +configure_file(data_path.hpp.in "${CMAKE_BINARY_DIR}/include/data_path.hpp" @ONLY) add_library(data_path INTERFACE IMPORTED) -target_include_directories(data_path INTERFACE ${CMAKE_BINARY_DIR}/pj_include) +target_include_directories(data_path INTERFACE ${CMAKE_BINARY_DIR}/include) # ============================================================================ # pj_bridge_app — backend-agnostic core library diff --git a/README.md b/README.md index a5bec4b..c481cf6 100644 --- a/README.md +++ b/README.md @@ -115,7 +115,7 @@ All dependencies (spdlog, nlohmann_json, ZSTD) are provided by the dependency ma TLS (`wss://`) support depends on IXWebSocket being built with OpenSSL. The CMake option `PJ_BRIDGE_TLS` (default `ON`) controls this for the FetchContent path (`-DPJ_BRIDGE_TLS=OFF` to disable); a system/conda-provided IXWebSocket must likewise have been built with TLS. See [docs/API.md](docs/API.md#tls--wss) for details. -Cloudini (the `cloudini` message transform) is optional: `find_package(cloudini_lib)` is tried first (version >= 1.2), otherwise it is fetched at configure time and linked statically. Builds without network access must either have `cloudini_lib` installed or pass `-DPJ_BRIDGE_FETCH_CLOUDINI=OFF`, which builds the bridge without that transform. +Cloudini (the `cloudini` message transform) is optional: `find_package(cloudini_lib)` is tried first (version >= 1.3.1), otherwise it is fetched at configure time and linked statically. Builds without network access must either have `cloudini_lib` installed or pass `-DPJ_BRIDGE_FETCH_CLOUDINI=OFF`, which builds the bridge without that transform. ### ROS2 — Pixi diff --git a/app/src/cloudini_transform.cpp b/app/src/cloudini_transform.cpp index b5d2176..70356e6 100644 --- a/app/src/cloudini_transform.cpp +++ b/app/src/cloudini_transform.cpp @@ -29,23 +29,6 @@ namespace { constexpr const char* kPointCloud2 = "sensor_msgs/msg/PointCloud2"; -// The encoder trusts field offsets, and the output header repeats width/height: -// a cloud whose metadata disagrees with its payload must not reach it. -tl::expected check_cloud(const cloudini_ros::RosPointCloud2& cloud) { - const uint64_t declared = uint64_t{cloud.width} * cloud.height * cloud.point_step; - if (declared != cloud.data.size()) { - return tl::make_unexpected( - "width*height*point_step = " + std::to_string(declared) + " but data has " + std::to_string(cloud.data.size()) + - " bytes"); - } - for (const auto& field : cloud.fields) { - if (uint64_t{field.offset} + static_cast(Cloudini::SizeOf(field.type)) > cloud.point_step) { - return tl::make_unexpected("field '" + field.name + "' extends past point_step"); - } - } - return {}; -} - class CloudiniTransform : public MessageTransform { public: explicit CloudiniTransform(const nlohmann::json& params) @@ -61,9 +44,6 @@ class CloudiniTransform : public MessageTransform { try { const Cloudini::ConstBufferView raw(reinterpret_cast(in.data()), in.size()); auto cloud = cloudini_ros::getDeserializedPointCloudMessage(raw); - if (auto valid = check_cloud(cloud); !valid) { - return valid; - } cloudini_ros::applyResolutionProfile(profile_, cloud.fields, resolution_); if (viz_preprocessing_) { cloudini_ros::applyVizLossyPreprocessing(cloud); From 872b48c549b5dd029daf7e00c7e68655d6f670bc Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 22:30:41 +0200 Subject: [PATCH 18/19] feat(transform)!: match_type is mandatory; a transform/type mismatch is always a startup error A rule matching by topic name alone could only be checked against a topic's type once that topic was discovered, so pairing e.g. cloudini with a non point cloud type surfaced as a runtime warning. Checking against the topics present at startup would depend on launch order (DDS discovery is incomplete when the bridge starts, and topics can appear later). match_type is now required and match_topic only narrows it: accepts(match_type) is verified when the profile is parsed, deterministically and independent of what is running. The runtime skip-and-warn path is removed, since the situation can no longer be expressed. Test first: All/TransformSetBadProfileTest/topic_only_rule failed before the change. The two tests describing the runtime skip are replaced by RuleNeverAppliesToATopicOfAnotherType. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CHANGELOG.rst | 5 +- app/include/pj_bridge/transform_set.hpp | 2 +- app/src/transform_set.cpp | 22 +++---- docs/API.md | 28 ++++----- .../2026-09-20-message-transforms-design.md | 11 ++-- tests/unit/test_ros2_subscription_manager.cpp | 16 +++-- tests/unit/test_transform_set.cpp | 58 ++++++++++--------- 7 files changed, 72 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7f16be9..c549262 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,8 +7,9 @@ Forthcoming * Message transforms (ROS2 only): operator-configured, off by default, a topic is transformed for all subscribers or none — no per-client negotiation. New ``transform_profile`` parameter points to a JSON file of - ordered per-topic rules (``match_type`` exact and/or ``match_topic`` - full-match regex, first match wins). A transformed topic is advertised + ordered per-topic rules (``match_type`` exact and required, optionally + narrowed by a ``match_topic`` full-match regex; first match wins; a transform + paired with a type it cannot handle is a startup error). A transformed topic is advertised with its transform's output type/schema, plus an optional ``source_type`` on ``get_topics`` entries (not yet on ``topics_changed`` entries); new ``message_transforms`` server capability. diff --git a/app/include/pj_bridge/transform_set.hpp b/app/include/pj_bridge/transform_set.hpp index e9e7a4d..9bbdcf4 100644 --- a/app/include/pj_bridge/transform_set.hpp +++ b/app/include/pj_bridge/transform_set.hpp @@ -79,7 +79,7 @@ class TransformSet { private: struct Rule { - std::optional match_type; + std::string match_type; // mandatory std::optional match_topic; std::string transform; nlohmann::json params = nlohmann::json::object(); diff --git a/app/src/transform_set.cpp b/app/src/transform_set.cpp index c756c9a..b722bc9 100644 --- a/app/src/transform_set.cpp +++ b/app/src/transform_set.cpp @@ -107,8 +107,10 @@ tl::expected, std::string> TransformSet::create( } tl::expected TransformSet::add_rule(Rule rule) { - if (!rule.match_type && !rule.match_topic) { - return tl::make_unexpected(std::string("needs match_type and/or match_topic")); + // Mandatory, so that a transform applied to a type it cannot handle is a + // startup error: topic types are only known once topics are discovered. + if (rule.match_type.empty()) { + return tl::make_unexpected(std::string("needs 'match_type' ('match_topic' can only narrow it)")); } if (rule.transform.empty()) { return tl::make_unexpected(std::string("missing 'transform'")); @@ -120,8 +122,8 @@ tl::expected TransformSet::add_rule(Rule rule) { if (auto ok = factory->second.check_params(rule.params); !ok) { return tl::make_unexpected("transform '" + rule.transform + "': " + ok.error()); } - if (rule.match_type && !factory->second.accepts(*rule.match_type)) { - return tl::make_unexpected("transform '" + rule.transform + "' does not accept type '" + *rule.match_type + "'"); + if (!factory->second.accepts(rule.match_type)) { + return tl::make_unexpected("transform '" + rule.transform + "' does not accept type '" + rule.match_type + "'"); } std::lock_guard lock(mutex_); rules_.push_back(std::move(rule)); @@ -154,21 +156,13 @@ std::shared_ptr TransformSet::bind(const std::string& topic, con // ponytail: unmatched topics re-run every rule on each call (once per topic poll); // cache the misses too if topics x rules ever shows up in a profile. for (const auto& rule : rules_) { - if (rule.match_type && *rule.match_type != source_type) { + if (rule.match_type != source_type) { continue; } if (rule.match_topic && !topic_matches(topic, *rule.match_topic)) { continue; } - const auto& factory = factories_.at(rule.transform); - if (!factory.accepts(source_type)) { - if (log_once(topic, rule.transform)) { - spdlog::warn( - "Transform '{}' matched topic '{}' but does not accept type '{}'; rule skipped", rule.transform, topic, - source_type); - } - continue; // a later rule (e.g. the strip_large_messages sugar) may still apply - } + const auto& factory = factories_.at(rule.transform); // accepts(match_type) was checked in add_rule() auto bound = std::make_shared(); std::string error = "factory returned no transform"; diff --git a/docs/API.md b/docs/API.md index 1ea9cbc..2c26afc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -310,22 +310,22 @@ for their fleet of clients. See also `docs/transform_profile.example.json`. -Each rule in `transforms` needs at least one of: +Each rule in `transforms` has: -- `match_type` — the topic's type, matched **exactly**. -- `match_topic` — full-match ECMAScript regex against the topic name, same - semantics as [`topic_whitelist`](#topic-whitelist) (`std::regex_match`, - not a substring search). +- `match_type` (**required**) — the topic's type, matched **exactly**. +- `match_topic` (optional) — full-match ECMAScript regex against the topic + name, same semantics as [`topic_whitelist`](#topic-whitelist) + (`std::regex_match`, not a substring search). It narrows the rule to some of + the topics of that type. +- `transform` (**required**) and `params` (optional) — see below. -When both are present, **both** must match. Rules are evaluated in order -and the **first match wins** — with the one exception described next. +Rules are evaluated in order and the **first match wins**. -A rule that matches a topic by `match_topic` only (no `match_type`) whose -named transform does not accept that topic's actual type is **skipped**: a -one-time warning is logged for that topic and evaluation continues with the -next rule. A `match_type` rule can never hit this case — `TransformSet` -rejects a rule at startup whose transform does not accept its declared -`match_type` (see below). +`match_type` is required so that pairing a transform with a type it cannot +handle is always a **startup error**: topic types are only known once topics +are discovered, so a rule matching by name alone could not be checked until a +mismatching topic appeared. For a transform that handles several types (such +as `strip`), write one rule per type. ### `strip_large_messages` interaction @@ -344,7 +344,7 @@ The server refuses to start (exits with a non-zero status) on any of: - The file is not valid JSON. - An unknown top-level key (anything other than `transforms`). - An unknown or mistyped rule key (e.g. `params` that isn't an object). -- A rule with neither `match_type` nor `match_topic`. +- A rule without `match_type`. - A rule missing `transform`. - A rule naming an unknown transform — including `cloudini` in a build without Cloudini compiled in (see `CLAUDE.md` / `README.md` for the build diff --git a/docs/superpowers/specs/2026-09-20-message-transforms-design.md b/docs/superpowers/specs/2026-09-20-message-transforms-design.md index 1bcce8f..43a5306 100644 --- a/docs/superpowers/specs/2026-09-20-message-transforms-design.md +++ b/docs/superpowers/specs/2026-09-20-message-transforms-design.md @@ -158,8 +158,9 @@ ROS2 parameter `transform_profile` (string, default empty = disabled). } ``` -- At least one of `match_type` (exact) / `match_topic` (full-match regex, same - semantics as `topic_whitelist`); when both are present both must match. +- `match_type` (exact) is required; `match_topic` (full-match regex, same + semantics as `topic_whitelist`) optionally narrows it. Requiring the type + makes every transform/type mismatch a startup error. - Ordered, first match wins. - `strip_large_messages: true` appends one `strip` rule per strippable type **after** the profile's rules, so an explicit profile rule wins. @@ -167,10 +168,8 @@ ROS2 parameter `transform_profile` (string, default empty = disabled). **Fail at startup** (clear error, non-zero exit): unreadable or malformed file, unknown top-level/rule/param key, unknown transform name, `cloudini` named in a build without Cloudini, invalid regex, params rejected by the -transform's `check_params`, a `match_type` the named transform does not `accept()`. A rule without -`match_type` whose transform does not `accept()` a topic it matched by -`match_topic` is reported once as a warning and that rule is skipped for the -topic — later rules still apply (types are only known at discovery time). A +transform's `check_params`, a rule without `match_type`, a `match_type` the named +transform does not `accept()`. A transform that cannot be instantiated for a topic (factory throws or returns nothing) is logged as an error and the topic is left untransformed. diff --git a/tests/unit/test_ros2_subscription_manager.cpp b/tests/unit/test_ros2_subscription_manager.cpp index f66259e..3065c03 100644 --- a/tests/unit/test_ros2_subscription_manager.cpp +++ b/tests/unit/test_ros2_subscription_manager.cpp @@ -139,7 +139,8 @@ TEST_F(Ros2SubscriptionManagerTest, SubscribesWithSourceTypeAndForwardsTransform // The output type differs from the source type: the manager is asked to // subscribe with the ADVERTISED type and must use the source type. auto set = TransformSet::create( - test_helpers::rule({{"match_topic", "/transform_me"}, {"transform", "fake"}}), + test_helpers::rule( + {{"match_type", "std_msgs/msg/String"}, {"match_topic", "/transform_me"}, {"transform", "fake"}}), {{"fake", test_helpers::fake_factory("std_msgs/msg/String", "fake_msgs/msg/Out")}}) .value(); ASSERT_NE(set->bind("/transform_me", "std_msgs/msg/String"), nullptr); // what get_topics would have done @@ -165,11 +166,14 @@ TEST_F(Ros2SubscriptionManagerTest, SubscribesWithSourceTypeAndForwardsTransform // The client was told the output type at subscribe time, so a sample whose // transform fails must be dropped, never forwarded untransformed. TEST_F(Ros2SubscriptionManagerTest, FailedTransformDropsTheSample) { - auto set = - TransformSet::create( - test_helpers::rule({{"match_topic", "/always_fails"}, {"transform", "fake"}, {"params", {{"fail", true}}}}), - {{"fake", test_helpers::fake_factory("std_msgs/msg/String", "std_msgs/msg/String")}}) - .value(); + auto set = TransformSet::create( + test_helpers::rule( + {{"match_type", "std_msgs/msg/String"}, + {"match_topic", "/always_fails"}, + {"transform", "fake"}, + {"params", {{"fail", true}}}}), + {{"fake", test_helpers::fake_factory("std_msgs/msg/String", "std_msgs/msg/String")}}) + .value(); auto bound = set->bind("/always_fails", "std_msgs/msg/String"); ASSERT_NE(bound, nullptr); diff --git a/tests/unit/test_transform_set.cpp b/tests/unit/test_transform_set.cpp index 827a6b2..00cecf4 100644 --- a/tests/unit/test_transform_set.cpp +++ b/tests/unit/test_transform_set.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -46,9 +47,10 @@ TEST(TransformSetTest, TopicRegexIsFullMatchAndCombinesWithType) { TEST(TransformSetTest, FirstMatchingRuleWinsAndAppendedRulesComeLast) { json profile = { - {"transforms", json::array( - {{{"match_topic", "/a"}, {"transform", "fake"}, {"params", {{"fail", true}}}}, - {{"match_type", "pkg/msg/In"}, {"transform", "fake"}}})}}; + {"transforms", + json::array( + {{{"match_type", "pkg/msg/In"}, {"match_topic", "/a"}, {"transform", "fake"}, {"params", {{"fail", true}}}}, + {{"match_type", "pkg/msg/In"}, {"transform", "fake"}}})}}; auto set = TransformSet::create(profile, factories()); ASSERT_TRUE(set.has_value()) << set.error(); ASSERT_TRUE((*set)->append_type_rule("pkg/msg/Other", "other").has_value()); @@ -59,19 +61,17 @@ TEST(TransformSetTest, FirstMatchingRuleWinsAndAppendedRulesComeLast) { EXPECT_EQ((*set)->bind("/c", "pkg/msg/Other")->transform_name, "other"); } -TEST(TransformSetTest, TopicOnlyRuleWithNonAcceptedTypeLeavesTopicUntransformed) { - auto set = TransformSet::create(rule({{"match_topic", "/a"}, {"transform", "fake"}}), factories()); - ASSERT_TRUE(set.has_value()) << set.error(); - EXPECT_EQ((*set)->bind("/a", "pkg/msg/Unrelated"), nullptr); -} - -TEST(TransformSetTest, NonAcceptingRuleIsSkippedSoLaterRulesStillApply) { - auto set = TransformSet::create(rule({{"match_topic", "/lidar/.*"}, {"transform", "fake"}}), factories()); +// A transform can only be paired with a type it accepts, and that is settled at +// startup: a topic of another type is simply not matched by the rule. +TEST(TransformSetTest, RuleNeverAppliesToATopicOfAnotherType) { + auto set = TransformSet::create( + rule({{"match_type", "pkg/msg/In"}, {"match_topic", "/lidar/.*"}, {"transform", "fake"}}), factories()); ASSERT_TRUE(set.has_value()) << set.error(); ASSERT_TRUE((*set)->append_type_rule("pkg/msg/Other", "other").has_value()); - auto bound = (*set)->bind("/lidar/image", "pkg/msg/Other"); + auto bound = (*set)->bind("/lidar/image", "pkg/msg/Other"); // falls through to the later rule ASSERT_NE(bound, nullptr); EXPECT_EQ(bound->transform_name, "other"); + EXPECT_EQ((*set)->bind("/lidar/unrelated", "pkg/msg/Unrelated"), nullptr); } TEST(TransformSetTest, FactoryReturningNullOrThrowingLeavesTopicUntransformed) { @@ -84,7 +84,10 @@ TEST(TransformSetTest, FactoryReturningNullOrThrowingLeavesTopicUntransformed) { }; json profile = { {"transforms", json::array( - {{{"match_topic", "/throws"}, {"transform", "broken"}, {"params", {{"fail", true}}}}, + {{{"match_type", "pkg/msg/In"}, + {"match_topic", "/throws"}, + {"transform", "broken"}, + {"params", {{"fail", true}}}}, {{"match_type", "pkg/msg/In"}, {"transform", "broken"}}})}}; auto set = TransformSet::create(profile, {{"broken", broken}}); ASSERT_TRUE(set.has_value()) << set.error(); @@ -145,9 +148,9 @@ TEST(TransformSetTest, RunIsSafeConcurrentlyWithBindFindAndStats) { EXPECT_GT(bound->samples.load(), 0u); } -// One log-once set serves two conditions. A "does not accept" warning for a -// topic must not swallow the later, more serious "could not be set up" error. -TEST(TransformSetTest, SkippedRuleWarningDoesNotHideALaterSetupError) { +// bind() runs for every topic on every topic poll: a transform that cannot be +// set up must be reported once, not once per second. +TEST(TransformSetTest, SetupErrorIsLoggedOncePerTopic) { std::ostringstream captured; auto previous = spdlog::default_logger(); spdlog::set_default_logger( @@ -157,21 +160,18 @@ TEST(TransformSetTest, SkippedRuleWarningDoesNotHideALaterSetupError) { broken.create = [](const std::string&, const json&) -> std::unique_ptr { throw std::runtime_error("no codec"); }; - json profile = { - {"transforms", json::array( - {{{"match_topic", "/a"}, {"transform", "other"}}, // accepts Other only - {{"match_type", "pkg/msg/In"}, {"transform", "broken"}}})}}; - auto set = TransformSet::create(profile, {{"other", fake_factory("pkg/msg/Other")}, {"broken", broken}}); + auto set = TransformSet::create(rule({{"match_type", "pkg/msg/In"}, {"transform", "broken"}}), {{"broken", broken}}); ASSERT_TRUE(set.has_value()) << set.error(); EXPECT_EQ((*set)->bind("/a", "pkg/msg/In"), nullptr); - (*set)->bind("/a", "pkg/msg/In"); // second poll: nothing new may be logged + (*set)->bind("/a", "pkg/msg/In"); // second poll + (*set)->bind("/b", "pkg/msg/In"); // another topic is reported separately spdlog::set_default_logger(previous); const std::string log = captured.str(); - EXPECT_NE(log.find("does not accept"), std::string::npos) << log; - EXPECT_NE(log.find("could not be set up"), std::string::npos) << log; - EXPECT_EQ(log.find("could not be set up"), log.rfind("could not be set up")) << "logged more than once:\n" << log; + EXPECT_EQ(std::count(log.begin(), log.end(), '\n'), 2) << log; + EXPECT_NE(log.find("'/a'"), std::string::npos) << log; + EXPECT_NE(log.find("'/b'"), std::string::npos) << log; } TEST(TransformSetTest, RebindsWhenSourceTypeChanges) { @@ -185,7 +185,10 @@ TEST(TransformSetTest, RebindsWhenSourceTypeChanges) { TEST(TransformSetTest, RunCountsSamplesBytesAndDrops) { json profile = { {"transforms", json::array( - {{{"match_topic", "/bad"}, {"transform", "fake"}, {"params", {{"fail", true}}}}, + {{{"match_type", "pkg/msg/In"}, + {"match_topic", "/bad"}, + {"transform", "fake"}, + {"params", {{"fail", true}}}}, {{"match_type", "pkg/msg/In"}, {"transform", "fake"}}})}}; auto set = TransformSet::create(profile, factories()); ASSERT_TRUE(set.has_value()) << set.error(); @@ -225,11 +228,12 @@ INSTANTIATE_TEST_SUITE_P( BadProfile{"transforms_not_an_array", json{{"transforms", 5}}}, BadProfile{"rule_not_an_object", json{{"transforms", json::array({5})}}}, BadProfile{"mistyped_rule_key", rule({{"match_type", 5}, {"transform", "fake"}})}, + BadProfile{"topic_only_rule", rule({{"match_topic", "/lidar/.*"}, {"transform", "fake"}})}, BadProfile{"rule_without_matcher", rule({{"transform", "fake"}})}, BadProfile{"rule_without_transform", rule({{"match_type", "pkg/msg/In"}})}, BadProfile{"unknown_rule_key", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"oops", 1}})}, BadProfile{"unknown_transform", rule({{"match_type", "pkg/msg/In"}, {"transform", "nope"}})}, - BadProfile{"invalid_regex", rule({{"match_topic", "("}, {"transform", "fake"}})}, + BadProfile{"invalid_regex", rule({{"match_type", "pkg/msg/In"}, {"match_topic", "("}, {"transform", "fake"}})}, BadProfile{ "unknown_param", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"params", {{"q", 1}}}})}, BadProfile{"type_not_accepted", rule({{"match_type", "pkg/msg/Unrelated"}, {"transform", "fake"}})}), From c02385eecaa10e5f04fb18f0a94773407bc0bfa3 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 20 Sep 2026 22:44:41 +0200 Subject: [PATCH 19/19] fix(transform): startup errors say why and where; harden tests From the review of the mandatory-match_type change: - every rejected profile is now tested for its REASON, not just for being rejected (a case could otherwise pass for the wrong reason); adds the empty match_type case - 'does not accept type' shows the expected spelling (the likely mistake is the short 'sensor_msgs/PointCloud2'); an empty match_type and a wrongly typed key get accurate wording; main() names the profile file - the log-capturing test restores the default logger on every exit path (its sink pointed at a stack stream) and counts the message, not newlines - setup_failed_ replaces the (topic, transform) log-once set and its helper, over-general now that one call site remains - the implementation plan is marked historical Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME --- CHANGELOG.rst | 2 +- CLAUDE.md | 4 +- app/include/pj_bridge/transform_set.hpp | 6 +- app/src/transform_set.cpp | 21 +++--- .../plans/2026-09-20-message-transforms.md | 2 + ros2/src/main.cpp | 2 +- tests/unit/test_transform_set.cpp | 64 +++++++++++++------ 7 files changed, 62 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c549262..faa6b02 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -32,7 +32,7 @@ Forthcoming microseconds/sample) logged with the final statistics at shutdown. * The project now requires **C++20** (``std::span``, and ``cloudini_lib``'s PUBLIC ``cxx_std_20`` requirement). -* 303 unit tests passing. +* 304 unit tests passing. 0.10.0 (2026-09-20) ------------------- diff --git a/CLAUDE.md b/CLAUDE.md index 79d6c4b..94b8f0d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -205,7 +205,7 @@ Then, for each message in the (compressed) payload: ## Testing -### Test Count: 303 unit tests +### Test Count: 304 unit tests ### Commands ```bash @@ -291,5 +291,5 @@ identity/capabilities object, and TLS setup). **Last Updated**: 2026-07-06 **Project Phase**: Unified multi-backend architecture -**Test Status**: 303 unit tests passing (all sanitizers clean) +**Test Status**: 304 unit tests passing (all sanitizers clean) **Executables**: `pj_bridge_ros2` (ROS2), `pj_bridge_rti` (RTI DDS, disabled), `pj_bridge_fastdds` (FastDDS) diff --git a/app/include/pj_bridge/transform_set.hpp b/app/include/pj_bridge/transform_set.hpp index 9bbdcf4..0a92d16 100644 --- a/app/include/pj_bridge/transform_set.hpp +++ b/app/include/pj_bridge/transform_set.hpp @@ -26,10 +26,9 @@ #include #include #include -#include #include #include -#include +#include #include #include "pj_bridge/message_transform.hpp" @@ -87,13 +86,12 @@ class TransformSet { TransformSet() = default; tl::expected add_rule(Rule rule); - bool log_once(const std::string& topic, const std::string& transform); FactoryMap factories_; std::vector rules_; mutable std::mutex mutex_; std::unordered_map> bindings_; - std::set> logged_; // (topic, transform) already reported + std::unordered_set setup_failed_; // topics whose setup error was already reported }; } // namespace pj_bridge diff --git a/app/src/transform_set.cpp b/app/src/transform_set.cpp index b722bc9..e803a8d 100644 --- a/app/src/transform_set.cpp +++ b/app/src/transform_set.cpp @@ -76,7 +76,7 @@ tl::expected, std::string> TransformSet::create( size_t index = 0; for (const auto& entry : profile["transforms"]) { - const std::string where = "transform profile rule " + std::to_string(index++) + ": "; + const std::string where = "rule " + std::to_string(index++) + ": "; if (!entry.is_object()) { return tl::make_unexpected(where + "must be an object"); } @@ -96,7 +96,7 @@ tl::expected, std::string> TransformSet::create( } else if (key == "params" && it->is_object()) { rule.params = *it; } else { - return tl::make_unexpected(where + "unknown or mistyped key '" + key + "'"); + return tl::make_unexpected(where + "key '" + key + "' is unknown or has the wrong JSON type"); } } if (auto added = set->add_rule(std::move(rule)); !added) { @@ -110,7 +110,7 @@ tl::expected TransformSet::add_rule(Rule rule) { // Mandatory, so that a transform applied to a type it cannot handle is a // startup error: topic types are only known once topics are discovered. if (rule.match_type.empty()) { - return tl::make_unexpected(std::string("needs 'match_type' ('match_topic' can only narrow it)")); + return tl::make_unexpected(std::string("needs a non-empty 'match_type' ('match_topic' can only narrow it)")); } if (rule.transform.empty()) { return tl::make_unexpected(std::string("missing 'transform'")); @@ -123,7 +123,9 @@ tl::expected TransformSet::add_rule(Rule rule) { return tl::make_unexpected("transform '" + rule.transform + "': " + ok.error()); } if (!factory->second.accepts(rule.match_type)) { - return tl::make_unexpected("transform '" + rule.transform + "' does not accept type '" + rule.match_type + "'"); + return tl::make_unexpected( + "transform '" + rule.transform + "' does not accept type '" + rule.match_type + + "' (use the full type name, e.g. 'sensor_msgs/msg/PointCloud2')"); } std::lock_guard lock(mutex_); rules_.push_back(std::move(rule)); @@ -138,19 +140,14 @@ tl::expected TransformSet::append_type_rule( return add_rule(std::move(rule)); } -// bind() runs for every topic on every topic poll: each (topic, transform) problem is reported once. -bool TransformSet::log_once(const std::string& topic, const std::string& transform) { - return logged_.emplace(topic, transform).second; -} - std::shared_ptr TransformSet::bind(const std::string& topic, const std::string& source_type) { std::lock_guard lock(mutex_); if (auto it = bindings_.find(topic); it != bindings_.end()) { if (it->second->source_type == source_type) { return it->second; } - bindings_.erase(it); // the topic's type changed: match again, and let it be reported again - std::erase_if(logged_, [&](const auto& entry) { return entry.first == topic; }); + bindings_.erase(it); // the topic's type changed: match again, and report a new failure again + setup_failed_.erase(topic); } // ponytail: unmatched topics re-run every rule on each call (once per topic poll); @@ -172,7 +169,7 @@ std::shared_ptr TransformSet::bind(const std::string& topic, con error = e.what(); } if (!bound->transform) { - if (log_once(topic, rule.transform)) { + if (setup_failed_.insert(topic).second) { // bind() runs on every topic poll: report once spdlog::error( "Transform '{}' could not be set up for '{}': {}; leaving it untransformed", rule.transform, topic, error); } diff --git a/docs/superpowers/plans/2026-09-20-message-transforms.md b/docs/superpowers/plans/2026-09-20-message-transforms.md index 375b753..2902053 100644 --- a/docs/superpowers/plans/2026-09-20-message-transforms.md +++ b/docs/superpowers/plans/2026-09-20-message-transforms.md @@ -1,5 +1,7 @@ # Message Transforms (v1: Cloudini) Implementation Plan +> **Historical.** This is the plan as written before implementation. The code diverged during review (output type/schema moved onto `MessageTransform`, `match_type` became mandatory, Cloudini 1.3.1 removed the CMake workarounds). The design doc in `docs/superpowers/specs/` and `docs/API.md` describe what was built. + > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. **Goal:** Operator-configured, per-topic payload transforms in pj_bridge, with Cloudini point cloud compression and the existing stripper as the two implementations. diff --git a/ros2/src/main.cpp b/ros2/src/main.cpp index 18cae76..0f04d0d 100644 --- a/ros2/src/main.cpp +++ b/ros2/src/main.cpp @@ -69,7 +69,7 @@ std::shared_ptr load_transforms(const std::string& prof auto transforms = pj_bridge::TransformSet::create(profile, std::move(factories)); if (!transforms) { - throw std::runtime_error(transforms.error()); + throw std::runtime_error("transform_profile '" + profile_path + "': " + transforms.error()); } if (strip_large_messages) { // after the profile's rules, so an explicit rule wins if (auto added = pj_bridge::append_strip_rules(**transforms); !added) { diff --git a/tests/unit/test_transform_set.cpp b/tests/unit/test_transform_set.cpp index 00cecf4..1d259a5 100644 --- a/tests/unit/test_transform_set.cpp +++ b/tests/unit/test_transform_set.cpp @@ -2,7 +2,6 @@ #include #include -#include #include #include #include @@ -152,7 +151,13 @@ TEST(TransformSetTest, RunIsSafeConcurrentlyWithBindFindAndStats) { // set up must be reported once, not once per second. TEST(TransformSetTest, SetupErrorIsLoggedOncePerTopic) { std::ostringstream captured; - auto previous = spdlog::default_logger(); + // Restored on every exit path: the sink points at `captured`, which dies with this test. + struct RestoreLogger { + std::shared_ptr previous = spdlog::default_logger(); + ~RestoreLogger() { + spdlog::set_default_logger(previous); + } + } restore_logger; spdlog::set_default_logger( std::make_shared("capture", std::make_shared(captured))); @@ -166,10 +171,14 @@ TEST(TransformSetTest, SetupErrorIsLoggedOncePerTopic) { EXPECT_EQ((*set)->bind("/a", "pkg/msg/In"), nullptr); (*set)->bind("/a", "pkg/msg/In"); // second poll (*set)->bind("/b", "pkg/msg/In"); // another topic is reported separately - spdlog::set_default_logger(previous); const std::string log = captured.str(); - EXPECT_EQ(std::count(log.begin(), log.end(), '\n'), 2) << log; + size_t reports = 0; + for (size_t at = log.find("could not be set up"); at != std::string::npos; + at = log.find("could not be set up", at + 1)) { + ++reports; + } + EXPECT_EQ(reports, 2u) << log; EXPECT_NE(log.find("'/a'"), std::string::npos) << log; EXPECT_NE(log.find("'/b'"), std::string::npos) << log; } @@ -211,30 +220,47 @@ TEST(TransformSetTest, RunCountsSamplesBytesAndDrops) { struct BadProfile { const char* name; json profile; + const char* expected_in_error; // the operator must be told WHY, not just "no" }; class TransformSetBadProfileTest : public ::testing::TestWithParam {}; -TEST_P(TransformSetBadProfileTest, IsRejectedAtStartup) { +TEST_P(TransformSetBadProfileTest, IsRejectedAtStartupWithAClearReason) { auto set = TransformSet::create(GetParam().profile, factories()); - EXPECT_FALSE(set.has_value()); + ASSERT_FALSE(set.has_value()); + EXPECT_NE(set.error().find(GetParam().expected_in_error), std::string::npos) << set.error(); } INSTANTIATE_TEST_SUITE_P( All, TransformSetBadProfileTest, ::testing::Values( - BadProfile{"not_an_object", json::array()}, BadProfile{"missing_transforms", json::object()}, - BadProfile{"unknown_top_level_key", json{{"transforms", json::array()}, {"extra", 1}}}, - BadProfile{"transforms_not_an_array", json{{"transforms", 5}}}, - BadProfile{"rule_not_an_object", json{{"transforms", json::array({5})}}}, - BadProfile{"mistyped_rule_key", rule({{"match_type", 5}, {"transform", "fake"}})}, - BadProfile{"topic_only_rule", rule({{"match_topic", "/lidar/.*"}, {"transform", "fake"}})}, - BadProfile{"rule_without_matcher", rule({{"transform", "fake"}})}, - BadProfile{"rule_without_transform", rule({{"match_type", "pkg/msg/In"}})}, - BadProfile{"unknown_rule_key", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"oops", 1}})}, - BadProfile{"unknown_transform", rule({{"match_type", "pkg/msg/In"}, {"transform", "nope"}})}, - BadProfile{"invalid_regex", rule({{"match_type", "pkg/msg/In"}, {"match_topic", "("}, {"transform", "fake"}})}, + BadProfile{"not_an_object", json::array(), "'transforms' array"}, + BadProfile{"missing_transforms", json::object(), "'transforms' array"}, + BadProfile{"unknown_top_level_key", json{{"transforms", json::array()}, {"extra", 1}}, "unknown key 'extra'"}, + BadProfile{"transforms_not_an_array", json{{"transforms", 5}}, "'transforms' array"}, + BadProfile{"rule_not_an_object", json{{"transforms", json::array({5})}}, "rule 0: must be an object"}, + BadProfile{ + "mistyped_rule_key", rule({{"match_type", 5}, {"transform", "fake"}}), + "key 'match_type' is unknown or has the wrong JSON type"}, + BadProfile{"empty_match_type", rule({{"match_type", ""}, {"transform", "fake"}}), "non-empty 'match_type'"}, + BadProfile{ + "topic_only_rule", rule({{"match_topic", "/lidar/.*"}, {"transform", "fake"}}), "non-empty 'match_type'"}, + BadProfile{"rule_without_matcher", rule({{"transform", "fake"}}), "non-empty 'match_type'"}, + BadProfile{"rule_without_transform", rule({{"match_type", "pkg/msg/In"}}), "missing 'transform'"}, + BadProfile{ + "unknown_rule_key", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"oops", 1}}), + "key 'oops' is unknown"}, + BadProfile{ + "unknown_transform", rule({{"match_type", "pkg/msg/In"}, {"transform", "nope"}}), + "unknown transform 'nope'"}, + BadProfile{ + "invalid_regex", rule({{"match_type", "pkg/msg/In"}, {"match_topic", "("}, {"transform", "fake"}}), + "invalid match_topic regex"}, + BadProfile{ + "unknown_param", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"params", {{"q", 1}}}}), + "unknown param 'q'"}, + // the short / ROS1 spelling is the likely real-world mistake: the message must show the expected form BadProfile{ - "unknown_param", rule({{"match_type", "pkg/msg/In"}, {"transform", "fake"}, {"params", {{"q", 1}}}})}, - BadProfile{"type_not_accepted", rule({{"match_type", "pkg/msg/Unrelated"}, {"transform", "fake"}})}), + "type_not_accepted", rule({{"match_type", "pkg/In"}, {"transform", "fake"}}), + "does not accept type 'pkg/In' (use the full type name, e.g. 'sensor_msgs/msg/PointCloud2')"}), [](const auto& info) { return std::string(info.param.name); });