PickNikRobotics / generate_parameter_library

Declarative ROS 2 Parameters
BSD 3-Clause "New" or "Revised" License
241 stars 45 forks source link

[Feature Request] Support for custom known structures in the parameters #104

Closed saikishor closed 1 year ago

saikishor commented 1 year ago

Hello, I have a question about adding known complex structures under parameters, when defined with generate_parameter_library.

Problem

I would like to define a structure of known parameters and extract a map of string and known structure out of it. The purpose of this is to enable a flexible setup of an arbitrary number of options that only makes sense when they are together. For instance, I want to have the joints and their types in the parameters, right now we can only have them as 2 separate vectors of elements and the problem here is they have to be the same size and then each corresponding element of these vectors has a relationship. If we can add support to such known structures, then it would be cool to have something like a map so all the elements such as joint_type correspond to the joint information.

Current structure limitation:

controller_1:
    joint_names: ["joint_1", "joint_2", "joint_3"]
    joint_type: ["prismatic", "rotary", "rotary"]
    joint_reduction_ratio: [100.0, 10.0, 20.0]

Target structure:

controller_1:
    joint_parameters:
        joint_1:
            type: prismatic
            reduction_ratio: 100.0

        joint_2:
            type: rotary
            reduction_ratio: 10.0

        joint_3:
            type: rotary
            reduction_ratio: 20.0

If the structure is this way, maybe we can have some generated API like parameters.joint_parameters["joint_1"].type and parameters.joint_parameters["joint_1"].reduction_ratio . I think this way it is better and clear as all the parameters are expressed without losing much of their significance. This would allow us to skip the usual checks of parameter sizes being the same or not. For instance, the same could also be useful for the PIDs of the controller, as they belong per joint and they only make sense together.

If this can be done, then in the future, It would be really cool to reuse the parameter structures from another package yaml information and generate the API.

What do you think about this kind of feature?

tylerjw commented 1 year ago

I think this is possible with the current map feature. I didn't document the map feature because I'm still undecided if it is nice for users. You can find an example of it here: https://github.com/PickNikRobotics/generate_parameter_library/blob/9350efd362f50cd25ec8cbade16770fa7419fb30/example/src/parameters.yaml#L34

The one area it differs from what you asked for is you need an array of joints which will populate the declared mapping parameter you can put a structure of parameters within.

saikishor commented 1 year ago

Hello @tylerjw,

Thank you for such an imminent reply. Thanks for pointing it out about the hidden map feature. However, I think there might be a limitation still right?. For instance, if I want to have a vector of structure within the parameters, It might not be possible right?.

A different question, do you think it is possible to have a structure defined in a package to be reused in another or a structure defined in one yaml to be reused in another?. For instance, PID may be it's wiser to have it declared at one place and reuse it somehow in your known structure. In this way, it's less maintenance. I think it's a huge change for the library.

you need an array of joints which will populate the declared mapping parameter you can put a structure of parameters within.

Do you mean that I need a separate joints list vector?, So that I can use them as a key to the map?. If not, could you please elaborate?

tylerjw commented 1 year ago

Here is a more complete example of the map feature being used: https://github.com/ros-controls/ros2_controllers/blob/master/joint_trajectory_controller/src/joint_trajectory_controller_parameters.yaml

Here is an example config that works with that yaml: https://github.com/ros-planning/moveit_resources/blob/0be98e1b263e823fa41b653359da582dc585c827/prbt_support/config/manipulator_controller.yaml#L22

As to your second idea of re-usable sets of parameters. I don't think that is a good idea. You are not the first one with that idea though.

My reasoning is that if you have a PID it will have code (to do the pid part) and not just a config. You wouldn't want many different copies of that code all over so you should probably make the PID a re-usable piece of code and include the parameters for it as part of the PID thing. Then you can create as many of them at different points in your code as you want and use the namespace feature to make them have different config namespaces. I also want configs to be as flat and delcarative as possible, this is part of why I don't like the map feature. I think one of the problems in ROS is that we too quickly reach for configs and create nodes with a huge combinitorial of possible configs we never test. Instead I think we should expose much less customization through configs and focus on creating better abstractions.

saikishor commented 1 year ago

Thank you so much @tylerjw for explaining me clearly the map feature. I clearly understand now. I think I can work with it.

Regarding the re-using of the parameters, I see your point. I agree with you that the config is definitely linked with a part of the code. Instead of over-complicating the library, we can simply use namespacing to get things done. Thanks again for the clear insight.

I will go ahead and close the issue. If I find something else, after our use-cases with this map feature. I will revert back.

Thank you for the clear explanation once again.

tylerjw commented 1 year ago

I'm always happy to reconsider my position and add more features. Part of my opinions come from working on MoveIt and feeling that the config of MoveIt and ros_control is unapproachable for many. Thank you for the questions.

saikishor commented 1 year ago

Yes, I understand. Some of those questions are from my previous experiences. However, I would like to give it a shot this way and see how much I can solve my use cases. If I encounter some interesting use cases, I will surely get back to get your opinion.

tylerjw commented 1 year ago

One thing that might interest you is I just merged a feature that will add bounds information to the parameter description. Internally people here at PickNik wanted that feature to make it easier to build a GUI with sliders for dynamic parameters that have a configured range.

saikishor commented 1 year ago

Awesome. Yes, I just saw it today before opening the ticket. It totally makes sense to have it. Looking forward to start using it.