Skip to content
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

backtransition when taking over manual control #20620

Closed
wants to merge 1 commit into from

Conversation

ThomasRigi
Copy link
Member

From https://discord.com/channels/1022170275984457759/1039594203149238332 @MaEtUgR @sfuhrer

Describe problem solved by this pull request

With #17404, a change was introduced in what happens when you take over manually with the RC from an automatic flight mode. Previously, the transition switch would be checked and the drone would transition accordingly if needed. Now the transition switch is ignored. Meaning for example that if the drone is flying in FW mission mode and you take over in Position mode with the transition switch already in MC mode, the drone will continue to fly in FW mode, and not according to the switches on the RC.

For some use cases (including ours, obviously), this is undesired. If we have to take over manually, it's because of a emergency situation, in which we want to switch to MC asap. (Though for most use cases the new functioning is arguably better.)

Describe your solution

When a switch to a manual flight mode is detected, a back transition is started immediately.

Describe possible alternatives

Don't merge this if it's not deemed better.

Test data / coverage

I forgot to activate permanent logging when bench testing. I will update the descriptions with logs once I get back to my test bench.

@ThomasRigi ThomasRigi marked this pull request as draft November 16, 2022 16:26
@ThomasRigi
Copy link
Member Author

In its current state the basic functionality of enforcing the back transition when switching to a manual mode works.

Before it's ready to be merged I still have to add a parameter that allows to enable/disable this new feature.

@sfuhrer
Copy link
Contributor

sfuhrer commented Nov 23, 2022

I see why you would want to keep the old behavior (you're used to it and have set up safety processes around how it used to work before). But in general it's not nice to "add alternative ways to achieve the same", and if possible we should try to keep these settings to a minimum. Thinking about test-ability, code compactness, documentation, .. .
Additionally I would not like to have no way to take over a VTOL in FW mode without first switching to MC, as it would be the case with this new setting activated.

Would it be okay for you to keep this customization on your privat fork of PX4 if you want to keep it?

@ThomasRigi
Copy link
Member Author

From what I understood, @MaEtUgR also wanted this feature for a client of yours.

I've already merged it to our private fork (with a parameter enabling/disabling the forced backtransition). If you think it's not useful in upstream I'm perfectly fine with closing this PR. If @MaEtUgR still wants it I can add the parameter also to this PR.

@MaEtUgR
Copy link
Member

MaEtUgR commented Nov 28, 2022

@ThomasRigi Thanks a lot for the PR! Again sorry that my change interfered with the operations you were used to. That's exactly what I meant with it should be handled explicitly rather than relying on the RC switch position as a system state. There were indeed discussions specifically about how to prevent an end-user of a product from flying a VTOL manually as a fixed-wing at all. Your suggestion would achieve this but I have to say these discussions are not finished and I don't know if it's the desired behavior. Maybe it will just get constrained by the UI to a certain user group maybe it will not be enforced at all. I agree with @sfuhrer's maintainability point particularly since I'm not (yet) maintaining much of the VTOL part. If it's ok I'd close the pr for now and would reopen it again in case the requirement comes up. Thanks again for your consideration.

@MaEtUgR MaEtUgR closed this Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants