ardupilot
ardupilot copied to clipboard
ArduPilot
Tracker: option not to scan on comms loss et al.
I'm doing some tracker research and I've realized that AUTO can do at least 3 different things at once without the operator being aware of what the device is actually trying to accomplish. It sure isn't a problem for a well-set tracker, but during development being aware of that information saves a lot of time from dumb mistakes (e.g. "are my servos dead? hold on I forgot I'm withing minimum range"). I've also reduced the message spam in GUIDED so that only the updates are posted and fixed a seemingly small oversight when vehicle.location_valid would be automatically set to true at startup even without the target being present in the network.
Resolved the bits that were addressed, removed the mode switch part in system.cpp altogether, but kept the conversation in case there's a better way.
@IamPete1 I've transplanted the mode enter/exit from Plane, the exits are currently completely futureproofing as well as the boolean return of the enter, but I think it can be useful at some point and doesn't really cost too much to have.
Also added a small commit introducing a new bit to AUTO_OPTIONS that allows disabling the automatic scan functionality altogether (off by default of course).
I think the changes look good. We are failing the tracker CI test tho. So there must be some change in behavior, we need to understand what that is and either fix it or decide its better and update the test. You can try it locally with:
./Tools/autotest/autotest.py build.Tracker test.Tracker
@IamPete1 I did a few attempts at the test, but still figuring things out. I renamed the PR to better capture the real meaning and rebased over 4.7.
I've been using this code for a while now and it seems to work alright (on 4.5.7 at least). That said I ran into similar issues with minimal distance and CR servos as the lack of mode switch clarity inside AUTO caused the servo to basically keep repeating the last rotation speed when the target is within minimal radius (which is very obviously a bug) and the "stop updating servos" logic wouldn't hold here because it's based on positional servo logic. I was meant to do that as a PR itself, but I honestly don't like how much of a mess AUTO code is and will be after even more changes.
I have another suggestion while I'm still on the test. What if we make a special mode for it than can only be entered from AUTO itself? E.g. call it IDLE and use it for the cases when the tracker is both active (i.e. not STOP where the user expects servos to never move), but not tracking at the moment. This would cover both cases:
- Waiting for the radio (subject of this PR)
- Within minimal tracking distance
This way we get a benefit of just using mode enter/exit instead of a very clunky layered sub-state machine as well as the user gets a clear indication that the servos aren't moving because they are suppressed (on the field it was really a headscratcher quite a few times). Currently the user would need to rely on GCS texts to see these transitions (including ones I've added here), but with another mode the texts would rather be an extra touch. This also helps with automation a lot. Simple state diagram of what I mean:
(If it's a bad idea I'll continue on with the test and maybe add the CR servo fix too)
The changes in 38103a1 should actually apply to an issue in master branch as well, although the commit isn't cherry-pickable as is. When we set DISTANCE_MIN and the target vehicle enters the suppression radius, the servos stop updating, but this means that CR servos (and I assume OnOff too) instead of being frozen continue to play the last PWM commanded, i.e. move uncontrollably. Seen this happen to a Pelco-D setup in the wild. The fix forces servos into a state similar to being disarmed without actually marking the vehicle as disarmed (which makes sense from the operator POV as the servos might just start moving again).
The only thing I'm not super sure about is the SAFE_DISARM_PWM parameter. I assume this is meant to give the positional servos a predictable disarmed orientation, but the way I implemented it that would also concern the suppressed states (target too close, scan disabled). I'm not sure if I should handle it differently because I honestly can't grasp the usecase for this parameter. In my understanding if the user wants to stow the disarmed antenna then they will also want it stowed when it's suppressed, but I may be wrong from UX perspective.
If needed I can make a separate PR to address this issue without the rest of this branch code. It would go into update_auto instead.