-
Notifications
You must be signed in to change notification settings - Fork 19.9k
AC_Fence: added FENCE_ALT_MIN_TP and FENCE_ALT_MAX_TP #31619
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
base: master
Are you sure you want to change the base?
Conversation
this allows for fence altitudes to be above ground level, absolute, origin or above home (the default and current behaviour)
|
Copter avoidance and plane FBWB altitude control will need updating to respect the new frames. |
fbbd373 to
d5695a5
Compare
|
@IamPete1 I have done OA and copter but its not clear to me that there is anything reasonable to do for plane. Plane appears to already be guessing something reasonable. LMK what change in behaviour you would like to see. |
|
Plane needs a update here: ardupilot/ArduPlane/altitude.cpp Lines 412 to 421 in 2a25847
Probably better to either remove the |
|
I'd like to give this a test. I just want to be sure that we've tested all the edge cases. Some things to worry about:
|
peterbarker
left a comment
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.
Needs autotests.
actually @IamPete1 this turns out to be the wrong change. The maths and checks are much, much simpler if everything is done in the configured frame. The odd one out are these plane methods that require a relative frame, so I have added methods that are specifically relative frame to cope with this particular use case. |
pre-arm check for terrain frames update safe alt min appropriately only enable terrain checks if terrain is available introduce get_relative_safe_alt_max_m and get_relative_safe_alt_min_m methods
737bf0c to
d8e78e8
Compare
timtuxworth
left a comment
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.
Some thoughts and suggestions.
| get our altitude in the FENCE_ALT_TYPE frame | ||
| return false if not available | ||
| */ | ||
| bool AC_Fence::get_alt_U_in_frame(float &alt, Location::AltFrame alt_frame) const |
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.
What is the "U" here? Up? If it's up, altitude is always up isn't it? So not needed?
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 I could change it, it was just getting very confusing to track the places where things were down instead.
| hal.util->snprintf(failure_msg, failure_msg_len, "Invalid FENCE_ALT_MIN value"); | ||
| return false; | ||
| } | ||
| #if AP_TERRAIN_AVAILABLE |
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.
This isn't correct; you want this prearm check to fail if your require the terrain database but it isn't available because it is compiled out.
Consider adding it into AP_Arming::terrain_checks instead - it will slot in right next to where we do the same stuff for mission.
| } | ||
|
|
||
| /* | ||
| get our altitude in the FENCE_ALT_TYPE frame |
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.
| get our altitude in the FENCE_ALT_TYPE frame | |
| get our altitude in the supplied frame |
| */ | ||
| bool AC_Fence::get_alt_U_in_frame(float &alt, Location::AltFrame alt_frame) const | ||
| { | ||
| const auto &ahrs = AP::ahrs(); |
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 aren't you just getting the current location and then asking that for your height in the frame you want?
| return true; | ||
| } | ||
|
|
||
| // update safe alt min |
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.
| // update safe alt min | |
| // update safe alt min - home *must* be set before this function is called |
(and in the header file)
... or assert it is set in SITL somehow. Or both.
| if (_alt_min_type == Location::AltFrame::ABSOLUTE) { | ||
| float home_alt; | ||
| UNUSED_RESULT(AP::ahrs().get_home().get_alt_m(Location::AltFrame::ABSOLUTE, home_alt)); | ||
| _safe_alt_min_m = _alt_min_m - home_alt - _margin_m; |
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.
| _safe_alt_min_m = _alt_min_m - home_alt - _margin_m; | |
| _safe_relhome_alt_min_m = _alt_min_m - home_alt - _margin_m; |
| #endif | ||
|
|
||
| // get altitude in alt max frame | ||
| bool get_alt_U_in_alt_max_frame(float &alt) const { return get_alt_U_in_frame(alt, (Location::AltFrame)_alt_max_type.get()); } |
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.
Do we still need the casts here?
| home_loc = self.mav.location() | ||
|
|
||
| self.max_alt_fence_frame(0, home_loc.alt + 100) # absolute | ||
| self.max_alt_fence_frame(1, 100) # above home |
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.
Can we do a little bit of setup here so home and origin are different altitudes, please?
That really should just be a set_home call.
| '''Test Max Alt Fence''' | ||
| self.takeoff(10, mode="LOITER") | ||
| """Hold loiter position.""" | ||
| def max_alt_fence_frame(self, frame, alt, terrain=0): |
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.
Please pass in an absolute altitude that you expect the vehicle to be close to (+/-5 metres?) when the breach happens and make sure that's about where the vehicle is when it breaches.
this allows for fence altitudes to be above ground level, absolute, origin or above home (the default and current behaviour)
This is a rebase of #29761 which I could not push to