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

declare_parameters allows conflicting type definition #879

Closed
NikolasE opened this issue Jan 14, 2022 · 6 comments
Closed

declare_parameters allows conflicting type definition #879

NikolasE opened this issue Jan 14, 2022 · 6 comments

Comments

@NikolasE
Copy link

NikolasE commented Jan 14, 2022

Bug report

  • Installation type:
    • Galactic Binaries
  • Client library (if applicable):
    rclpy

Steps to reproduce issue

self.declare_parameters(namespace="", parameters=[("param_name", "HelloWorld", ParameterDescriptor(type=ParameterType.PARAMETER_BOOL, description="a parameter"))])

Expected behavior

Exception or Warning that the given type in the ParameterDescriptor does not fit to the type of the value.

Actual behavior

ParameterDescription.type is ignored, parameter-type is deduced from value (-> string-parameter)

Feature request

Feature description

Raise an InvalidParameterValueException

Implementation considerations

Could break existing code but fixing the value-type mismatch is easy

@NikolasE NikolasE reopened this Jan 14, 2022
@NikolasE
Copy link
Author

NikolasE commented Jan 14, 2022

I'm working on a PR and I think I even found a bug in the current test cases:

rclpy/test/test_node.py:
def test_describe_parameters(self): (shortened version)

        self.node.declare_parameter(
            'bar',
            10,
            ParameterDescriptor(
                name='bar',
                type=ParameterType.PARAMETER_DOUBLE,
                additional_constraints='some more constraints',
                read_only=True,
                floating_point_range=[FloatingPointRange(from_value=-3.0, to_value=3.0, step=0.3)],
                integer_range=[IntegerRange(from_value=-20, to_value=20, step=3)]
            )
        )

        # The descriptor gets the type of the parameter.
        bar_descriptor = descriptor_list[1]
        self.assertEqual(bar_descriptor.name, 'bar')
        self.assertEqual(bar_descriptor.type, ParameterType.PARAMETER_INTEGER)

The author tried to create declared a PARAMETER_DOUBLE, but value is '10' instead of '10.', so the type is later PARAMETER_INTEGER.
Missing the trailing dot happens quite fast, and if the user actively declares the type, I think this choice should not be ignored. In this specific case, we even escaped the floating_point_range-check.

@NikolasE
Copy link
Author

This also stops users from using empty lists as default values:
#912 (comment)

An empty list is interpreted as byte-array even if the user explicitly declared a string_array

@ivanpauno ivanpauno added question Further information is requested and removed bug Something isn't working in review Waiting for review (Kanban column) more-information-needed Further information is required labels May 13, 2022
@ivanpauno
Copy link
Member

See my comments here.
IMO, this is known behavior, we only need to improve the docs.

I don't think we can reasonably fix it without breaking a lot of currently valid code.

@ivanpauno
Copy link
Member

An empty list is interpreted as byte-array even if the user explicitly declared a string_array

That can be solved using:

param = Parameter('my_param', type_=Parameter.Type.STRING_ARRAY, value=[])
node.declare_parameter(param.name, param.get_parameter_value())

@ivanpauno ivanpauno added documentation backlog and removed question Further information is requested labels Jun 14, 2022
@ivanpauno
Copy link
Member

@NikolasE @fujitatomoya I think we need to improve docs.
If you're planning to contribute that let me know and I will help reviewing it.
Thanks!

@ivanpauno
Copy link
Member

I think we need a more detailed tutorial in https://github.com/ros2/ros2_documentation/, but at least #957 clarified a bit how it works in the API docs.

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

Successfully merging a pull request may close this issue.

4 participants