AC_Fence: fixed 2 regressions from 4.5.x #28729
Conversation
tridge
force-pushed
the
pr-fence-autoenable3-fix
branch
from
bc06501 to
3686029
Compare
|
@tridge I fully rely on the autotests to tell me whether a change is ok or not - you can see that this breaks one so will need some investigation. The change looks ok on the face of it. |
tridge
force-pushed
the
pr-fence-autoenable3-fix
branch
from
3686029 to
a9ba852
Compare
|
@andyp1per I had to modify the autotest, the test was relying on having the RC option causing the fence to not enable, which is what this PR fixes |
@tridge I don't use fences on a switch so don't particularly care, but I was trying to preserve behavior from 4.5 - so on 4.5 this test would fail? |
the presence of an RC switch for fence in 4.5 doesn't prevent the FENCE_AUTOENABLE=2 or 3 from working. In 4.6 before this PR if you have the RC switch then neither autoenable=2 or 3 works to enable the fence. |
|
@andyp1per can you approve? |
Needs some more discussion because the test you fixed is for FENCE_AUTOENABLE=2 |
|
@tridge I put this test into 4.5 and got a different error related to fence breach which this test does not trigger. I would really like to see the test work in 4.5 and fail in 4.6 before I approve. The branch I tried on was https://github.com/andyp1per/ardupilot/tree/test-fence-4.5 |
do you realise that the test currently in master fails in 4.5.x? |
|
@andyp1per looking into this further, I think there are much deeper problems with the fence code in master. It looks like the auto tests aren't written to check for correct behaviour, but to pass the current behaviour. |
Right, I did this branch to investigate - https://github.com/andyp1per/ardupilot/tree/test-fence-4.5 - but it fails in a different way. The reason I wanted to discuss is its not clear to me what the behaviour should be. The test changes you made completely invert the test - they may be correct, but I would like to at least understand why. I had thought I had written this test to replicate either yours or Henry's manual testing so I'd be a little surprised if its completely upside down |
Make sure that when you run it in sim_vehicle you use the same speedup as the autotests - the speedup plays havoc with some of these testss. |
we should only add fence types to the no auto-enable mask if the enable actually changed that type of fence. This fixes the case where the user has both FENCE_AUTOENABLE=3 and RCn_OPTION=11. The disable triggered by the init of the aux function was preventing the fence from auto-enabling
for polygon fences we need to check if the vehicle has a position and is inside the polygon
this is only ever set/checked within a function
the _auto_enable_mask was try to make AUX function overrides disable the FENCE_AUTOENABLE functionality. This isn't the right bevaviour, both the aux function and the auto-enable should be edge triggered, with last function taking effect
tridge
force-pushed
the
pr-fence-autoenable3-fix
branch
2 times, most recently
from
03ea95f to
1fed6d1
Compare
|
@Hwurzburg I'd like to discuss with you too |
the automatic min-alt fence should not auto-enable based on altitude if the fence has been manually disabled. This is needed to allow for a manual landing by disabling the fence before descending
the FENCE_AUTOENABLE option should be honoured even with a fence switch in the disable position
tridge
force-pushed
the
pr-fence-autoenable3-fix
branch
from
1fed6d1 to
237226a
Compare
|
The changes look good to me, but as I say - I really rely on the autotests to catch problems. Not sure what testing we want to do with this. |
|
@andyp1per I really need an approval if we follow our rules. Any chance you can approve? |
This fixes 2 regressions in fence behaviour since 4.5.x.