AP_Radar: Nearby-craft OSD addition for formation flying#32333
Conversation
|
Howdy folks! Right now I've just reworked the original PR to work on This approach is extensible, potentially paving the way for future alternate data sources or actions with this data. However it's quite the heavy addition. If you'd prefer I can redo this to store the |
t413
force-pushed
the
radar
branch
2 times, most recently
from
fc35741 to
159ba6f
Compare
Hwurzburg
left a comment
Hwurzburg
left a comment
There was a problem hiding this comment.
Neat...but very specialized feature! As one of AP's FPV advocates I really like this!...that said, this feature should be disabled by default in all builds and use the custom firmware builder to enable... it's a good chunk of code and would be used by very few users...it needs a good wiki also linking Formation Flight...
|
Any reason this would not work on copters or rovers? It might be better at the vehicle level rather than in plane. |
|
No reason at all! I was considering that myself, but kept it closely aligned to the original PR initially. And I agree it's pretty niche. But it is a supported documented type in MSP, so I do think it would be reasonable try a MSP-stored OSD direct implementation (see my last comment) since it'd be a far far simpler and smaller implementation. I'll give that a first pass. |
Hwurzburg
left a comment
Hwurzburg
left a comment
There was a problem hiding this comment.
needs to be a build option only
|
Alright, brand new re-written approach is ready for review! My hope is that this more minimal lightweight structure can be included by default, without a default-off build option. That will greatly help adoption and the ease of including friends in formation flights. I believe, with the new minimal footprint, that it's appropriate. So instead of adding a AP_Radar library, task, and large associated flash&memory– this follows as much AP president as possible and adds the protocol support to AP_MSP. Rather than 20 files/632 lines it's now 8 files/178 lines. It now adds exactly 123 bytes to memory, something we can lower if you'd like to reduce the RADAR_MAX_PEERS. So please re-review and reconsider, given the new approach @Hwurzburg and @IamPete1! |
Hwurzburg
left a comment
Hwurzburg
left a comment
There was a problem hiding this comment.
@t413 I dont know where you came up with it only being 123bytes....its well over a 1KB....this needs to be a separate build option for this feature
also commit structure is incorrect...one commit involving two different libraries...apparently our Malformed commits CI check needs some work...
|
Thank you for your review and efforts @Hwurzburg, I appreciate it. A few things:
|
t413
force-pushed
the
radar
branch
4 times, most recently
from
36a6067 to
afb88b6
Compare
|
Alright! Added the feature enabler, defaulted to off. Separated into two separate commits, one for each library. Got the extract_features.py system working and it passes all tests.. but looks like your Again, thank you for your help– |
Hwurzburg
left a comment
Hwurzburg
left a comment
There was a problem hiding this comment.
Colcon is always problematic,,,,restarted
you did not disable the feature for F7/H7 devices....we need a discussion on whether we spend 1Kbyte on this feature...especially when it brings some F7s down from 15K free to 14K free....personally, I am against this,even though I am an FPV flyer exclusively on all vehicle types...
Also, I agree with @peterbarker that this data should be in the location storage so as to enable following, auto gimbal poiting, etc. but I dont consider it a blocker to this...the flash impact above is, however, to me
| #define RADAR_PEER_FRESH_TIME_MS 3000 | ||
|
|
||
| struct MSP_RadarPeer { | ||
| MSP::msp_radar_pos_message_t info; |
There was a problem hiding this comment.
I think you could save a fair bit by only storing what its actually used. Only lat long and alt?
|
Great to hear back @Hwurzburg and @IamPete1. A few things
|
I think this fundamentally shouldn't be limited to MSP datalink, soon somebody will want this with MAVLink, FLARM and ADSB 😀, |
|
Sounds great @LupusTheCanine, let's work on that in a new PR afterwards! This PR adds
1 and 2 are valuable additions however the data is stored in the future. Let's keep appropriate scope and move forward. I can't wait for people to want MAVLink, FLARM and ADSB additions and expand the system. In the future we could add an AP_Radar library like the original PR #25684 from 2023 did or a locations DB like in #24429. |
see the CI size tests under "All checks have passed"...for example the kakuteF7...
in the MSP.h you enable this for ANY MSP inclusion...the F7/H7 has MSP enabled...so it gets enabled...I dont think this feature needs to be enabled anytime MSP is included... |
|
Thank you @Hwurzburg! I'll look into how that test runs so I can figure out how to get this info locally. Waiting 2h for actions to run isn't feasible. And so the default parameter in |
|
Pushed a newly updated version, now with
Ready to go! Now to wait 2h for the actions to run. Update: finished! 5h this time 🫠 But it looks good! I've back-ported it to the latest stable branch and got it flashed and running great on two different wings of mine. |
|
Im so happy your working on this. I didn't even know that had this feature. I definitely want to try this out on my wings and drones too. Can't wait to see the progression here. Thanks |
|
Happy to see someone working on a solution for this! Excited to try it out! |
|
Hi Tim, |
|
Hey folks! Did lots of testing and analysis with this and came up with a few changes. Was holding out hope that they could be an additional PR later but it's been 6 weeks. So a few changes!
Hopefully we can get some movement on this? Are there reservations? As requested it's now a default-off feature. Edit: Edit2: Figured it out with fresh eyes. Should pass tests now. |
t413
force-pushed
the
radar
branch
3 times, most recently
from
14d85ab to
41880c0
Compare
Hwurzburg
left a comment
Hwurzburg
left a comment
There was a problem hiding this comment.
Tools stuff needs to be separate commits since its a different library
- Adds the MSP radar protocol type/struct (MSP2_SET_RADAR_POS, 0x100B) - stored in the static singleton AP_MSP with getters * now stored as Location + time only * includes is_healthy method * now ignores incoming duplicate data for better stale reporting - Enables reporting of nearby craft for formation flying - Used for aided formation FPV flights, broadcasting 3D position - fully disabled by default, AP will keep ignoring these packets - logs to AP::logger under "MSPR" type * now documents log fields with correct format - sends gcs messages for new and lost contacts Based on the 2023 PR ArduPilot#25684 by @MUSTARDTIGERFPV
- adds to build_options.py - adds to extract_features.py - adding to both allows it to be enabled/detected and pass tests
- enabled with OSD1_RADAR_EN=1 - displays bearing arrow, horizontal distance, and vertical distance to the current peer, cycling through healthy peers every 2 seconds. - reports stale data age - Peer tracking state (_radar_peer_id, _radar_last_peer_change) is held as screen instance members. - not included in the build by default, gated by AP_MSP_RADAR_ENABLED - extracted draw_location method from draw_home * saves quite a bit of program space
| } | ||
| #endif | ||
| GCS* gcs = GCS::get_singleton(); | ||
| if (!was_present and now_present) { |
There was a problem hiding this comment.
| if (!was_present and now_present) { | |
| if (!was_present && now_present) { |
| GCS* gcs = GCS::get_singleton(); | ||
| if (!was_present and now_present) { | ||
| gcs->send_text(MAV_SEVERITY_INFO, "MSP Radar %c detected", msg.radar_no + 'A'); | ||
| } else if (was_present and !now_present) { |
There was a problem hiding this comment.
| } else if (was_present and !now_present) { | |
| } else if (was_present && !now_present) { |
tridge
left a comment
tridge
left a comment
There was a problem hiding this comment.
looks good to me, but needs to be compiled on at least SITL so it doesn't bit-rot
| #endif | ||
|
|
||
| #ifndef AP_MSP_RADAR_ENABLED | ||
| #define AP_MSP_RADAR_ENABLED 0 |
There was a problem hiding this comment.
should be enabled on at least SITL so it gets compile tested, possibly should enable on all boards with more than 2M flash so it also gets built on boards like CubeRed and thus compile tested on STM32
| UNUSED_RESULT(peer->location.get_height_above(loc, vertical_distance)); | ||
| // Simple lightweight version: arrow + integer meters | ||
| const char sym = (vertical_distance >= 0) ? SYMBOL(SYM_UP) : SYMBOL(SYM_DOWN); | ||
| backend->write(x, y, false, "%c%dm", sym, (int)fabsf(vertical_distance)); |
There was a problem hiding this comment.
both location and vertical dist writing to same x,y location on screen? doesn't seem right
That would be a killer feature. MAVLink in particular, it's ArduPilot's "native" language. Just dreaming ... |
|
Yeah, glad to see this being worked on getting merged. Looks like nots of new stuff getting added to it too. |
|
Does this include @robustini 's changes here #25684 (comment) ? |
|
Thank you for this implementation! EDIT: I’ve already implemented everything, but based on my previous implementation. |
11 participants
Summary
See the original PR #25684 here by @MUSTARDTIGERFPV!
This is to add support for FormationFlight to Ardupilot. It's an open-source inter-UAS positioning & telemetry for FPV pilots. It lets FPV pilots find each other in the air through beacons sent with positioning information over LoRa or ESPNOW.
Details
This commit:
Changes from original PR
Testing
Examples