-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Auto mode handling UX improvements #6523
Conversation
@@ -0,0 +1,324 @@ | |||
/**************************************************************************** |
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.
I think this slipped in unintentionally?
} | ||
|
||
// abort landing or auto or loiter if sticks are moved significantly | ||
if (internal_state.main_state == commander_state_s::MAIN_STATE_AUTO_LAND || |
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.
Any reason this wasn't combined with the same behaviour for RTL above?
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.
Not really. We should do a function doing the comparisons.
src/modules/commander/commander.cpp
Outdated
// transition to previous state if sticks are touched | ||
if ((_last_sp_man.timestamp != sp_man.timestamp) && | ||
((fabsf(sp_man.x) - fabsf(_last_sp_man.x) > min_stick_change) || | ||
(fabsf(sp_man.y) - fabsf(_last_sp_man.y) > min_stick_change) || |
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.
Should min_stick_change factor in time between man sp changes?
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.
I think it should. We should implement a change rate.
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.
Actually - I think last_sp_man is implemented as the last setpoint in the last mode (implicitly the last manual mode). Its where you left the sticks when you handed over control. Locking that position and comparing it makes more sense than a change rate.
9e76f3c
to
ed5d39f
Compare
Thanks for taking care of this, I think it makes a lot of sense. |
@@ -194,6 +194,7 @@ static float max_ekf_dang_bias = 3.5e-4f; | |||
/* pre-flight IMU consistency checks */ | |||
static float max_imu_acc_diff = 0.7f; | |||
static float max_imu_gyr_diff = 0.09f; | |||
static float min_stick_change = 0.25f; |
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 is not a good default.
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.
I recant, that's actually an OK default.
We might need to play with it to see if it actually matters, but I'm wondering about the case where you're flying manually, violate the fence, and RTL is triggered. Letting the sticks settle back to neutral shouldn't be enough to change the state to POSCTL. |
That has been a corner case before as well. I think any pilot would try to take over again. We need to test it. To me RTL on fence breach is sort of incompatible with manual input anyway. Its very scary and counter-intuitive. So maybe breaking out of it immediately is not so bad (if you are inside the radius again). |
IMHO it comes down to the question if the operator is aware that RTL on fence breach is activated and that a fence is active in the first place. If we assume that is the case, then RTL on fence breach is fine even in manual mode. My suggestion to improve the awareness was to let RTL turn the vehicle around first before you let the user switch out (which would also make sure it's inside the fence again). |
74bf35e
to
2222002
Compare
I just quickly tested it in sitl: |
2222002
to
48ba241
Compare
* If you move in LAND, AUTO or HOLD the sticks the system will give control back to the pilot * If you do not connect any RC the system will default to HOLD and will allow you tablet control * If you gain position lock for the first time the system will re-evaluate the mode switch (so if you dropped down to alt hold it will now go into position) * If the system breaches the Geofence it will now always drop back to POSCTRL if the sticks are moved
48ba241
to
266bb94
Compare
tested on real vehicle.
if in rtl:
What exactly is the main advantage of this feature? for instance, if I am in hold and move the sticks, then I would enter position control but the remote controller still has the stick at hold. If I now want to switch back again to hold, then I first have to switch back the stick to position and then back to hold. it seems more intuitive to just switch remote stick from hold to position |
@@ -2498,15 +2506,34 @@ int commander_thread_main(int argc, char *argv[]) | |||
main_state_before_rtl == commander_state_s::MAIN_STATE_RATTITUDE || | |||
main_state_before_rtl == commander_state_s::MAIN_STATE_STAB)) { | |||
|
|||
// transition to previous state if sticks are increased | |||
const float min_stick_change = 0.2f; | |||
// transition to previous state if sticks are touched | |||
if ((_last_sp_man.timestamp != sp_man.timestamp) && | |||
((fabsf(sp_man.x) - fabsf(_last_sp_man.x) > min_stick_change) || |
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.
If I read it correctly this is handled like a switch transition, meaning the value is compared to what it was when the fc last received an RC update. So the actual speed with which you have to move your stick depends on the RC update rate and if you happen to have a high update frequency you might never reach such a step.
I guess RTL is not bothered because you might accidentally overlook an automatic RTL on low battery. But the same would also apply to the land mode. An automatic emergency land because of critial battery could go unnoticed. I know it makes things more complicated but maybe we need a distinction of forced reactions or a way the user can not accidentally switch out of these modes. |
We should do these UX changes one release later. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Is this still wanted? |
I like this as a feature. Seems very sensible. |
Hi @LorenzMeier and @dagar , I've checked through this PR and the only changes proposed here that are not already incorporated into current PX4:master involve If you would like, I could submit a PR with the Let me know if you'd like those remaining changes submitted or how else I can help. |
@mcsauder I recall a discussion within the past few months about how this specific behavior would be inappropriate for Fixed Wing as POSCTRL for fixed wings isn't "just hover" like it is for multicopter. In your review of this old PR, did you see that feature as MC only? |
Hi @Antiheavy , I think your discussion is unrelated to this particular change. For convenience here is a compare of what I *believe is the appropriate application of the remaining intent in this PR: master...mcsauder:mode_switch_automodes I did a quick flight test to see if anything was obviously broken by these changes and things appear to be functioning properly. (They will require some more scrutiny.) |
This adds a few useful changes: