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

Reconsider automatically_declare_parameters_from_overrides for all controllers #531

Closed
destogl opened this issue Sep 9, 2021 · 7 comments

Comments

@destogl
Copy link
Member

destogl commented Sep 9, 2021

Long story-short: "I have a follow-up issue with this parameter"

Originally posted by @destogl in #504 (comment)

Long story:

I understand that we need this automatic declaration for JTC per-joint trajectory limits. And adding this parameter caused further fix in form of the auto_declare method because otherwise, the controllers are declaring parameters twice.

Now I have an issue when using external libraries which declare their parameters, like ros/filters library, especially filters chain structure. To get this working we would need to add an auto_declare throughout rclcpp. this leads me to the conclusion that the approach done in #504 is probably not the right one and we should reconsider it again.

@bmagyar
Copy link
Member

bmagyar commented Sep 13, 2021

Yeah but what do we do then? Can't filters work on a different "nodehandle/namespace"?

@destogl
Copy link
Member Author

destogl commented Sep 13, 2021

No, because they are not a node on their own. And it shouldn't be. I think that we don't have this issue if we properly declare parameters in the JTC. This was the source of this issue.

TBH the current solution is a bit dirty and hacky.

@bmagyar
Copy link
Member

bmagyar commented Sep 15, 2021

It seems we are alternating between two bad solutions. Is there a third one that's clean??

@destogl
Copy link
Member Author

destogl commented Sep 16, 2021

I don't think so. I am actually sure that the current approach is not correct for our library. This is OK if you have some simpler node without possiblity to extend it with many libraries. The old approach is right and we should simply declare those parameter explicity. As done here (ros-controls/ros2_controllers#225)

@livanov93
Copy link
Contributor

I agree the current solution is not the cleanest, however in my understanding the solution that existed before was aiming towards current behavior (avoiding the need to explicitly declare every parameter for the controller's use). However, as @destogl says, incorporation of other packages that don't use 'auto_declare' could really be a problem here.
We can then remove automatically_declare_parameters_from_overrides flag in the controller interface here and do all declaration explicitly.
In the future we have to read rclcpp docs more carefully regarding parameter propagation here and here.

However auto_declare could be useful as node's auxiliary method which is more of a topic for ros2/rclcpp repository.

@destogl
Copy link
Member Author

destogl commented Sep 24, 2021

@livanov93 if you have some capacity, this would be great. I think that only JointTrajectoryController has these issues.

@livanov93
Copy link
Contributor

@destogl I will address it soon and let hardcode the parameter declaration for jtc.

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

No branches or pull requests

3 participants