PickNikRobotics / generate_parameter_library

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

Fix mapped paramteres generation for python #183

Closed Kotochleb closed 6 months ago

Kotochleb commented 7 months ago

This PR fixes incorrectly created parameters for maps in Python.

With this yaml:

my_node:
  my_params:
    params_to_be_mapped: {
      type: string_array,
    }
    __map_params_to_mapped:
      in_map_param: {
        type: bool,
      }

Current Jinja template will create following code:

# declare any new dynamic parameters
for value_1 in updated_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

# ...

for value_1 in updated_params.params_to_mapped:
  param_name = f"{self.prefix_}my_params.{value_1}.in_map_param"

# ...

# declare and set all dynamic parameters
for value_1 in updated_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

While the expected output is:

# declare any new dynamic parameters
for value_1 in updated_params.my_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

# ...
for value_1 in updated_params.my_params.params_to_mapped:
  param_name = f"{self.prefix_}my_params.{value_1}.in_map_param"

# ...

# declare and set all dynamic parameters
for value_1 in updated_params.my_params.params_to_mapped:
  updated_params.my_params.add_entry(value_1)

Namespace of my_params is not taken into account in for loops of the mapped values.

Kotochleb commented 6 months ago

@sea-bass I saw #185, but it did not solve this issue. I tested it once again after you comment and neither edfeb82 nor current main do not work. That is why I still want to keep this PR open.

As you suggested, I added check if {{struct_name}} is not empty, and I also merged main to make sure everything will still work after adding #185.

pac48 commented 6 months ago

Okay, I think I see the issue here. params_to_be_mapped is referred to but it is not at the top-level scope. Currently, the library assumes that because it may be ambiguous in any other case. For example,

my_node:
  params_to_be_mapped: {
     type: string_array,
   }  
  my_params:
     params_to_be_mapped: {
        type: string_array,
      }
      __map_params_to_mapped:
        in_map_param: {
          type: bool,
          }

this is valid YAML but it is unclear which params_to_be_mapped is referred to. So we need a new syntax to support this. I think it would make sense to use a syntax like this: __map_my_params.params_to_mapped.

The current change in this PR breaks the example file here: https://github.com/PickNikRobotics/generate_parameter_library/blob/main/example_python/generate_parameter_module_example/parameters.yaml

Kotochleb commented 6 months ago

Thank you for clarification. Since the changes I proposed are breaking, I will simply modify my own code then. Feel free to close this PR without merging.

pac48 commented 6 months ago

@Kotochleb Do you mind opening an issue for this? That way, this feature can potentially be added when I have some time.

Kotochleb commented 6 months ago

Since the issue is open now I am closing this PR