-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Conversation
@svpcom check CI failure (trivial style issue). @LorenzMeier do we still need to limit direct mavlink usage to the mavlink module? |
@dagar Style fixed |
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. |
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> |
There was a problem hiding this comment.
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 | \ |
There was a problem hiding this comment.
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.
In that case @svpcom can you add the relevant fields to transponder report? https://github.com/PX4/Firmware/blob/master/msg/transponder_report.msg#L12 |
@dagar I can add something like 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). |
@svpcom You can just copy the flags from MAVLink into the uORB message and then add code when the MAVLink message gets assembled. |
@LorenzMeier
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:
|
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. |
@dagar Please squash if you merge before me. |
I've fix a typo. |
@svpcom Style check failed. |
src/modules/navigator/navigator.h
Outdated
@@ -79,6 +79,18 @@ | |||
*/ | |||
#define NAVIGATOR_MODE_ARRAY_SIZE 11 | |||
|
|||
typedef enum adsb_transponder_flags_e | |||
{ | |||
PX4_ADSB_FLAGS_VALID_COORDS=1, /* | */ |
There was a problem hiding this comment.
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?
@dagar Fixed |
Ignore messages that have undefined fields required for collision estimation.