PX4-Autopilot icon indicating copy to clipboard operation
PX4-Autopilot copied to clipboard

Open Drone ID Live GNSS and Arm status prototype

Open dirksavage88 opened this issue 1 year ago • 27 comments

-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 -> https://github.com/mavlink/qgroundcontrol/pull/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.

dirksavage88 avatar May 28 '23 13:05 dirksavage88

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

julianoes avatar May 29 '23 03:05 julianoes

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

We also need to reconcile this with https://github.com/PX4/PX4-Autopilot/pull/21652.

dagar avatar May 30 '23 14:05 dagar

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 avatar May 30 '23 23:05 dirksavage88

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

dirksavage88 avatar Jun 02 '23 01:06 dirksavage88

Is this planned for PX4 v1.14?

hamishwillee avatar Jun 29 '23 05:06 hamishwillee

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

junwoo091400 avatar Jun 29 '23 12:06 junwoo091400

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

mrpollo avatar Jun 29 '23 13:06 mrpollo

@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

junwoo091400 avatar Jun 30 '23 06:06 junwoo091400

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

dirksavage88 avatar Jul 11 '23 03:07 dirksavage88

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?

Davidsastresas avatar Jul 12 '23 23:07 Davidsastresas

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.

dirksavage88 avatar Jul 20 '23 17:07 dirksavage88

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

DronecodeBot avatar Aug 02 '23 15:08 DronecodeBot

This should be merged.

ryanjAA avatar Aug 08 '23 00:08 ryanjAA

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.

dirksavage88 avatar Aug 08 '23 00:08 dirksavage88

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

jnomikos avatar Aug 09 '23 15:08 jnomikos

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.

ryanjAA avatar Aug 09 '23 18:08 ryanjAA

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 avatar Aug 11 '23 01:08 dirksavage88

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.

dirksavage88 avatar Aug 13 '23 21:08 dirksavage88

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

hamishwillee avatar Aug 21 '23 22:08 hamishwillee

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.

dirksavage88 avatar Aug 22 '23 00:08 dirksavage88

@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 avatar Nov 01 '23 00:11 hamishwillee

Is this ever going in?

hamishwillee avatar Nov 28 '23 21:11 hamishwillee

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 (https://github.com/PX4/PX4-Autopilot/pull/21647#discussion_r1410473763)

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.

ThomasRigi avatar Nov 30 '23 13:11 ThomasRigi

Bump, should this still go in?

julianoes avatar Apr 04 '24 01:04 julianoes

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

julianoes avatar May 01 '24 22:05 julianoes

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.

dirksavage88 avatar May 02 '24 11:05 dirksavage88

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

DronecodeBot avatar Jun 12 '24 15:06 DronecodeBot

While implementing https://github.com/PX4/PX4-Autopilot/pull/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.

julianoes avatar Jul 10 '24 23:07 julianoes

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

hamishwillee avatar Jul 11 '24 00:07 hamishwillee