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

Open Drone ID Live GNSS and Arm status prototype #21647

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dirksavage88
Copy link
Contributor

@dirksavage88 dirksavage88 commented May 28, 2023

-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
remoteid

-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.

@julianoes julianoes self-requested a review May 29, 2023 03:51
@julianoes
Copy link
Contributor

@dirksavage88 thanks! Make sure to rebase on top of main to fix the conflicts, and mark the PR is ready when I should review.

@dagar
Copy link
Member

dagar commented May 30, 2023

Can you revert the whitespace changes, it makes the overall PR huge and hard to review.

We also need to reconcile this with #21652.

@dirksavage88
Copy link
Contributor Author

dirksavage88 commented May 30, 2023

Can you revert the whitespace changes, it makes the overall PR huge and hard to review.

We also need to reconcile this with #21652.

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.

@dirksavage88 dirksavage88 marked this pull request as ready for review May 31, 2023 01:58
@dirksavage88
Copy link
Contributor Author

dirksavage88 commented Jun 2, 2023

This PR is ready to be tested. I was able to successfully arm with the Dronebeacon DB200. All required ODID messages have been satisfied. Reverted some files to work with #21652
@mrpollo

@@ -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);
Copy link
Contributor

@hamishwillee hamishwillee Jun 29, 2023

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 ...

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor

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 🤔

https://github.com/ArduPilot/ardupilot/blob/5fa8b887a2df8a85822f84f41eec64bef5066895/libraries/AP_OpenDroneID/AP_OpenDroneID.cpp#L266-L326

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.

Copy link
Contributor

@hamishwillee hamishwillee Jul 5, 2023

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

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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.

@hamishwillee
Copy link
Contributor

Is this planned for PX4 v1.14?

@junwoo091400
Copy link
Contributor

For reference, user testing report in discord: https://discord.com/channels/1022170275984457759/1038284900081614879/1115609787317628998

@mrpollo
Copy link
Contributor

mrpollo commented Jun 29, 2023

@hamishwillee not by design, but would make sense if its a clean cherry pick

@junwoo091400
Copy link
Contributor

junwoo091400 commented Jun 30, 2023

@dirksavage88 Btw, you have merged the master branch into your branch (git merge master), but for rebase it is commonly done via rebase (git rebase master) to not create that merge commit.

It is not critical, but just for your information!

More info: https://medium.com/@harishlyadav/when-to-use-git-rebase-explained-3c8192cba5c7

@dirksavage88
Copy link
Contributor Author

dirksavage88 commented Jul 11, 2023

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).

@Davidsastresas
Copy link

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?

@dirksavage88
Copy link
Contributor Author

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.

@DronecodeBot
Copy link

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

@ryanjAA
Copy link
Contributor

ryanjAA commented Aug 8, 2023

This should be merged.

@dirksavage88
Copy link
Contributor Author

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.

@jnomikos
Copy link

jnomikos commented Aug 9, 2023

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

@ryanjAA
Copy link
Contributor

ryanjAA commented Aug 9, 2023

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.

@dirksavage88
Copy link
Contributor Author

dirksavage88 commented Aug 11, 2023

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.

@dirksavage88
Copy link
Contributor Author

dirksavage88 commented Aug 13, 2023

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:

  • [x ] BlueMark Db200

  • [x ] Cube ID*

  • [x ] Holybro

Modules I have not yet tested:

  1. Mro
  2. Wurzbach electronics
  • no internal logic to handle Arm status if not receiving the required ODID messages

@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.

@hamishwillee
Copy link
Contributor

Sounds cool. Just FMI, any work on "tamper proofness" of the IDs happened?

@dirksavage88
Copy link
Contributor Author

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.

@hamishwillee
Copy link
Contributor

hamishwillee commented Nov 1, 2023

@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.

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.

With this PR I reduced the streams so that QGC does most of the work.

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.

@hamishwillee
Copy link
Contributor

Is this ever going in?

Comment on lines 52 to 54
/* 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;
Copy link
Member

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.

Copy link
Contributor Author

@dirksavage88 dirksavage88 Mar 11, 2024

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example:

if (_offboard_control_mode_sub.copy(&offboard_control_mode)) {

Copy link
Member

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.

Copy link
Contributor Author

@dirksavage88 dirksavage88 Mar 12, 2024

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?

Copy link
Contributor Author

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):

if (!context.status().open_drone_id_system_present) {

Copy link
Member

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.

Copy link
Contributor Author

@dirksavage88 dirksavage88 Mar 12, 2024

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

@ThomasRigi
Copy link
Member

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.

@julianoes
Copy link
Contributor

Bump, should this still go in?

dirksavage88 and others added 4 commits May 2, 2024 10:32
@julianoes
Copy link
Contributor

  • I've rebased the PR and force pushed the branch.
  • I've also fixed formatting.
  • And I've restored OPEN_DRONE_ID_BASIC_ID.hpp which was probably removed by accident.

That being said I'm not sure the changes are correct.

@dirksavage88
Copy link
Contributor Author

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.

@DronecodeBot
Copy link

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-sync-q-a-june-12-2024/39209/1

@julianoes
Copy link
Contributor

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.

@hamishwillee
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.