[ros2] Port get/set properties services

[nzlz]

First approach to the migration of get/set properties.
As I stated in #779 I am trying to migrate the properties related services to ros2.

This a World plugin. I tried to follow the same structure as ros2_state branch.
get_physics_properties and set_physics_properties are commented as @ironmig stated in #779:

Remove the get_physics_properties and set_physics_properties services, which duplicate the functionality of the physics dynamic reconfigure in the current plugin. Replace with parameters for physics properties

Also seems like get_world_properties needs some change too:

Remove get_world_properties. Replace with get_model_list (perhaps under factory?). The other attributes are sim_time (will come from clock) and rendering_enabled (currently unused)

Note: This is just a first version, the intend is not to merge but to discuss the structure of these functionalities.
Note2: I will be off a few days, so feel free to reuse/update this code in any way.

[chapulina]

Thank you for getting started on this! The general direction looks good to me, I'm happy to review this again once you make more progress.

Also, I think you've already seen this, but just in case, here's a guide that should help a bit. For example, you can remove all references to Gazebo <= 8.

[chapulina]

I agree with @ironmig that GetWorldProperties has a misleading name and if we want to keep the functionality we should rename it to GetModelList.

[chapulina]

Yes, I agree that it's good to avoid duplication, and porting just the dynamic reconfigure part should be enough. Note that on ROS 2 that would be implemented with basic parameters, I think you'll need to register a callback with ros_node_->register_param_change_callback.

[nzlz]

Review.

[nzlz]

Regarding the required tests, I guess the services that have a get/set are pretty straight forward to implement. What about the test for get_model_properties ?

Is it a good idea to spawn a custom xml file and expect the get_model_properties to return equal values? Since this does not seem a very good idea, how could this be tested in another way?

[nzlz]

Continuing the test discussion:
Get/Set Link properties and Get/Set Light properties are easy to test since they use the same parameters in the respective services. But in Get/Set Joint properties, service request/response differs so much. I think this needs some kind of rework, either in the request or the response part (I guess the Request).

GetJointProperties.srv [Response]

string joint_name                    # name of joint
---
# joint type
uint8 type
uint8 REVOLUTE    = 0                # single DOF
uint8 CONTINUOUS  = 1                # single DOF (revolute w/o joints)
uint8 PRISMATIC   = 2                # single DOF
uint8 FIXED       = 3                # 0 DOF
uint8 BALL        = 4                # 3 DOF
uint8 UNIVERSAL   = 5                # 2 DOF
# dynamics properties
float64[] damping
# joint state
float64[] position
float64[] rate
# service return status
bool success                         # return true if get successful
string status_message                # comments if available

SetJointProperties.srv [Request]

string joint_name                               # name of joint
gazebo_msgs/ODEJointProperties ode_joint_config # access to ODE joint dynamics properties
---
bool success                                    # return true if get successful
string status_message                           # comments if available

ODEJointProperties message

# access to low level joint properties, change these at your own risk
float64[] damping             # joint damping
float64[] hi_stop             # joint limit
float64[] lo_stop             # joint limit
float64[] erp                 # set joint erp
float64[] cfm                 # set joint cfm
float64[] stop_erp            # set joint erp for joint limit "contact" joint
float64[] stop_cfm            # set joint cfm for joint limit "contact" joint
float64[] fudge_factor        # joint fudge_factor applied at limits, see ODE manual for info.
float64[] fmax                # ode joint param fmax
float64[] vel                 # ode joint param vel

[nzlz]

Another thing, both set_entity_state and and set_link_properties offer the possibility to change the pose. Should the pose-changing option be available only in the state-related service? Or is it okay is it is duplicated (as it was in ros1).

Note: updated PR commit.

Update: Confusion between different pose elements. Leaving as it was in ros1 version.

[chapulina]

Is it a good idea to spawn a custom xml file and expect the get_model_properties to return equal values?

Probably the easiest way would be to have the model already in the world instead of going through the trouble of spawning it. For example, this test world has a simple model with a joint.

both set_entity_state and and set_link_properties offer the possibility to change the pose. Should the pose-changing option be available only in the state-related service? Or is it okay is it is duplicated (as it was in ros1).

I vote for removing duplication and having a clear separation of concerns across the plugins.

But in Get/Set Joint properties, service request/response differs so much. I think this needs some kind of rework, either in the request or the response part

Very good point. This touches a broader topic about refactoring physics properties so that those which are common across physics engines are in a common message and only the engine specific ones are in separate messages. This subject is definitely larger than the scope of this pull request though, ideally we will eventually have some consistency across gazebo_ros, SDF and ign-msgs.

So I'd propose one of these solutions for now:

1. Create new services only with the bare minimum functionality you need right now, and leave the rest of the properties for later.
2. Just keep the same message structure until this can be revisited.
3. Work on a broader proposal for how to organize joint properties across physics engines.

[nzlz]

Regarding Get/Set Joint properties, I will go with number 2, which is:

Just keep the same message structure until this can be revisited.

The reason is that another package I have to migrate is using the ODE-> damping. Will keep this for now . I will spend some time with the new organization of joint properties across physics engines once the ros2 branch gets stable (merges _state, _time_cmds and upcoming _properties). Not vital thing right now.

[nzlz]

Updated main commit right now. Current state. UPDATED:

• get_world_properties migrated to get_model_list inside factory. Seems like everything is working, including tests.
• get_joint_properties added. Working.
• set_joint_properties added. Working.
• get_link_properties added. Working.
• set_link_properties added. Working.
• get_light_properties added. Working.
• set_light_properties added. Working.

TODO:

• get/set:physics properties.

[nzlz]

Hi @chapulina,

Pushed ready-to-merge code, ready for a new review, possibly final?. CI errors are fine on my side.

---

Regarding dynamic physics reconfigure:

Yes, I agree that it's good to avoid duplication, and porting just the dynamic reconfigure part should be enough. Note that on ROS 2 that would be implemented with basic parameters, I think you'll need to register a callback with ros_node_->register_param_change_callback.

get/set:physics properties not migrated yet since its not urgent for our needs. I could spend some time working on this in the future if you could provide extra information regarding this topic, but I don't think I it will be possible in the short term.

[chapulina]

TODO(@nzlz ) : rebase against new crystal branch

PS: Sorry I haven't been able to make time to review the latest changes.

[nzlz]

Rebased against crystal.

[chapulina]

Sorry for the delay! I did another pass on the PR.

One thing that stood out to me is how the messages use outdated fields, like body and geom to refer to link and collision. It would be really nice to update those, but I understand if this is too much trouble.

I started fixing some linter errors on cb891db, but could you take care of the rest? Make sure there are no failures when you run colcon test --packages-select gazebo_ros.

Great demo and tests!

[chapulina]

Mind removing the status_message field? It's something that is present in gazebo_ros1 and we'd like to stop doing. I also added a note to the contribution guide. No need to remove it from the old messages you're using.

[chapulina]

Please document all fields, even if they're going to be ignored

[chapulina]

How about getting rid of this field and putting sim time in the header instead? Like this:

_res->header.stamp = Convert<builtin_interfaces::msg::Time>(world_->SimTime());

[chapulina]

Nitpick: keep includes in alphabetic order

[chapulina]

Alphabetize includes

[chapulina]

Consider adding a conversion function from gazebo::msgs::Color to std_msgs::ColorRGBA, not required though

[chapulina]

humm vel doesn't seem to work for me. Not an issue with this PR though, probably a problem on Gazebo.

[chapulina]

This is fun 😄

[chapulina]

Your demo shows that this works, so this todo can be removed

[chapulina]

Please use nullptr instead of NULL

[chapulina]

This has been merged into dashing on #972 . Closing this PR, reopen it a backport for crystal is necessary.