-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
AP_DDS: Parameters service #28298
base: master
Are you sure you want to change the base?
AP_DDS: Parameters service #28298
Conversation
I have tested with the Plane SITL, trying to ask for a wrong parameter. I mispelled
Instead, if you ask for the correct parameter I have:
Overall seems correct. I have tried to set and get multiple parameters, changed the airspeed in flight, made sure it was effective. |
The error gets reported as a GCS message. It should read I was thinking of populating the fields in the response message with bogus entries if that sounds like an acceptable error response? |
I think it's fair to just use the |
Overall, this is looking great. Do you know what additional services would be required to get this call to work?
Seems like maybe just a topic name, you are already using the right interface: Consider making the topic this: We could also just change the node name to If you did one of those, then we get this awesome ability to use the ROS 2 CLI like so! ryan@ryan-thinkpad:~$ ros2 node info /ap
/ap
Subscribers:
/ap/cmd_gps_pose: ardupilot_msgs/msg/GlobalPosition
/ap/cmd_vel: geometry_msgs/msg/TwistStamped
/ap/joy: sensor_msgs/msg/Joy
/ap/tf: tf2_msgs/msg/TFMessage
Publishers:
/ap/battery/battery0: sensor_msgs/msg/BatteryState
/ap/clock: rosgraph_msgs/msg/Clock
/ap/geopose/filtered: geographic_msgs/msg/GeoPoseStamped
/ap/gps_global_origin/filtered: geographic_msgs/msg/GeoPointStamped
/ap/imu/experimental/data: sensor_msgs/msg/Imu
/ap/navsat/navsat0: sensor_msgs/msg/NavSatFix
/ap/pose/filtered: geometry_msgs/msg/PoseStamped
/ap/tf_static: tf2_msgs/msg/TFMessage
/ap/time: builtin_interfaces/msg/Time
/ap/twist/filtered: geometry_msgs/msg/TwistStamped
Service Servers:
/ap/arm_motors: ardupilot_msgs/srv/ArmMotors
/ap/get_parameters: rcl_interfaces/srv/GetParameters
/ap/mode_switch: ardupilot_msgs/srv/ModeSwitch
/ap/set_parameters: rcl_interfaces/srv/SetParameters
Service Clients:
Action Servers:
Action Clients:
ryan@ryan-thinkpad:~$ ros2 param get ap LOIT_SPEED
Double value is: 1250.0 |
rcl_interfaces_srv_SetParameters_Response set_parameter_response; | ||
const bool deserialize_success = rcl_interfaces_srv_SetParameters_Request_deserialize_topic(ub, &set_parameter_request); | ||
if (deserialize_success == false) { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to add a GSC_SEND_TEXT warning here. I don't think people will be calling this in a loop.
strncpy(param_key, (char *)param.name, AP_MAX_NAME_SIZE); | ||
param_key[AP_MAX_NAME_SIZE] = 0; | ||
|
||
// Only worried about integer and float types for parameter setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be useful to comment which param types we don't handle.
break; | ||
} | ||
default: { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we silently discard other parameter types? What's the expected user behavior in this case?
// the user can set BRD_OPTIONS to enable set of internal | ||
// parameters, for developer testing or unusual use cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the user can set BRD_OPTIONS to enable set of internal | |
// parameters, for developer testing or unusual use cases | |
// The user can set BRD_OPTIONS to enable set of internal | |
// parameters, for developer testing or unusual use cases. |
Please end multi-line comments in periods which are easier to detect issues in merge conflicts.
vp->set_float(param_value, var_type); | ||
|
||
// Force the save if the value is not equal to the old one | ||
bool force_save = !is_equal(param_value, old_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you look at using set_and_save_by_name_ifchanged
?
|
||
// set parameter | ||
AP_Param *vp; | ||
char param_key[AP_MAX_NAME_SIZE+1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
char param_key[AP_MAX_NAME_SIZE+1]; | |
char param_key[AP_MAX_NAME_SIZE + 1]; |
Style nit - astyle
doesn't catch these.
enum ap_var_type var_type; | ||
|
||
AP_Param *vp; | ||
char param_key[AP_MAX_NAME_SIZE+1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
char param_key[AP_MAX_NAME_SIZE+1]; | |
char param_key[AP_MAX_NAME_SIZE + 1]; |
Style nit
continue; | ||
} | ||
|
||
float param_value = vp->cast_to_float(var_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
float param_value = vp->cast_to_float(var_type); | |
const float param_value = vp->cast_to_float(var_type); |
Use const for locals when possible
|
||
uxr_buffer_reply(uxr_session, reliable_out, replier_id, sample_id, reply_buffer, ucdr_buffer_length(&reply_ub)); | ||
|
||
GCS_SEND_TEXT(MAV_SEVERITY_INFO, "%s Get Parameters Request : %s", msg_prefix, successful_read ? "SUCCESSFUL" : "ONE OR MORE PARAM NOT FOUND"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCS_SEND_TEXT(MAV_SEVERITY_INFO, "%s Get Parameters Request : %s", msg_prefix, successful_read ? "SUCCESSFUL" : "ONE OR MORE PARAM NOT FOUND"); | |
GCS_SEND_TEXT(successful_read ? MAV_SEVERITY_INFO : MAV_SEVERITY_WARNING, "%s Get Parameters Request : %s", msg_prefix, successful_read ? "SUCCESSFUL" : "ONE OR MORE PARAM NOT FOUND"); |
I like this trick to make the log level different dependent on the result code.
* This makes params work in the CLI Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com>
1106239
to
4ce5403
Compare
Needs a rebase on |
ISSUE
#28292
Changes
-Add set parameters service call to AP_DDS library
Test
Using ros2 CLI, build and launch ardupilot sitl
Expected output
Expected output
mavproxy window:
service call window:
Expected output
mavproxy window:
service call window: