AP_DDS: Rally Get and Set

[tizianofiorenzani]

Issue

#28444

What Changed

• Service /ap/rally_set appends a Rally Point to the list or clears the list (using the clear flag).
  • point: geolocation
  • altitude_frame mandatory, follows enum of AltFrame.
• Replies with:
• success is the action is successful;
• size is the list's size.
• Service /ap/rally_get gets the Rally Point at a given index. Returns:
  • success
  • size is the list's size.
  • rally the rally point as ardupilot_msgs/Rally type.
• New ardupilot_msgs/Rally type is defined, that copies the structure definition in AP_Rally.

Test

Auto Test

colcon test --packages-select ardupilot_dds_tests --event-handlers=console_cohesion+ --pytest-args -k test_rally_point

Manual Test

• run the SITL
• Call the service to clear the rally list ros2 service call /ap/rally_set ardupilot_msgs/srv/RallySet "{clear: true}"
• Open up Mavproxy with map and make sure the rally list is clear.
• Append a new Rally Point: ros2 service call /ap/rally_set ardupilot_msgs/srv/RallySet "{rally: {point: {latitude: -35.36, longitude: 149.17, altitude: 400}}}"
• In mavproxy, update the rally list and make sure the rally point appears on the map

• use the service call to get the Rally at index 0: ros2 service call /ap/rally_get ardupilot_msgs/srv/RallyGet "index: 0"

[Ryanf55]

Nice addition, great to see some more interfaces like this.

[timtuxworth]

The altitude needs a frame.

[tizianofiorenzani]

@timtuxworth I have set the altitude frame explicitly now.
I don't have a clear answer for what to do with the break_altitude: RallyLocation expects that in Above Home level.

[tizianofiorenzani]

As there is no code associated with the Rally flags, the DDS interface has been limited to only the Geopoint and the altitude frame

[tizianofiorenzani]

Rebased and pushed again.

[Ryanf55]

Functionally, looks good. Just needs code cleanup and removal of potential link-flooding.

[tizianofiorenzani]

@Ryanf55 suggestions applied, rebased and tested.

[timtuxworth]

Maybe it's something I need to understand about ROS, but how would you know if there is more than one rally point, there doesn't seem to be a request rally count message, do you just keep requesting indexes until you get a fail?

[tizianofiorenzani]

When you set a rally, it responds with the length of the Rally list. When you request a particular index, it fails if it does not exist. You can also use the service set call with an invalid point to obtain the length of the Rally list without adding any.

[timtuxworth]

An AP might have previously been configured with zero, one or more rally points. After booting, the only way to find out if there are any rally points would be to start requesting them at the first and then incrementally keep requesting until you get a fail, or to request some really large index number (what the largest valid number +1?) to get the number.

I'm not sure I understand how that's ok.

[Ryanf55]

I agree that we should ensure this API works if there are pre-existing rally points. A sequence diagram could be helpful

[tizianofiorenzani]

Would adding a field to the set_rally response that provides the number of rally points solve this issue?

[tizianofiorenzani]

@timtuxworth the rally_get has been extended with a size field. Now it returns also the size of the list.

[timtuxworth]

Good answer. Should Set also return size? Oh I see it does, so at least one example needs the size return added.

[tizianofiorenzani]

Set does return size already

[tizianofiorenzani]

In my example, Size already reports the current size:

response:
ardupilot_msgs.srv.RallySet_Response(success=True, size=1)

[timtuxworth]

Looks good

[tizianofiorenzani]

Rebased, ready for another round.

[tizianofiorenzani]

@Ryanf55 @timtuxworth I applied your suggestions. It should be good to go now.

[timtuxworth]

A couple of minor documentation/example fixes. I'm confused why altitude is documented as having 0 digits (which I would question), whereas several examples who altitudes with at least 1 decimal place. For small vehicles 1 meter is a long way.

[timtuxworth]

Good answer. Should Set also return size? Oh I see it does, so at least one example needs the size return added.

[timtuxworth]

LGTM - thanks for your work on this!

[tizianofiorenzani]

@timtuxworth @Ryanf55 could you approve this PR?