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

Warn or error on unknown ROS arguments #167

Open
nnmm opened this issue May 16, 2022 · 2 comments
Open

Warn or error on unknown ROS arguments #167

nnmm opened this issue May 16, 2022 · 2 comments

Comments

@nnmm
Copy link
Contributor

nnmm commented May 16, 2022

Currently, there is no warning for unknown ROS arguments, for instance:

cargo run --bin minimal_subscriber -- --ros-args --params-file=lol.yaml

This does not load the parameter file lol.yaml, since ROS 2 doesn't support the = syntax here (it should be a space instead).

I think this can be done using rcl_arguments_get_unparsed_ros().

@Nizerlak
Copy link
Contributor

Nizerlak commented Jul 3, 2022

Depending on which approach would be chosen (warn or error) there should be logging (#184) implemented before or not. In case of warn prompting, logging mechanism is necessary, since error won't be returned from Context::new() and log call should happen there. Error raising variant would be simpler, since the only thing to do would be to add new enum to RclrsError (with Display implementation) and handle unparsed arguments similarly to non-ROS ones as in #147.

I think that warn is more appropriate and logging mechanism should be implemented anyway, so in this case this issue will be blocked by #184. To make work more parallel maybe introduce bare logging interface without implementation, so call to it could be performed and this issue will be independent of #184 progress.

@nnmm
Copy link
Contributor Author

nnmm commented Jul 4, 2022

Yes, I agree logging is probably the way to go. Theoretically we could also make Context::new() return something like

struct ContextWithExtraInfo {
    context: Context,
    unparsed_ros_args: Vec<String>
}

but that's cumbersome to use.

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

No branches or pull requests

2 participants