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

fix registering issue when node is not immediately spinned #42

Closed

Conversation

damien-robotsix
Copy link
Contributor

Hello,

Here is a suggestion for the classes in NodeWithMode. They immediately trigger the registering but if you don't spin the node fast enough (like I do since I need extra configuration after the creation of the instance)

  • then you quickly get flagged unresponsive by the Autopilot
  • then disconnection happen because the Autopilot is not sending you check request anymore when flagged unresponsive (is it wanted? or a bug?)

In all cases, it could be good to either:

  1. let the hand to the user to trigger the registration just before spinning the node
  2. have a more complex mechanism to check that the node is spinning before doing the registration.

The modification if put in this PR is for solution 1). However, this changes usage as the user must do the registration "manually" before spinning the node.
So I am open to suggestions on this.

Also, I did a minor modification to access the Executor because you might need that if you want to access specific functions you implemented in the class inherited from ExecutorBase.

Damien

@bkueng
Copy link
Collaborator

bkueng commented Jun 28, 2024

I think in your case I would rather not use NodeWithMode, and instead manually create the ros node, which gives you more flexibility.
NodeWithMode is meant for simplicity, and adding an extra registration call would not allow to use it like this anymore:
rclcpp::spin(std::make_shared<px4_ros2::NodeWithMode<MyExecutor, MyMode>>("my_node"));

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