Skip to content

AC_Fence: Do not perform unnecessary parameter checks #29896

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.

Sign up for GitHub

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
wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −2
Open

AC_Fence: Do not perform unnecessary parameter checks #29896

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions libraries/AC_Fence/AC_Fence.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,11 @@ bool AC_Fence::pre_arm_check_polygon(char *failure_msg, const uint8_t failure_ms
// additional checks for the circle fence:
bool AC_Fence::pre_arm_check_circle(char *failure_msg, const uint8_t failure_msg_len) const
{
if (!(_configured_fences & AC_FENCE_TYPE_CIRCLE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The check is a sanity check prior to vehicle arming. If one was to enable or make use of circular fences during flight, and _circle_radius is not correctly set, what is the resultant behaviour?

I would tend to prefer to have sane values set whether or not I am actively making use of the fence.

Copy link
Contributor Author

@muramura muramura Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe that performing health checks on unused parameters is unnecessary.
Even if an anomaly is detected in the health check of an unused parameter, it has no impact on operation.
I would like anomaly notifications to be limited to issues that actually affect operation.

I think it is unacceptable if the system cannot arm due to an anomaly in an unused parameter.
I consider it spam if anomalies in unused parameters are repeatedly notified.

Health checks on unused parameters can cause misunderstandings for users.
Users may mistakenly believe that the parameters are actually being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

The check is a sanity check prior to vehicle arming. If one was to enable or make use of circular fences during flight, and _circle_radius is not correctly set, what is the resultant behaviour?

There are many parameters in ArduPilot that you could change at runtime and get anomalous behaviour. Perhaps if the default values of variables would be problematic we should care, but I'm not sure this is such a case.

Is there a workflow you're thinking of where a user would go and enable these parameters in-flight go get some sort of behaviour - and where they wouldn't already have considered the radius ahead of time?

Not unrelated to the fact that you can say, "honour polygon fences" and not actually have any, BTW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the polygon fence is enabled, the presence of the polygon fence is being checked.
There are cases where only the polygon fence is enabled.
Other fences should be treated the same way as the polygon fence.

if (!(_configured_fences & AC_FENCE_TYPE_POLYGON)) {
    // not enabled; all good
    return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, I'm certainly not holding this up. And struggle to think of a case where it might make less sense.

I think the catch is, you can have configured fences, but not have them enabled. In that case, you should be advised when your configured fence has an issue.

// not enabled; all good
return true;
}

if (_circle_radius < 0) {
hal.util->snprintf(failure_msg, failure_msg_len, "Invalid Circle FENCE_RADIUS value");
return false;
Expand All @@ -429,12 +434,12 @@ bool AC_Fence::pre_arm_check_circle(char *failure_msg, const uint8_t failure_msg
// additional checks for the alt fence:
bool AC_Fence::pre_arm_check_alt(char *failure_msg, const uint8_t failure_msg_len) const
{
if (_alt_max < 0.0f) {
if ((_configured_fences & AC_FENCE_TYPE_ALT_MAX) && (_alt_max < 0.0f)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto for these changes.

Copy link
Contributor Author

@muramura muramura Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe that performing health checks on unused parameters is unnecessary.
Even if an anomaly is detected in the health check of an unused parameter, it has no impact on operation.
I would like anomaly notifications to be limited to issues that actually affect operation.

I think it is unacceptable if the system cannot arm due to an anomaly in an unused parameter.
I consider it spam if anomalies in unused parameters are repeatedly notified.

Health checks on unused parameters can cause misunderstandings for users.
Users may mistakenly believe that the parameters are actually being used.

If FENCE_TYPE is allowed to be changed while armed, the parameters for each fence must be checked every time a fence check is performed.
The parameters of each fence can be changed while armed.
Currently, the implementation assumes that FENCE_TYPE is not changed while armed.

hal.util->snprintf(failure_msg, failure_msg_len, "Invalid FENCE_ALT_MAX value");
return false;
}

if (_alt_min < -100.0f) {
if ((_configured_fences & AC_FENCE_TYPE_ALT_MIN) && (_alt_min < -100.0f)) {
hal.util->snprintf(failure_msg, failure_msg_len, "Invalid FENCE_ALT_MIN value");
return false;
}
Expand Down
Loading