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

AP_Mission: Relative Mission (translation, rotation, altitude-adaption) #14100

Closed
wants to merge 12 commits into from

Conversation

WillyZehnder
Copy link
Contributor

@WillyZehnder WillyZehnder commented Apr 15, 2020

Translation of a defined Mission at start or restart of AUTO-mode
related to the Homepoint and the Location where AUTO has been switched on.
Different translation-modes are selectable via MIS_Parameters.

  • parallel translation
  • additional rotation
  • additional adaption of altitude
  • ignore necessity of achieving altitude target at first Waypoint
    2020-04-13 18_58_09-parallel
    2020-04-13 19_04_47-rotated

Translation of a defined Mission at start or restart of AUTO-mode.
Related to the Homepoint.
Different translation-modes are selectable via MIS_Parameters.
- parallel translation
- additional rotation
- additional adaption of altitude
- ignoration of achieving of altitude at first Waypoint
@khancyr
Copy link
Contributor

khancyr commented Apr 15, 2020

whaaa, that looks interesting! But that is a big change in the mission library... It will be long to merge.

sqrt() replaced by sqrtf() to conform with DO_NOT_USE_DOUBLE_MATHS()
to conform with DO_NOT_USE_DOUBLE_MATHS()
@WillyZehnder
Copy link
Contributor Author

That's my first contribution, so a little advice and support how and where to find the root cause of the failing checks would be very welcome.

Literal "Bit" in @bitmask:... deleted
@WillyZehnder WillyZehnder marked this pull request as ready for review April 15, 2020 21:04
Copy link
Contributor

@WickedShell WickedShell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The translation math is incorrect. Past that this is invasive enough and seems to be enough of a special case that I'd rather see us support this via scripting if that's okay with you? We have a PR open that will hopefully be in soon which would let you read/write the mission items from a script, and you could do all the translation effort that is appropriate for your use case without having to make this invasive a change in AP_Mission.

}
else {
if (restart_behaviour >= Restart_Behaviour::RESTART_PARALLEL_TRANSLATED){ // do at least parallel translation
cmd.content.location.lat += _translation.lat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not accurately translating anything. You can't simply add lat/long pairs, you actually have to project it down a bearing at a distance. It probably looks correct at a small scale, but it will slowly be warping the mission.

@WillyZehnder
Copy link
Contributor Author

WillyZehnder commented Apr 18, 2020

The translation math is accepting a positioning error of <0.02% at 45 and <0.09% at 80 degrees of latitude if the mission is translated one Km north or south. In normal visual flight, with definition of the Start-Position in flight by the vehicle, that should be acceptable.
And yes, I'm aware that in other cases the scaling of longitude is necessary (see lines 1700+1708).
But I agree, that if we will make that feature waterproof and all-purpose safe, I should take account of that.
I think the change in AP_Mission is not so invasive, because (except of two small structures) I used still existing data-structures and methodes. It's just a very little geometry and logic in two main-blocks.

In the past I asked for a feature like that here: #1611 (comment) but nobody understood my aim, I guess - so I did it by myself.
The result now is a very handy and easy to use feature and I would prefer that it will be merged in the master. Maybe we can discuss in the forum, if such a feature is needed at all.

If the solution by script is as smart and easy to handle as my solution, it might be a better way. But that will decide the experienced developers.

@WillyZehnder
Copy link
Contributor Author

@WickedShell
I had a look at the scripting abilities of AP and I have the impression, that there has been a really great progress in the last months. That looks really very interresting.
But at a deeper look, I realized that with all of my FCs that is not possible. I have some OmnibusNanoV6 and especially MatekF405-Wing FCs, that are very common for sailplanes here. Both of them has just 1 MB of Flash.
So unfortunately a solution via lua-script is not an option for me.

@lvale lvale requested a review from tridge April 30, 2020 14:02
@davidbuzz
Copy link
Collaborator

this is the open 'mission' PR to enable scripting to edit missions arbitrarily.
#13685

@rmackay9
Copy link
Contributor

rmackay9 commented May 5, 2020

Thanks very much for the effort put into this. As discussed on the dev call we're unfortunately not going to be able to merge this because we are worried about the increased complexity and ongoing support.

@tridge
Copy link
Contributor

tridge commented May 5, 2020

this is too complex in its current form, it makes the core mission code a lot more complex and uses quite a lot of flash space.
This would need to be separated out cleanly, in a separate cpp file, with conditional compilation based on a hwdef.dat define

@auturgy
Copy link
Contributor

auturgy commented May 6, 2020

Linking related issue: #1428

Rollback of all changes in AP_Mission.cpp and AP_Mission.h for separating the code in extra files and making the feature optional - as requested by Tridge
Rollback of all changes in AP_Mission.cpp and AP_Mission.h for separating the code in extra files and making the feature optional as requested by Tridge - now with updated submodules
@WillyZehnder
Copy link
Contributor Author

I fought against Rebase - and it seems I lost...

@WillyZehnder WillyZehnder deleted the MissionRelative branch May 7, 2020 15:59
@rishabsingh3003
Copy link
Contributor

@WillyZehnder let me know if you need help in rebasing. Can help you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants