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

Fast Copter Fences #25698

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Fast Copter Fences #25698

merged 1 commit into from
Feb 28, 2024

Conversation

andyp1per
Copy link
Collaborator

@andyp1per andyp1per commented Dec 4, 2023

An attempt at checking fences quickly to avoid massive breaches

Adds support for auto-enabling fence floors on copter

Current master is fixed at 3Hz so if you are going 30m/s you will be 10m before the fence before it is even checked

Demo: https://youtu.be/8umjR5EAKfg

@andyp1per
Copy link
Collaborator Author

I am testing using this script: https://gist.github.com/andyp1per/85e5e29dac031e4c93d6e83d8f71641f

Here is an example log: 00000056.zip

The copter flies through the fence at a very high speed and is about 50m past it before the fence kicks in. On the way down the fence just about kicks in before it hits the ground.

@andyp1per andyp1per requested a review from rmackay9 December 4, 2023 17:07
ArduCopter/mode.cpp Outdated Show resolved Hide resolved
@Hwurzburg
Copy link
Collaborator

Hwurzburg commented Dec 22, 2023

does this fix plane also? I think mine fixes both....note also that autoenable handles floor differently for each case

@andyp1per
Copy link
Collaborator Author

does this fix plane also? I think mine fixes both....note also that autoenable handles floor differently for each case

Well it fixes the bugs I want fixed of which there are a few. Copter is actually missing the autoenable feature which this implements amongst other things. What is the bug you think that needs fixing in plane?

@Hwurzburg
Copy link
Collaborator

Hwurzburg commented Dec 22, 2023

Fences have the following issues:

  • MIN_ALT fence records breach even if floor is not enabled
  • MIN_ALT floor always disabled on boot irrespective FENCE_ENABLE, never gets enabled unless: autoenabled, or overt enable(GCS or switch)
  • MIN_ALT floor stays disabled after disable floor for landing, used by NAV_LAND, QLAND and other VTOL landings in Plane, LAND, Copter RTL
  • Plane automatically ignores any breach while performing a breach action. If landing occurs, MIN_ALT breach is recorded but not cleared after landing, preventing mode changes if mode change prevention is enabled (Copter allows mode changes always). This prevents mode changes after landing.
  • AutoEnable on arming or not using AutoEnabled never disables a floor for landing. So all the above situations which think they disable floor, wont.

Any changes to AC_Fence module without considering how it may impact Plane or future fixes for plane fence issues is a problem IMHO

Having floor always enable once MIN_ALT is reached by itself wont impact future fixes and only impacts the MIN_ALT fence, whether that fence is setup or not...but it must be able to do this after disables from any altitude, and after landing for both Copter and Plane

And CI heavily incorporates all the existing wierdness of fences, so virtually any ,even minor, changes breaks something!

@@ -24,7 +24,8 @@ void Copter::fence_check()
if (new_breaches) {

if (!copter.ap.land_complete) {
Copy link
Member

Choose a reason for hiding this comment

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

Its a bit odd that we don't print if landed. Its the current behavior of course. But now it will print when the breach clears in all caces, so you can end up reporting the clear but not the breach.

ArduCopter/mode.cpp Outdated Show resolved Hide resolved
// @Description: 0:Disable mode change following fence action until fence breach is cleared. When bit 1 is set the allowable flight areas is the union of all polygon and circle fence areas instead of the intersection, which means a fence breach occurs only if you are outside all of the fence areas.
// @Bitmask: 0:Disable mode change following fence action until fence breach is cleared, 1:Allow union of inclusion areas
// @Description: 0:Disable mode change following fence action until fence breach is cleared. When bit 1 is set the allowable flight areas is the union of all polygon and circle fence areas instead of the intersection, which means a fence breach occurs only if you are outside all of the fence areas. When bit 2 is set any active minimum altitude floor will be disabled.
// @Bitmask: 0:Disable mode change following fence action until fence breach is cleared, 1:Allow union of inclusion areas, 2:Auto-disable floor fence on disarming
Copy link
Member

Choose a reason for hiding this comment

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

Why would you have this bit ever disabled? I would have thought you would also want the floor disabled when disarming if you have AutoEnable::ALWAYS_ENABLED

Copy link
Collaborator Author

@andyp1per andyp1per Jan 1, 2024

Choose a reason for hiding this comment

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

@IamPete1 because the tests check for the current disarmed behaviour. We can change the tests of course, but we need to be all agreed that this is the correct behaviour - because it is a change. At the moment I have added new tests for the new behaviour.

@andyp1per
Copy link
Collaborator Author

@Hwurzburg

  • MIN_ALT fence records breach even if floor is not enabled

I think this is fixed by this PR, but its possible there is something special going on in plane.

  • MIN_ALT floor always disabled on boot irrespective FENCE_ENABLE, never gets enabled unless: autoenabled, or overt enable(GCS or switch)

Yes, this is the main fix of this PR. Without this MIN_ALT is mostly useless on Copter.

  • MIN_ALT floor stays disabled after disable floor for landing, used by NAV_LAND, QLAND and other VTOL landings in Plane, LAND, Copter RTL

Can you expand on this? Current behaviour is that fence remains enable but is ignored for landing so that you cannot rearm. This PR provides an option which automatically clears the breach once you have landed so that you can rearm without rebooting.

  • Plane automatically ignores any breach while performing a breach action. If landing occurs, MIN_ALT breach is recorded but not cleared after landing, preventing mode changes if mode change prevention is enabled (Copter allows mode changes always). This prevents mode changes after landing.

For copter see above - I think this is partially fixed (second part). I'm not sure that the first part is the wrong behavior - once your breach action has engaged you want it to stay engaged. I did fix a bug which preventing scripting seeing new breaches.

  • AutoEnable on arming or not using AutoEnabled never disables a floor for landing. So all the above situations which think they disable floor, wont.

I don't quite get this one but I think its plane specific as auto enable is not currently supported on copter without this PR. With this PR I am able to land ok (at least in RTL).

@andyp1per andyp1per requested a review from IamPete1 January 1, 2024 13:39
@Hwurzburg
Copy link
Collaborator

@andyp1per any change to AC_Fences must be THOROUGHLY tested for impacts on Plane....it took me two days of SITL sims on just MIN_ALT fence to find all the hidden anomalies...

@andyp1per
Copy link
Collaborator Author

@andyp1per any change to AC_Fences must be THOROUGHLY tested for impacts on Plane....it took me two days of SITL sims on just MIN_ALT fence to find all the hidden anomalies...

Sure. The solution here, of course, is making sure that the autotests cover the edge cases. I have added some tests but more can be added - be good to know what tests you think are missing. I also have spent a long time flying this in RF, that also enabled me to uncover many of these hidden "gems". Clearly the fencing code is not as widely used as we suspect otherwise they would have been revealed sooner.

libraries/AC_Fence/AC_Fence.cpp Outdated Show resolved Hide resolved
@@ -147,6 +147,9 @@ const AP_Scheduler::Task Copter::scheduler_tasks[] = {

SCHED_TASK(rc_loop, 250, 130, 3),
SCHED_TASK(throttle_loop, 50, 75, 6),
#if AP_FENCE_ENABLED
SCHED_TASK(fence_check, 25, 100, 7),
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth moving fence checking to an IO callback, or add a FENCE_OPTIONS bit for fast fence checking

libraries/AC_Fence/AC_Fence.cpp Outdated Show resolved Hide resolved
@rmackay9
Copy link
Contributor

rmackay9 commented Jan 2, 2024

We discussed at length on the dev call but disagreed about how fast the fence check could be run. Various suggestions came up re adding a compile time update rate parameter, a parameter option, dependent upon the CPU time etc.

I think it would be good to first get a baseline of how expensive the fence check is. For example, how complex can the fence be (e.g. how many polygon points) and still complete within 1ms on an F4 CPU?

@andyp1per
Copy link
Collaborator Author

Fence fixes split out into #25875

@andyp1per
Copy link
Collaborator Author

andyp1per commented Feb 26, 2024

On a CubeBlack I get about 10us per cycle for general fence checks, if I add a poly fence with 25 points It goes to about 22us percycle, if I increase this to include an exclusion fence with another 42 points I get about 33us. So being ultra conservative lets say we can get 1 point per micro that means we can have a 1000 point fence for 1ms on an F4, but in all likelihood considerably more than this. Not sure what this means for my update rate, but I think we can go a decent amount faster than 3Hz.

Copy link
Contributor

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

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

Thanks for the testing. Personally I'd make it 10hz because of the law of diminishing returns but it's probably fine at 25hz too. A very fast copter flying at 20m/s will get 6.6m breach currently, only 2m at 10hz, 0.8m at 25hz.

@andyp1per andyp1per requested a review from tridge February 27, 2024 11:52
@tridge tridge merged commit f0b691d into ArduPilot:master Feb 28, 2024
72 checks passed
@andyp1per andyp1per deleted the pr-fast-fences branch February 28, 2024 08:29
@andyp1per
Copy link
Collaborator Author

Maybe for 4.5.1 if the other fence PR gets finally approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 4.5.0-beta3
Development

Successfully merging this pull request may close these issues.

6 participants