AC_Fence: Do not perform unnecessary parameter checks #29896
Conversation
| @@ -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)) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_radiusis 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.
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
| @@ -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)) { | |||
There was a problem hiding this comment.
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.
|
This is a change in behaviour. I'm personally not against disabling pre-arm checks for sub-elements of features that are not enabled, but people do enable fences in the air and this would lead to much more dangerous issues so I am inclined to leave the functionality as is. |
|
The MAVLink command is used to enable or disable the fence, but it does not change the value of the configuration parameter FENCE_TYPE. |
Parameter checks are being performed without consulting the fence settings.
This is an unnecessary check.
Parameter validation should be carried out in accordance with the fence settings.