Added Lifecycle Subscription wrapper#5772
Conversation
Signed-off-by: Prathmesh Atkale <atkaleprathmesh@gmail.com>
|
@Prathmesh2931, this does not build, did you test this? https://app.circleci.com/pipelines/github/ros-navigation/navigation2/17109/workflows/dd740d4c-6d31-44af-90d1-a5133d5950e9/jobs/50347 |
|
@mini-1235 |
|
Take a look at https://github.com/ros-navigation/nav2_docker/pkgs/container/nav2_docker, you can directly pull the image: |
|
Please fill in the PR template properly :-) |
| const bool allow_parameter_qos_overrides = true, | ||
| const rclcpp::CallbackGroup::SharedPtr callback_group_ptr = nullptr, | ||
| rclcpp::QOSMessageLostCallbackType qos_message_lost_callback = nullptr, | ||
| rclcpp::SubscriptionMatchedCallbackType subscription_matched_callback = nullptr, | ||
| rclcpp::IncompatibleTypeCallbackType incompatible_qos_type_callback = nullptr, | ||
| rclcpp::QOSRequestedIncompatibleQoSCallbackType requested_incompatible_qos_callback = nullptr, | ||
| rclcpp::QOSDeadlineRequestedCallbackType qos_deadline_requested_callback = nullptr, | ||
| rclcpp::QOSLivelinessChangedCallbackType qos_liveliness_changed_callback = nullptr) |
There was a problem hiding this comment.
We need all this, do not change
| */ | ||
| template<typename MessageT> | ||
| using Subscription = rclcpp::Subscription<MessageT>; | ||
| using Subscription = LifecycleSubscription<MessageT>; |
There was a problem hiding this comment.
This file should contain the new subscription, not lifecycle_subscription.hpp. This is now Nav2 subscription
| double bond_heartbeat_period{0.1}; | ||
| rclcpp::TimerBase::SharedPtr autostart_timer_; | ||
|
|
||
| // ADDED: Vector to store all managed entities (publishers, subscriptions, etc.) |
There was a problem hiding this comment.
This line and ones like it are making me think this is the output of an LLM. How did you validate, test, review, and compile this? We do not typically review AI outputs without you taking responsibility for quality control, we cannot review direct AI outputs - it is not a good use of maintainer time and resources. If you want to contribute a feature or fix, please put in your own legwork / effort to at least do QA and testing.
There was a problem hiding this comment.
Thanks for your feedback @SteveMacenski ,
The issue was on my side -I hadn’t fully verified the build environment and Docker setup before submitting the changes. That’s my mistake.
I will make sure to do Proper checks , validation and QC from my end .
I really appreciate your guidance and it means lot to me .
Sorry for inconvenience .
There was a problem hiding this comment.
Its ok - just for the future its important to do your own QC before involving other developers :-)
|
Any update? |
|
Yes , I am doing some lifecycle subscription testing |
Add nav2::Subscription class that wraps rclcpp subscriptions with lifecycle state management, blocking message processing when inactive. Signed-off-by: Prathmesh Atkale <atkaleprathmesh@gmail.com>
|
I am running into a build issue here - the existing test files are trying to use the old subscription type but I changed it to a lifecycle wrapper class. |
|
And also the ordering of parameter is different of rclcpp::Node::create_subscription and nav2::LifecycleNode::create_subscription . |
Signed-off-by: Prathmesh Atkale <atkaleprathmesh@gmail.com>
Signed-off-by: Prathmesh Atkale <atkaleprathmesh@gmail.com>
Signed-off-by: Prathmesh Atkale <atkaleprathmesh@gmail.com>
9ca0dbc to
ef44b00
Compare
Add subscription helpers that are aware of LifecycleNode state. Subscriptions created from a LifecycleNode will ignore incoming messages when the node is inactive, while regular Node subscriptions continue to behave as before. Update affected tests to use SharedPtr callback signatures to match the updated interfaces. This change preserves backwards compatibility with existing Nav2 code Signed-off-by: Prathmesh Atkale <atkaleprathmesh@gmail.com>
|
This pull request is in conflict. Could you fix it @Prathmesh2931? |
Signed-off-by: Prathmesh2931 <145354550+Prathmesh2931@users.noreply.github.com>
- Adjust indentation in test_service_server.cpp - Fix alignment in geometry_utils.hpp" Signed-off-by: Prathmesh Atkale <atkaleprathmesh@gmail.com>
|
Hi @mini-1235 , |
There was a problem hiding this comment.
File Purpose: Updated callback signatures in TwistSubscriber test to use SharedPtr for lifecycle subscription compatibility.
Change Summary:
- Changed callback parameters from raw messages to SharedPtr
- Updated assignment logic to dereference SharedPtr (
*msg)
There was a problem hiding this comment.
File Purpose: Updated TwistPublisher test callback signatures to use SharedPtr for lifecycle subscription compatibility.
Change Summary:
- Updated
create_subscriptioncallbacks from raw message to SharedPtr pattern - Modified assignment to dereference SharedPtr (
*msg)
There was a problem hiding this comment.
File Purpose: Updated PathConverter test callback signature to use SharedPtr for lifecycle subscription compatibility.
Change Summary:
- Updated Path subscription callback from raw message to SharedPtr pattern
- Modified assignment to dereference SharedPtr (
*msg)
There was a problem hiding this comment.
File Purpose: Updated route operations test callback signature to use SharedPtr for lifecycle subscription compatibility.
Change Summary:
- Updated SpeedLimit subscription callback from raw message to SharedPtr pattern
- Modified assignment to dereference SharedPtr (
*msg) - Preserved both the flag setting and message capture logic
There was a problem hiding this comment.
File Purpose: Added test executable for lifecycle subscription feature validation.
Change Summary:
- Added test_lifecycle_sub executable target
- Configure dependencies for lifecycle subscription testing
- Added installation directive for the test binary
There was a problem hiding this comment.
File Purpose: New test executable to validate lifecycle-aware subscription .
What Tests are cover:
Creates a test for lifecycle subscriptions which shows:
- Messages are not received while the node is unconfigured or inactive
- Messages are processed only after activation
- Subscriptions correctly stop receiving messages once the node is deactivated
Test exercise on on_configure , on_activate and on_deactivate callbacks to confirm behavior
There was a problem hiding this comment.
File Purpose: Updated action server test to use LifecycleNode and SharedPtr callbacks for lifecycle subscription compatibility.
Change Summary:
- Class Inheritance: Changed from
rclcpp::Nodetonav2::LifecycleNode - Callback Signatures: Updated from
UniquePtrtoSharedPtrpattern - Constructor Update: Updated base class initialization
- QoS Syntax: Fixed subscription parameter order
There was a problem hiding this comment.
File Purpose: Nav2 nodes rely on managed entities to ensure subscriptions follow lifecycle
transitions like activate and deactivate correctly.
There was a problem hiding this comment.
File Purpose: Extended LifecycleNode with managed entity interface support for lifecycle management.
Additions:
- Managed Entity System: Track publishers, subscriptions, services, actions as lifecycle-managed entities
- Coordinated Activation and Deactivation:
activateInterfaces()- activate all entities anddeactivateInterfaces()- deactivate all entities in order. - Entity Registration:
add_managed_entity()- register entities for lifecycle management
There was a problem hiding this comment.
File Purpose: -Implements lifecycle-aware create_subscription and Provides overloads for rclcpp::Node and rclcpp_lifecycle::LifecycleNode.
Changes:
Replaced single complex create_subscription function with two overloads that automatically select appropriate behavior based on node type.
There was a problem hiding this comment.
File Purpose: Updated trajectory visualization test callback signatures to use SharedPtr for lifecycle subscription compatibility.
Summary:
- Updated multiple test callbacks from raw message to SharedPtr pattern
- Modified assignments to dereference SharedPtr (
*msg) - Consistent pattern across all visualization test cases
|
@Prathmesh2931 I think #5834 should supersede. Thanks for your work here and collaborating with @Lotusymt -- I think that has yielded a really nice solution 😄 Any interest in working on the services / actions? Closing since I think we're moving forward with Lotusymt's design |
|
Thanks Steve, appreciate the feedback . |
Added new LifecycleSubscription wrapper class.
Integrated lifecycle-state checking before executing user callbacks.
Added new create_subscription() helper that automatically:
Applies Nav2 QoS & SubscriptionOptions
Wraps user callbacks in lifecycle checks
Registers the subscription inside the lifecycle wrapper
Updated:
to support the new lifecycle subscription interface.
Added the new header:
nav2_ros_common/lifecycle_subscription.hpp