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

Port param plugin #1567

Closed
wants to merge 10 commits into from
Closed

Port param plugin #1567

wants to merge 10 commits into from

Conversation

rob-clarke
Copy link
Contributor

Initial work to port param plugin to ROS2
Compiles but not yet tested.

Progress for #1559

@vooon
Copy link
Member

vooon commented Apr 18, 2021

Oh, sorry, i done that port few hours ago. Not yet sure that it's working as i want.

I will review your port later. Maybe i could get some better ideas...
But i certainly wouldn't have time to deal with mission plugins till next weekend.

@vooon
Copy link
Member

vooon commented Apr 18, 2021

Also please note that i'm thinking to use futures instead of cond vars where i can.

@vooon vooon added the ros2 label Apr 19, 2021
msg.value.real = to_real();
msg.param_id = get_name();
msg.value.integer = as_int();
msg.value.real = as_double();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At that point you'll get exception: one of as_*() would throw it.

param_value = float_tmp;
break;
// [[[end]]] (checksum: 5950e4ee032d4aa198b953f56909e129)
return Parameter(param_name, uv.param_float, pmsg.param_count, pmsg.param_index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That part of conversions should work - ints are convertible to PARAMETER_INTEGER.

param_value = float_tmp;
break;
// [[[end]]] (checksum: c30ee34dd84213471690612ab49f1f73)
return Parameter(param_name, pmsg.param_value, pmsg.param_count, pmsg.param_index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But here you likely get wrong type: pmsg.param_value is float, but it carries bools and integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my plan was to wrap it in an appropriate type(pmsg.param_value) style cast to force the template. Evidently I never got around to it.

set_srv = param_nh.advertiseService("set", &ParamPlugin::set_cb, this);
get_srv = param_nh.advertiseService("get", &ParamPlugin::get_cb, this);
auto parameters_qos = rclcpp::ParametersQoS();
param_value_pub = node->create_publisher<mavros_msgs::msg::Param>("param_value", parameters_qos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should use that qos.

@rob-clarke
Copy link
Contributor Author

Added some fixes for "completeness". As you've already got the port done I'll close this

@rob-clarke rob-clarke closed this Apr 19, 2021
vooon added a commit that referenced this pull request Apr 19, 2021
@vooon vooon added this to the Version 2.0 milestone May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants