Stop sensor subscribe-check timer once subscribed (fix #23 rmw_zenoh ERROR spam)#24
Merged
Conversation
added 3 commits
May 27, 2026 19:53
SensorBase::subscribeCheck() was written for a ROS 1 one-shot timer, but the ROS 2 port uses rclcpp::create_wall_timer, which always repeats. Once a sensor subscribed, the 1 s check timer was never cancelled and kept firing, re-running subscribe() every tick and recreating the live subscription. Under rmw_zenoh the per-second teardown raced in-flight samples, producing "SubscriberCallback triggered over ..." ERROR spam. - subscribeCheck() now early-returns on the not-ready paths (the single repeating constructor timer keeps polling) and cancels the timer once subscribed, restoring one-shot semantics in one place for all three sensor types. - Add an already-subscribed guard to each subscribe() override as defense-in-depth so a live subscription is never torn down/recreated. - Add test_subscribe_once: drives a sensor with a live publisher and asserts subscribe() runs exactly once and the check timer is cancelled (verified to fail before this fix). Refs #23
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #23 by restoring subscribe-once behavior for sensor topic detection in the ROS 2 port, preventing repeated subscription teardown/recreation and the resulting rmw_zenoh_cpp error spam.
Changes:
- Cancels
SensorBase’s polling timer after a successful subscription while keeping retry polling for unknown/unsupported topics. - Adds already-subscribed guards to orientation, position, and velocity sensor subscription methods.
- Adds a regression test that verifies
subscribe()runs once and the check timer stops.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
mru_transform/include/mru_transform/sensor.hpp |
Stops the repeating subscribe-check timer after successful subscription. |
mru_transform/src/orientation_sensor.cpp |
Avoids recreating an existing orientation subscription. |
mru_transform/src/position_sensor.cpp |
Avoids recreating an existing position subscription. |
mru_transform/src/velocity_sensor.cpp |
Avoids recreating an existing velocity subscription. |
mru_transform/test/test_subscribe_once.cpp |
Adds regression coverage for subscribe-once timer behavior. |
mru_transform/CMakeLists.txt |
Registers and links the new regression test. |
.agent/work-plans/issue-23/progress.md |
Adds work-plan/progress documentation for issue #23. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SensorBase::subscribeCheck()was written for ROS 1's one-shotros::Timer, but the ROS 2 port usesrclcpp::create_wall_timer— which always repeats (create_timer.hpponly takesautostart; no one-shot option exists in rclcpp). The original comment// subscribed, so bail out before setting a new timerdescribes semantics the new timer no longer has, so once a sensor subscribed, the 1 s check timer kept firing andsubscribe()recreatedsubs_.imu/subs_.navsat_fix/ etc. every tick. Underrmw_zenoh_cppthe per-second teardown raced in-flight samples, producing the sporadic ERROR logs reported in #23:The defect was latent since the ROS 2 port; the previous DDS RMW didn't log the teardown race, so it only became visible after the late-Apr-2026 Zenoh migration.
Changes
SensorBase::subscribeCheck()(mru_transform/include/mru_transform/sensor.hpp): restructured into early-returns on the not-ready paths (the single repeating constructor timer keeps polling) andsubscribe_check_timer_->cancel()once subscribed. Cancelling a timer inside its own callback is the standard rclcpp one-shot idiom. One fix in the base covers all three sensor types.subscribe()guards inorientation_sensor.cpp,position_sensor.cpp,velocity_sensor.cpp: early-returntrueif any of that sensor'ssubs_members is already set. Defense-in-depth — with the timer cancel,subscribe()is normally never re-entered, but the guard prevents any future regression from tearing down a live subscription.test/test_subscribe_once.cpp: drives a realOrientationSensorwith a live publisher on the configured topic, spins past several timer periods, and assertssubscribe()runs exactly once and the check timer is cancelled. Verified to fail on the pre-fix code (subscribe call count climbs ~1 Hz) and pass after the fix.Behavior note (intentional)
This restores the original "subscribe once to the first supported type, then stop" semantics. A topic publisher that later switches to a different supported type (e.g.
Imu→QuaternionStamped) will not be re-evaluated. That matches the ROS 1 behavior and the existing design; if dynamic type-switching on a live topic is ever wanted, please open a separate issue.Test plan
colcon build(worktree): mru_transform builds cleanly.colcon test: 9 tests, 0 failures (including the newtest_subscribe_once).sensor.hpprevert) and pass with the fix restored.SubscriberCallback triggered over …ERROR spam stops in a fresh run ofmru_transform_nodeagainst/bizzy/mavros/imu/dataunder rmw_zenoh.Closes #23
Authored-By:
Claude Code AgentModel:
Claude Opus 4.7 (1M context)