Gimbal protocol v2 plugin - addressing PR comments

[Crowdedlight]

Addressing the PR comments from #1825 to help get the work done by Mark-Beaty merged in.

Copied in his PR description here:

This plugin has been tested with ROS2 Foxy and Humble and everything appears to be working correctly. Certain services and topics haven't been observed as working due to limitations of my testing hardware (Freefly Astro) and software (Auterion Simulator), but everything follows the documentation currently available for MAVLink's Gimbal Protocol v2. I also plan on adding another branch on my fork with the plugin separate from Mavros to enable use as a standalone plugin prior to the PR being approved/incorporated in a release.

[Crowdedlight]

Original comment by "vooon":
"I think you haven't used cog.py to re-generate that file?"

I am not sure how to run it so it effects this file? I can the ./mavros/tools/cogall.sh but it did not seem to regenerate this file. Could you point me in the right direction to figure out how to regenerate it?

[vooon]

https://github.com/mavlink/mavros/blob/ros2/cog-requirements.txt

Cogall depends on installed cog.py, probbly in today's world that be easier with pipx, but i haven't exact directions for that.

So in general, you must do like that (beware, not tested!):

python3 -m venv ~/ros/.venv
source ~/ros/.venv
pip3 install -r cog-requirements.txt
pip3 install pymavlink
ln mavros_cog.py ~/ros/.venv/lib/python3.11/site-packages/

Then

cog -cr mavros_extras/mavros_plugins.xml

[vooon]

Also note, cog would raise an error if generated part didn't match md5. But you simply can remove hash after [[[end]]].

[Crowdedlight]

Original comment by vooon:

Locking in the constructor might be a bad idea.
Maybe better to check for that service later?

As the rest is callbacks based on ros/mavlink message traffic, then you would have to add a check to every callback needing this client, instead of having it in the constructor once.
The locking here is also set with a timeout of 5s, so worst-case is locked for 5s before moving on.

If you have a suggestion to how to handle it better, let me know :)

[Crowdedlight]

edit: I miswrote. I can see it will block here in periods of 5s forever.
Potential change could be to make it to only try once with that timeout and then abandon the plugin and throw the error, if the service can't be made?

[vooon]

I think better to trow away client after the use, so you don't need to keep it between calls.

[Crowdedlight]

I can change to do that, I just thought the constant recreation of the client would be worse?
As if a caller is using the service to set ROI region, or pitchyaw with even just 1hz it is still initializing a client and destroying it per call.

I could create it in the constructor, but move the check if the service is available to each message using it? And change so it only tries for 5s then just returns error to the caller to let them know?

Would keep the constructor free from blocking, but still ensure the service exist before calling it, and otherwise return message to caller that the request could not be completed due to the mavros/cmd service not being found?

[vooon]

You can cache the client, but must create it on request, not in the constructor.

Something like that pseudo:

*Client get_cmd_long() {
  mutex Locker();

  if _cache {
    return _cache;
  }

  client = node_->create_client(...);
  // error handling!
  _cache = client;
  return client;  
}

[Crowdedlight]

ofc, that makes sense. I'll do that.

[Crowdedlight]

original comment by vooon:

Ideally that should be generated like for MAV_CMD.

I am not sure how to do that? I looked around briefly but didn't manage to find where it was generated for MAV_CMD in the msg files. Can you help point out where to find it, or the flow to make it generated like MAV_CMD?

[vooon]

You may find the generator code in the comment. Cog simply execute this embedded python scripts:

mavros/mavros_msgs/msg/CommandCode.msg

Lines 5 to 8 in 072df7e

	# [[[cog:
	# import mavros_cog
	# mavros_cog.idl_decl_enum_mav_cmd()
	# ]]]

[Crowdedlight]

@vooon I addressed most of the comments you left on #1825 , but I have a few questions to some of them. I copy-pasted them over here and added them as comments above with my questions to them too. :)

[Crowdedlight]

Thanks for fixing the last parts and getting it merged! Much Appricated :)

I had some priority work dumped on me at work and busy weekends so had to postpone it. Sorry about that!

[vooon]

@Crowdedlight thank you too. When you have time, please test it, as i didn't have all required things to setup an environment.

[Crowdedlight]

@Crowdedlight thank you too. When you have time, please test it, as i didn't have all required things to setup an environment.

I just tested part of the release in simulation. I found no errors. Gimbal moves as I expect sending the quaternion to it with this topic.

And Using the service to take control over the gimbal with /mavros/gimbal_control/manager/configure works as well. Due to other commitments I have not had time to go through all the services and topics, but at least these work which should give you full gimbal attitude control. :)