mavlink / mavros

MAVLink to ROS gateway with proxy for Ground Control Station
Other
880 stars 990 forks source link

Waypoints don't support mission type #1157

Closed fnoop closed 3 years ago

fnoop commented 5 years ago

Issue details

Waypoints don't support mission types (fence and rally), it is hardcoded for MAV_MISSION_TYPE = MISSION: https://github.com/mavlink/mavros/issues/957 https://github.com/mavlink/mavros/commit/415b789e13524e7c91b6eaad2373ac456340385d

It looks like the waypoint message needs to be extended for the new field? http://docs.ros.org/api/mavros_msgs/html/msg/Waypoint.html

And then mavros needs to be updated to allow this field to be queried/set as per the other fields.

MAVROS version and platform

Mavros: 0.28 ROS: Melodic Ubuntu: 18.04

Autopilot type and version

[ x ] ArduPilot [ x ] PX4 (presumably)

fnoop commented 5 years ago

https://mavlink.io/en/services/mission.html#mission_types https://mavlink.io/en/messages/common.html#MISSION_ITEM https://mavlink.io/en/messages/common.html#MAV_MISSION_TYPE

vooon commented 5 years ago

No, i think it is useless in Waypoint itself. More likely that it may be used in Push/Pull services, to manipulate with this lists.

Or i misunderstood spec, and this points is part of whole mission (single list)?

SamuelDudley commented 5 years ago

@vooon My understanding is in line with yours in that while the same message is used for waypoint, rally and fence communication ( e.g. https://mavlink.io/en/messages/common.html#MISSION_ITEM ) they represent discreet functionality. So perhaps two additional push/pull services should be created (similar to the waypoint system) but with hard coded MAV_MISSION_TYPE = 1 for the fence point service and MAV_MISSION_TYPE = 2 for the rally point service (similar to how the value was hardcoded here:https://github.com/mavlink/mavros/commit/415b789e13524e7c91b6eaad2373ac456340385d) ?

vooon commented 5 years ago

@SamuelDudley perhaps. Maybe it is better to make basic waypoint io base class, and then do 3 plugins for each MISSION_TYPE. Usual mission have some additional features, like tracking item change etc. But basic exchange should be same.

Charlie-Burge commented 3 years ago

Hi @vooon I have had a go at implementing this here: https://github.com/Charlie-Burge/mavros/tree/feature/rally-points.

I was initially adding rallypoints but it became clear quickly that adding functionality to send geofences wasn't hard to add beyond this. Does this implementation agree with your comment above?

vooon commented 3 years ago

@Charlie-Burge yes, that's close to what i want to see.

A few things to change:

  1. i think mission_protocol_base.{h,cpp} would be more descriptive
  2. likely it should be part of plug-in library rather than node lib
  3. base class should implement only protocol part, not ROS interface, because i suppose it might be good to have more specific interface.
  4. also note that logging all of that calls as a wp may be error prone.
Charlie-Burge commented 3 years ago

@vooon I believe I have managed to address your comments above in my latest commit - would it be best for me to submit a pull request to review in further detail?

vooon commented 3 years ago

@Charlie-Burge yes, please do it.