ADSB Plugin #696

TSC21 · 2017-04-17T18:21:15Z

This adds a ADS-B (Automatic dependent surveillance-broadcast) wrapper so it can allow data reception/publish of modules.

Add handler for ADSB_VEHICLE and publish data to ROS msg - Ping RX
Add ROS subscriber to publish msg to FCU (in case the ADS-B system is connected offboard - pingUSB)

vooon

Who is gonna to use that plugin? Which FCU support that?

I don't want to merge plugin without a users.

vooon · 2017-04-18T06:48:50Z

mavros/include/mavros/utils.h

@@ -35,6 +35,10 @@ namespace utils {

 using mavconn::utils::format;

+using mavlink::common::ADSB_ALTITUDE_TYPE;
+using mavlink::common::ADSB_EMITTER_TYPE;
+using mavlink::common::ADSB_FLAGS;


Where then? I put there so the below code in utils.h could work.

vooon · 2017-04-18T06:49:58Z

mavros/src/lib/enum_to_string.cpp

+static const std::array<const std::string, 2> adsb_altitude_type_strings{{
+/*  0 */ "ADSB_ALTITUDE_TYPE_PRESSURE_QNH", // Altitude reported from a Baro source using QNH reference
+/*  1 */ "ADSB_ALTITUDE_TYPE_GEOMETRIC",  // Altitude reported from a GNSS source
+}};


ADSB_ALTITUDE_TYPE_ prefix should be removed.

vooon · 2017-04-18T06:51:30Z

mavros/src/lib/enum_to_string.cpp

+	}
+
+	ROS_ERROR_STREAM_NAMED("uas", "ADSB_ALTITUDE_TYPE: Unknown type: " << type);
+}


No return - error! And what you will return if there is no requested type?

Suggestion?

vooon · 2017-04-18T06:52:00Z

mavros/src/lib/enum_to_string.cpp

+// for k, e in enum:
+//     value = e.name
+//     value = value.replace("ADSB_EMITTER_TYPE_",'')
+//     value.replace("{ename}",'')


Wtf? Look at sensor orientation code.

vooon · 2017-04-18T06:53:07Z

mavros/src/lib/enum_to_string.cpp

+/* 16 */ "VALID_CALLSIGN",                //
+/* 32 */ "VALID_SQUAWK",                  //
+/* 64 */ "SIMULATED",                     //
+}};


You cannot decode bit field that way!

Is this a bit field?

vooon · 2017-04-18T07:02:49Z

mavros_msgs/msg/ADSBVehicle.msg

+
+uint32 ICAO_address
+geographic_msgs/GeoPoint lla
+std_msgs/String altitude_type # Type from ADSB_ALTITUDE_TYPE enum


http://wiki.ros.org/msg#Fields string

vooon · 2017-04-18T07:03:04Z

mavros_msgs/msg/ADSBVehicle.msg

+uint16 heading  # cog
+uint16 hor_velocity
+int16 ver_velocity
+char[9] callsign  # callsign, 8+null


It is string!

vooon · 2017-04-18T07:04:06Z

mavros_extras/src/plugins/adsb.cpp

+
+		std_msgs::Header header;
+		header.stamp = ros::Time::now();//TODO: request add time_boot_ms to msg definition
+		header.frame_id = frame_id;


Why? Did you use that header twice?

vooon · 2017-04-18T07:05:04Z

mavros_msgs/msg/ADSBVehicle.msg

+char[9] callsign  # callsign, 8+null
+std_msgs/String emitter_type  # Type from ADSB_EMITTER_TYPE enum
+std_msgs/Time tslc
+std_msgs/String data_status  # Flags from ADSB_FLAGS


Leave binary data here. Who want to parse strings?

It's more useful parsed than otherwise. For debugging and for usage. They are INFO fields...

vooon · 2017-04-18T07:05:46Z

mavros_msgs/msg/ADSBVehicle.msg

+uint16 hor_velocity
+int16 ver_velocity
+char[9] callsign  # callsign, 8+null
+std_msgs/String emitter_type  # Type from ADSB_EMITTER_TYPE enum


why not parse it?

TSC21 · 2017-04-18T08:47:18Z

Who is gonna to use that plugin? Which FCU support that?

Check the link. Both APM and PX4 compatible FCU. I'm buying one myself. Even useful for global planning and avoidance.

vooon

That to_stirng and back is completely unneeded.

vooon · 2017-04-20T09:10:46Z

mavros/include/mavros/utils.h

@@ -65,6 +64,9 @@ std::string to_string(mavlink::common::MAV_AUTOPILOT e);
 std::string to_string(mavlink::common::MAV_TYPE e);
 std::string to_string(mavlink::common::MAV_STATE e);
 std::string to_string(timesync_mode e);
+std::string to_string(mavlink::common::ADSB_ALTITUDE_TYPE e);
+std::string to_string(mavlink::common::ADSB_EMITTER_TYPE e);
+std::string to_string(const uint16_t &bitmask);


Nope! Unacceptable.

this is for debugging purposes. it's not for msg parsing. if you just tell it is not acceptable but don't suggest a solution, it's the same as nothing...

Because it is generic type. How you suppose to deal when another uin16_t bit field will be needed?

But you may leave to_string for that enums.

if I change to

std::string flags_to_string(const uint16_t &adsb_bitmask);

is it ok?

No. Shouldn't be in lib.

No. Shouldn't be in lib.

Hmm ok

vooon · 2017-04-20T09:20:00Z

mavros_extras/src/plugins/adsb.cpp

+		adsb.hor_velocity = req->hor_velocity * 1E2;
+		adsb.ver_velocity = req->ver_velocity * 1E2;	// up is positive
+		mavlink::set_string(adsb.callsign, req->callsign);
+		adsb.emitter_type = utils::emitter_type_from_str(req->emitter_type);


vooon · 2017-04-20T09:20:06Z

mavros_extras/src/plugins/adsb.cpp

+		adsb.lat = req->lla.latitude * 1E7;
+		adsb.lon = req->lla.longitude * 1E7;
+		adsb.altitude = req->lla.altitude;	// in meters, TODO: #693
+		adsb.altitude_type = utils::alt_type_from_str(req->altitude_type);


vooon · 2017-04-20T09:20:19Z

mavros_extras/src/plugins/adsb.cpp

+		adsb_msg->hor_velocity = adsb.hor_velocity / 1E2;
+		adsb_msg->ver_velocity = adsb.ver_velocity / 1E2;	// up is positive
+		std::copy(adsb_callsign.cbegin(), adsb_callsign.cend(), adsb_msg->callsign.begin());
+		adsb_msg->emitter_type = utils::to_string(emitter_type);


vooon · 2017-04-20T09:20:25Z

mavros_extras/src/plugins/adsb.cpp

+		adsb_msg->lla.latitude = adsb.lat / 1E7;
+		adsb_msg->lla.longitude = adsb.lon / 1E7;
+		adsb_msg->lla.altitude = adsb.altitude;	// in meters, TODO: #693
+		adsb_msg->altitude_type = utils::to_string(altitude_type);


vooon · 2017-04-20T09:21:25Z

mavros_msgs/msg/ADSBVehicle.msg

+std_msgs/Header header
+
+uint32 ICAO_address
+geographic_msgs/GeoPoint lla


Replace with:

float64 latitude float64 longitude float32 altitude

I'm following ROS conventions. Also: ros-geographic-info/geographic_info@b8043a9#commitcomment-21800531.
So don't see any problem on this. After conversion applied, everything is good

vooon · 2017-04-20T09:21:49Z

mavros_msgs/msg/ADSBVehicle.msg

+
+uint32 ICAO_address
+geographic_msgs/GeoPoint lla
+string altitude_type # Type from ADSB_ALTITUDE_TYPE enum


No! Add constants to msg, and leave it integer.

you want me to add 29 constants to a msg just because you don't like a to_string parser on the cb?

vooon · 2017-04-20T09:22:46Z

mavros_msgs/msg/ADSBVehicle.msg

+uint16 hor_velocity
+int16 ver_velocity
+string callsign  # callsign, 8+null
+string emitter_type  # Type from ADSB_EMITTER_TYPE enum


vooon · 2017-04-20T09:23:00Z

mavros_msgs/package.xml

@@ -22,6 +22,7 @@
  <exec_depend>message_runtime</exec_depend>
  <depend>std_msgs</depend>
  <depend>geometry_msgs</depend>
+  <depend>geographic_msgs</depend>


Sames as https://github.com/mavlink/mavros/pull/696/files#r112418896

…parse

vooon · 2017-04-20T10:34:50Z

mavros_extras/src/plugins/adsb.cpp

+		adsb.heading = req->heading * 1E2 * 180.0f / M_PI + M_PI;
+		adsb.hor_velocity = req->hor_velocity * 1E2;
+		adsb.ver_velocity = req->ver_velocity * 1E2;	// up is positive
+		mavlink::set_string(adsb.callsign, req->callsign);


Should be mavlink::set_string_z(), _z is important.
set_string() is for message fields like param_id, where '\0' may be omitted and only indicate shorter strings than buffer.
set_string_z() ensures that last byte is '\0', which required by adsb message desc.

vooon · 2017-04-20T10:40:30Z

mavros_extras/src/plugins/adsb.cpp

+	ros::Subscriber adsb_sub;
+
+	ADSB_ALTITUDE_TYPE altitude_type;
+	ADSB_EMITTER_TYPE emitter_type;


For what purpose that two enums are here?

vooon · 2017-04-20T10:43:27Z

mavros_msgs/msg/ADSBVehicle.msg

+int16 ver_velocity
+string callsign  # callsign, 8+null
+uint16 emitter_type  # Type from ADSB_EMITTER_TYPE enum
+std_msgs/Time tslc


Oh, again. tslc.data - you think i did not stumble at this?

Isn't it better to convert to ros:time in this case?

And more over, that is not time! It is duration.

tslc uint8_t Time since last communication in seconds

vooon · 2017-04-20T10:46:52Z

mavros_extras/src/plugins/adsb.cpp

+	}
+
+	void adsb_cb(const mavros_msgs::ADSBVehicle::ConstPtr &req)
+	{


Use ROS_DEBUG_NAMED_STREAM("adsb", "ADSB: at type " << utils::to_string(e)); For enums! Use C++ streams for std::string.
Use 0x%02x format for bit-filed.

vooon · 2017-04-20T10:51:17Z

mavros_extras/src/plugins/adsb.cpp

+		adsb_msg->altitude_type = adsb.altitude_type;
+		adsb_msg->heading = adsb.heading / 1E2 / 180.0f * M_PI - M_PI;
+		adsb_msg->hor_velocity = adsb.hor_velocity / 1E2;
+		adsb_msg->ver_velocity = adsb.ver_velocity / 1E2;	// up is positive


Your heading, hor_, ver_velocity is int16! Check your message spec!

use / 100? or 1e2?

vooon · 2017-04-20T10:54:27Z

mavros_extras/src/plugins/adsb.cpp

+		adsb_msg->heading = adsb.heading / 1E2 / 180.0f * M_PI - M_PI;
+		adsb_msg->hor_velocity = adsb.hor_velocity / 1E2;
+		adsb_msg->ver_velocity = adsb.ver_velocity / 1E2;	// up is positive
+		std::copy(adsb_callsign.cbegin(), adsb_callsign.cend(), adsb_msg->callsign.begin());


Oh, man, look at available API first! mavlink::to_string()

vooon · 2017-04-20T11:03:52Z

mavros_msgs/msg/ADSBVehicle.msg

+
+uint32 ICAO_address
+geographic_msgs/GeoPoint lla
+uint16 altitude_type # Type from ADSB_ALTITUDE_TYPE enum


In mavlink it is uint8_t.

vooon · 2017-04-20T21:14:48Z

Replaced.

TSC21 added plugin APM PX4 labels Apr 17, 2017

TSC21 modified the milestone: Version 0.19 Apr 17, 2017

TSC21 self-assigned this Apr 17, 2017

vooon requested changes Apr 18, 2017

View reviewed changes

vooon requested a review from mhkabir April 18, 2017 07:09

TSC21 force-pushed the adsb_plugin branch 7 times, most recently from da9ecab to bc373b8 Compare April 20, 2017 08:22

plugin: adsb

bc373b8

vooon requested changes Apr 20, 2017

View reviewed changes

TSC21 force-pushed the adsb_plugin branch from 5f525f8 to fd6379a Compare April 20, 2017 10:31

update to_string methods; remove from_string methods; use int on msg …

fd6379a

…parse

vooon reviewed Apr 20, 2017

View reviewed changes

TSC21 force-pushed the adsb_plugin branch from e0e62b5 to 0ebcb0d Compare April 20, 2017 10:47

vooon reviewed Apr 20, 2017

View reviewed changes

TSC21 force-pushed the adsb_plugin branch from 0ebcb0d to 7f5dcab Compare April 20, 2017 13:39

replace plugin: adbs: debug calls; minor changes on lib

b16d8a5

TSC21 force-pushed the adsb_plugin branch from 7f5dcab to b16d8a5 Compare April 20, 2017 13:41

TSC21 added 3 commits April 20, 2017 16:42

msgs: ADBSVehicle: update field types

7dda2e1

plugin: adsb: add bitmask print to debug

c9e18d1

adsb: use ros::Duration instead of ros::Time

27e99e4

vooon mentioned this pull request Apr 20, 2017

TSC21 adsb plugin #697

Merged

vooon closed this Apr 20, 2017

TSC21 deleted the adsb_plugin branch June 22, 2017 19:04

ADSB Plugin #696

ADSB Plugin #696

Conversation

TSC21 commented Apr 17, 2017 • edited Loading

vooon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TSC21 commented Apr 18, 2017

vooon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TSC21 Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vooon Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TSC21 Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vooon commented Apr 20, 2017

TSC21 commented Apr 17, 2017 •

edited

Loading

TSC21 Apr 20, 2017 •

edited

Loading

vooon Apr 20, 2017 •

edited

Loading

TSC21 Apr 20, 2017 •

edited

Loading