-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Open Drone ID Live GNSS and Arm status prototype #21647
base: main
Are you sure you want to change the base?
Conversation
@dirksavage88 thanks! Make sure to rebase on top of |
Can you revert the whitespace changes, it makes the overall PR huge and hard to review. We also need to reconcile this with #21652. |
0bf6b4e
to
77341fa
Compare
I reverted files with large diffs. Also rebased to main @julianoes For reconciling with the other PR, I really intended this draft PR to be for people to experiment with the RID modules and get something working since there wasn't anything out there yet. |
@@ -1729,7 +1737,11 @@ Mavlink::configure_streams_to_default(const char *configure_single_stream) | |||
configure_stream_local("MAG_CAL_REPORT", 1.0f); | |||
configure_stream_local("MANUAL_CONTROL", 5.0f); | |||
configure_stream_local("NAV_CONTROLLER_OUTPUT", 10.0f); | |||
configure_stream_local("OPEN_DRONE_ID_ARM_STATUS", 1.f); |
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 are we streaming OPEN_DRONE_ID_ARM_STATUS? Its inbound from the remoteID hardware. Should be used to indicate healthy/unhealthy for the remoteID.
I.e. an update to #21652
I'm probably missing something ...
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.
The Bluemark module requires the stream to work correctly. Without it, the module health checks are not satisfied and arming is prevented. Perhaps Bluemark can clarify on why this is.
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.
how much of this behaviour is defined in the spec vs left to the manufacturer to figure out?
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.
@tridge may know more about the how the spec has been implemented. The vendor may have just an abstraction layer on top of ODID core library for their specific hardware. Or they may use proprietary software and just satisfy the mavlink ODID definitions and don’t use ardu (or ODID core library)
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.
Yeah even in Ardupilot, the ARM_STATUS
message isn't getting sent out, so I am puzzled as to why Bluemark module needs it 🤔
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.
The Bluemark module requires the stream to work correctly. Without it, the module health checks are not satisfied and arming is prevented. Perhaps Bluemark can clarify on why this is.
Well in the standard Remote ID regulation, you need to make sure the Remote ID information is correct and there are no failures. So if the Remote ID module detect an issue, it should report it back to the GCS.
Note there are open PRs that needs to be merged into master of ArduRemoteID: PR#101 and PR#102 that will improve these checks or its status message.
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.
@BluemarkInnovations I was going to sarcastically say where did you get that "requirement", but you're absolutely right - its in the MAVLink spec:
Similarly, the autopilot and GCS components must listen to the ARM_STATUS from the RID transmitter component(s) and not allow the UA to be airborne before the RID transmitter component(s) is ready. During flight, if the arm status indicates a failure, similar action must be taken as for a lack of HEARTBEAT messages.
The vehicle shouldn't start if it doesn't have this so what the GCS does is irrelevant. Though this is needed for monitoring the precise cause of arming failure, unless the flight stack were to emit an event (and none is standardized).
@dirksavage88 What is interesting here is that the status would also have to be updated in LOCATION (that is what appears to happen if the HEARTBEAT disappears).
I'll see if I can tidy the spec: mavlink/mavlink-devguide#511
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.
FWIW I don't think this should be streamed by default. If there is no GPS then firing this off is a waste of bandwidth. perhaps it could be requested by GCS or enabled by a parameter.
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.
Late to the party as I only started looking into Remote ID yesterday, but I had the same initial reaction as @hamishwillee back in June.
In my mind it's more logical if you just set up mavlink forwarding so that the GCS can subscribe directly to the topic from the Remote ID hardware without going through the "useless" steps of repackaging the incoming message and publishing it as uORB only to subscribe to the uORB message to republish a mavlink message with basically the same content as the initial message.
Maybe I am too naive and miss something essential here?
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.
Yes, that would make sense, just forwarding it. However, for a module that talks DroneCAN or some other protocol, this might be required again.
Is this planned for PX4 v1.14? |
For reference, user testing report in discord: https://discord.com/channels/1022170275984457759/1038284900081614879/1115609787317628998 |
@hamishwillee not by design, but would make sense if its a clean cherry pick |
@dirksavage88 Btw, you have merged the master branch into your branch ( It is not critical, but just for your information! More info: https://medium.com/@harishlyadav/when-to-use-git-rebase-explained-3c8192cba5c7 |
BASIC ID is now cached. @jnomikos will take a look at the system message. I still am not sure how to go about the serial number or the takeoff vs live vs fixed (and thus standard remote ID vs broadcast module add-on). I can not arm with takeoff or fixed when FAA is selected: trying to select either in QGC does not work. If I select EU it allows to choose fixed/takeoff. @Davidsastresas is this not implemented for the FAA option in QGC? I would like to be able to choose takeoff or fixed for FAA rules (under broadcast module exception). |
Hello @dirksavage88, back when we implemented this, for FAA it wasn't allowed the fixed/takeoff location, that is why it is only available for CE. Has it changed? |
This is the case for standard remote ID under FAA rules. I think the confusion is that some recreational users who assemble their own pixhawk drones will buy a broadcast module like the Bluemark one tested in this PR and not realize that it (currently) forces them to comply with standard remote ID, where they would probably be fine under the broadcast module rules under FAA which allowed takeoff position instead of live operator coordinates. Bluemark and other vendors make their own “broadcast rules” module that has zero interface with the autopilot and this can only be used recreationally (and with a built kit, not a fully assembled product) under FAA rules starting this September. This is my current interpretation and I was also confused with the broadcast module vs standard remote ID nuances as there is overlap. |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: https://discuss.px4.io/t/px4-community-q-a-august-02-2023/33434/1 |
This should be merged. |
I am working on cleaning it up and rebasing. The plan is to merge in the next 2 weeks. Emergency status will be handled via a separate PR (still trying to figure out the QGC side of it). We have a RemoteID call this Thursday 10AM CST if you are interested. I will have a cube ID and the bluemark module in hand and can support if anyone has technical issues getting their module up and running. |
Yep, I am currently working on getting Emergency Status working |
The aim would be for this to get into 1.14 before the release in a couple weeks but depends on how much more is needed. |
So the Bluemark module is working as intended with the latest on this PR. I can even take out the system message stream and just forward the mavlink system message tream to the module from QGC (pending a push from my local branch). When I disconnect the ground station gps, the module reports operator location failure and sets arm status to 1 (fail). It also checks for the self ID, basic ID, and Operator ID and reports a failure if it doesn't see these messages. The Cube ID has no such logic. It does not have a built-in uas_id either. The Cube ID does not have logic to check the basic RID requirements and then set the arm status to fail. We would have to implement that logic in PX4. |
923e4a8
to
3745a94
Compare
Rebased to main and squashed. I removed all streams that QGC currently handles: https://github.com/mavlink/qgroundcontrol/blob/1d59706e2b1d35465307bad03151a0bb15ae7b9c/src/Vehicle/RemoteIDManager.cc#L156 with these changes in place mavlink forwarding will have to be enabled on the telem port the RID is connected to @hamishwillee and for FAA rules a external gps or tablet/phone/UDP gps source is to be used for live operator position under standard remote ID rules. The Bluemark module has internal logic to set arm status if it does not get valid location data or missing ID messages. The Cube ID from my testing does not have this logic built-in and PX4 does not handle setting arm status (nor does it do checks beyond reading arm status coming from the module). Modules Tested to work:
Modules I have not yet tested:
@dagar I would like to know your ideas for getting open drone ID baked in at compile time instead of a parameter. It can be a separate PR if needed. |
Sounds cool. Just FMI, any work on "tamper proofness" of the IDs happened? |
@hamishwillee I didn’t look into tamper proofness as it could be it’s own PR with a more focused effort (e.g more architecture and resource constraints consideration). I’m open to ideas on how to implement remote ID enforced at compile time (maybe a module?) instead of a parameter change. With this PR I reduced the streams so that QGC does most of the work. |
FWIW My understanding is that QGC will not allow the parameter to be set/it will be hidden in the UI. Commercial vendors mostly seem to want to implement their own solutions for more than that. I guess we'll talk more about what might be done in the flight stack once FAA starts making more rulings on particular implementations.
When this goes in, let's talk docs again. I presume what you mean here is that rather than streaming by default QGC makes requests for the things it expects the vehicle to emit? IF so, not sure about that - QGC might not be in the loop at setup time. But anyone :-). Any progress on getting this in? It would be great to revisit the docs that currently mention this as being experimental. |
Is this ever going in? |
/* EVENT | ||
* @description | ||
* Open Drone ID system failed to report. Make sure it is setup and installed properly. | ||
* | ||
* <profile name="dev"> | ||
* This check can be configured via <param>COM_ARM_ODID</param> parameter. | ||
* </profile> | ||
*/ | ||
reporter.armingCheckFailure(affected_modes, health_component_t::open_drone_id, | ||
events::ID("check_open_drone_id_missing"), | ||
events::Log::Error, "Open Drone ID system missing"); | ||
if (_open_drone_id_arm_status_sub.copy(&odid_module_arm_status)) { | ||
|
||
if (reporter.mavlink_log_pub()) { | ||
mavlink_log_critical(reporter.mavlink_log_pub(), "Preflight Fail: Open Drone ID system missing"); | ||
if (_param_com_arm_odid.get() == 2) { | ||
// disallow arming without the Open Drone ID system | ||
affected_modes = NavModes::All; |
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.
Wouldn't it be better to switch the order here ? First check the parameter, and fail if _open_drone_id_arm_status_sub.copy
fails ?
I'm not very familiar with the details of uORB, but don't we run the danger to not fail the check if we never received a open_drone_id_arm_status message in the first place ? I'm assuming that if it was never published then _open_drone_id_arm_status_sub.copy
could fail.
More generally, I'm assuming that if it's in an if-clause it can fail. No questions asked why. And in this case you wouldn't have the arming prevention.
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.
we are copying into a local variable (uorb 'struct') from the subscribed topic. We modify the copy and fill out the message data accordingly. Check any other commander check and the same logic applies.
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.
Example:
PX4-Autopilot/src/modules/commander/HealthAndArmingChecks/checks/offboardCheck.cpp
Line 44 in 73c77f4
if (_offboard_control_mode_sub.copy(&offboard_control_mode)) { |
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.
I didn't see any advertise()
of this uORB topic, and it's only published if you've received an incoming MAVLINK_MSG_ID_OPEN_DRONE_ID_ARM_STATUS
. Seems like .copy
is not working if it was never published: https://logs.px4.io/plot_app?log=fd3f182f-4703-415c-930e-b2aad8307eb9
I could arm with COM_ARM_ODID = 2 and definitely no RemoteID connected. I could arm fine with QGC, which I should NOT be able to do.
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.
Strange, I remember testing removing the RID module and watching arm status prevent arming. What QGC version are you using?
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.
you can add a px4_info in that if clause as a sanity check, but it should still prevent arming since wether the topic info is being advertised should be independent of the rid module presence (e.g you can still copy the struct empty or not):
PX4-Autopilot/src/modules/commander/HealthAndArmingChecks/checks/openDroneIDCheck.cpp
Line 57 in 73c77f4
if (!context.status().open_drone_id_system_present) { |
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.
I didn't remove the RID module, I never plugged it in in the first place. I'm using a daily build from a few months ago. (around 27.11.23). I could do the same thing without QGC though, so it shouldn't matter.
If we don't pass line 50 because open_drone_id_arm_status
was never advertised (published) in the first place then it doesn't matter how we would refuse the check in line 57 as we never enter inside.
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.
@ThomasRigi please try with the latest commit on the feature branch on my fork
I had a look at this PR and think that in general it's good to go in except for my comment on the ODID arming check sequence (#21647 (comment)) I also reopened an old discussion because I didn't quite understand / agree with it, but I might be biased from only using the Flarm Atom, who tell you in their manual to enable mavlink forwarding. If other units can be set up without mavlink forwarding, I guess that it makes sense to actively republish it from PX4, so I'm not going to insist on this point if consensus was clear that republishing is the only "safe enough" way. |
Bump, should this still go in? |
…. Removed all streams that QGC sends currently. Signed-off-by: dirksavage88 <[email protected]>
Co-authored-by: Thomas Stauber <[email protected]>
Signed-off-by: dirksavage88 <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
That being said I'm not sure the changes are correct. |
It needs to be tested again with the RID modules @julianoes IIRC the basic ID file was removed and mavlink forwarding was enabled (forward basic ID) to save resources on the FC. |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: |
While implementing #23113 I find that we need at least part of this PR, specifically the operator ID. Basically, QGC would send the operator ID and PX4 forwards it to the CAN module. I'm not sure what other messages QGC sends that need to be translated. |
Possibly the operator initiated emergency triggered by https://mavlink.io/en/messages/development.html#MAV_CMD_ODID_SET_EMERGENCY - I'm having a bit of trouble remembering the detail, but I think the right thing to do is for https://mavlink.io/en/messages/common.html#MAV_ODID_STATUS to actually be set automatically by the flight stack too |
-Changed operator position to Live GNSS: To test you will need a tablet/laptop with built-in GPS or a USB GPS receiver module. Using QGC daily (they have merged the Remote ID support) you will need to select remote ID in the settings -> mavlink/qgroundcontrol#10600
-Added prototype arm status through the health and arming checks module. This is just for testing purposes and the actual implementation can be different. If you have remote ID enabled but don't have a module plugged in, you will still be able to arm. However if you have a module plugged in and it is not receiving valid GPS or some other failure in operator ID or basic ID it will result in a preflight fail and not allow you to arm.
-Added mavlink receiver support for drone ID arm status
According to David Sastre we are trying to verify the following settings:
GPS indicator: If set to FIXED, it will go green if valid lat/long is introduced. If it is set to LIVE, it will go green if valid GPS is received, and latest GCS GPS info is received less than 5 seconds ago. It will go red otherwise. Also, if it is set to LIVE, but region is set to FAA, it will go red if GCS GPS doesn't provide altitude ( needed for FAA region ), but for EU this doesn't apply as altitude is not mandatory.
Basic ID: will go green if the armstatus message received is "GOOD_TO_ARM". It will go red if armstatus message reports an error string "missing basic_id message". This is not ideal, but it is the only way given the circumstances of detecting precisely when it is just Basic ID failure. A bitmask or something in armstatus message will be more appropriate, but these are the cards we can play with.
Arm status: will go green if armstatus message is good. If armstatus message contains errors it will go red.
Comms indicator: will go green as soon as we receive an armstatus message. Will go red as soon as the timeout ( 2.5 seconds ) expires with no message received.
Operator ID: will only check if the data set in QGC is valid, not empty and valid ID type.