Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use ADSB messages with undefined fields in navigator #8900

Merged
merged 4 commits into from
Feb 18, 2018

Conversation

svpcom
Copy link
Contributor

@svpcom svpcom commented Feb 16, 2018

Ignore messages that have undefined fields required for collision estimation.

@dagar
Copy link
Member

dagar commented Feb 16, 2018

@svpcom check CI failure (trivial style issue).

@LorenzMeier do we still need to limit direct mavlink usage to the mavlink module?

@svpcom
Copy link
Contributor Author

svpcom commented Feb 17, 2018

@dagar Style fixed

@dagar
Copy link
Member

dagar commented Feb 17, 2018

In the past we had to be careful to not include mavlink headers outside of the mavlink module. We need to verify if that's still the case.

@LorenzMeier
Copy link
Member

That's still the case because we don't want to start leaking MAVLink as a dependency into the system. In particular now that FastRTPS is picking up speed.

@@ -58,6 +58,7 @@
#include <px4_defines.h>
#include <px4_posix.h>
#include <px4_tasks.h>
#include <v2.0/common/mavlink.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be changed as it hardwires MAVLink into PX4 as the only protocol.

@@ -963,7 +964,8 @@ void Navigator::fake_traffic(const char *callsign, float distance, float directi
strncpy(&tr.callsign[0], callsign, sizeof(tr.callsign));
tr.emitter_type = 0; // Type from ADSB_EMITTER_TYPE enum
tr.tslc = 2; // Time since last communication in seconds
tr.flags = 0; // Flags to indicate various statuses including valid data fields
tr.flags = ADSB_FLAGS_VALID_COORDS | ADSB_FLAGS_VALID_HEADING | ADSB_FLAGS_VALID_VELOCITY | \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be abstracted in the appropriate UORB message spec.

@dagar
Copy link
Member

dagar commented Feb 17, 2018

In that case @svpcom can you add the relevant fields to transponder report? https:/PX4/Firmware/blob/master/msg/transponder_report.msg#L12

@svpcom
Copy link
Contributor Author

svpcom commented Feb 17, 2018

@dagar I can add something like all_fields_defined to UORB message to indicate that ADSB message has all fields needed to run collision detection. But I'm unable to remove flags field because it used for reconstruct ADSB mavlink message to route it via telemetry link (src/modules/mavlink/mavlink_messages.cpp#1342).

It seems that is needed for directly attached (to FMU via UART) ADSB receiver which emits messages that needed not only for FMU (collision detection in navigator module), but for QGroundControl too (to display airplanes on the map).

@LorenzMeier
Copy link
Member

@svpcom You can just copy the flags from MAVLink into the uORB message and then add code when the MAVLink message gets assembled.

@svpcom
Copy link
Contributor Author

svpcom commented Feb 17, 2018

@LorenzMeier
So there are four possible implementations:

  1. Copy definition of enum ADSB_FLAGS from mavlink headers. This is simplest variant because we don't need to map mavlink adsb flags to uORB adsb flags. But if their definitions will be changed in future mavlink versions this break navigator code.
  2. Add own enum for adsb flags and make a mapping from mavlink to uORB flags in mavlink module (during parsing and reconstruction phases).
  3. Add boolean field for each flag to uORB message. This is mostly equal to previous case.
  4. Add additional boolean field (use current flags field only for mavlink packet reconstruction) and set it to true if we have ADSB_FLAGS_VALID_COORDS, ADSB_FLAGS_VALID_HEADING, ADSB_FLAGS_VALID_VELOCITY and ADSB_FLAGS_VALID_ALTITUDE set in message. This doesn't required to copy enum ADSB_FLAGS or add additional mapping.

Also the current implementation always retranslate ADSB mavlink messages. Even if these message were emitted by external receiver and got via telemetry link. I use modified dump1090 which run on companion computer and use mavlink-router to inject packets to telemetry stream. I don't have http://www.uavionix.com/products/pingrx/ and don't know which sys_id and comp_id are used in their messages to make a decision - retranslate message to telemetry link or not.

So my questions are:

  1. What implementation to choose?
  2. What to do with duplication of adsb messages which are received via telemetry link?

@LorenzMeier
Copy link
Member

The best is to start with a literal copy but still add the code that translates between them (rather than just copying the value). This is future-proof but right now the least effort. If you already see that you would like to do more medium-term than is supported by MAVLink right now you can already now diverge where needed.

LorenzMeier
LorenzMeier previously approved these changes Feb 17, 2018
@LorenzMeier
Copy link
Member

@dagar Please squash if you merge before me.

@svpcom
Copy link
Contributor Author

svpcom commented Feb 17, 2018

I've fix a typo.

LorenzMeier
LorenzMeier previously approved these changes Feb 17, 2018
@LorenzMeier
Copy link
Member

@svpcom Style check failed.

@@ -79,6 +79,18 @@
*/
#define NAVIGATOR_MODE_ARRAY_SIZE 11

typedef enum adsb_transponder_flags_e
{
PX4_ADSB_FLAGS_VALID_COORDS=1, /* | */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not define these in the uORB message?

@svpcom
Copy link
Contributor Author

svpcom commented Feb 18, 2018

@dagar Fixed

@LorenzMeier LorenzMeier merged commit 1351625 into PX4:master Feb 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants