Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LifecycleSubscription #2715

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

Amronos
Copy link

@Amronos Amronos commented Dec 24, 2024

This PR uses most of the changes made in #2254, fixes some of the issues in that PR, and adds a test.

Signed-off-by: Aarav Gupta <[email protected]>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides the comments below, most of the implementation code is from #2254. i would add @carmiac as a co-author.

{
}

/// TODO: Hold onto the data that arrives before activation, and deliver that on activation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides this, i think subscription should not be discovered in the graph if that is in unconfigure state. if that comes to inactive state (means it is configured), subscription will be discovered in the ROS 2 network. and then hold onto the data in the rmw message queue not to take them out and dispose as described here.

for doing that, probably we need to have state control (configured, inactive) in SubscriberBase class and underlying implementations, and then on_configure callback, we can configure the subscription to be discovered. (on_activate, it can take the data out of the queue.) i am not sure how exactly we want to design this at this moment, but we would want to add some comments here too. (this feature is similar with LazySubscription concept.)

note that this requirement should also go to LifecyclePublisher as well.

what do you think?

}
}

void empty_callback(const test_msgs::msg::Empty msg) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think one of the most important test case is missing here.

  • inactive state callback should not be called. if called, asserts the test.
  • active state, callback should be called. if not called, asserts the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants