Plane: Move navigation functions into flight mode classes. #14959

samuelctabor · 2020-07-31T11:10:58Z

After discussing with @magicrub this should be a step in the right direction. I'm planning to flight test this soon.

magicrub

I wrote up some comments about not using the indirect _navigate() but after seeing that copter does a very similar thing with run_autopilot() I'm ok with this as-is. Wonderful abstraction, thanks for the help @samuelctabor on the onion v2 !!!

magicrub · 2020-07-31T17:15:41Z

This needs a rebase but otherwise is a straightforward refactor. Sadly, refactors like this don't last long in master without a conflict so lets take a look at this one again after the rebase and get it in. It looks like a low-risk refactor, no behavior change.

samuelctabor · 2020-07-31T19:30:27Z

Good stuff, I will rebase tomorrow. All tests pass on my local branch.

peterbarker

This is a nice cleanup.

That fallthrough-to-loiter-from-RTL was kinda nasty, and narrowing the scope of radius a qrtl_radius is cool.

Same as @magicrub I noted the indirect navigate() calling _navigate() - but it seems to be a trend in Plane :-)

peterbarker · 2020-07-31T23:16:00Z

ArduPlane/mode_avoidADSB.cpp

@@ -11,3 +11,8 @@ void ModeAvoidADSB::update()
    plane.mode_guided.update();
 }

+void ModeAvoidADSB::_navigate()
+{
+    plane.mode_loiter.navigate();


I'm not sure these things should be calling into mode_loiter to do stuff. They could call update_loiter themselves.

On the Copter side we've often factored out the functionality - land_run_vertical / land_run_horizontal, for example. That's not universal - for example, mode-auto uses mode-rtl for RTL'ing and mode-guided for guided'ing....

It's a minor point and doesn't need to be addressed, really.

peterbarker · 2020-07-31T23:19:28Z

ArduPlane/mode_rtl.cpp

+         */
+        plane.set_mode(plane.mode_qrtl, ModeReason::UNKNOWN);
+        return;
+    } else if (plane.g.rtl_autoland == 1 &&


Don't need the else here.

samuelctabor · 2020-08-01T16:43:27Z

Rebased and removed the extra "else".

samuelctabor · 2020-08-07T08:39:04Z

Any other changes required before this can be merged or taken to the dev call?

peterbarker · 2020-08-07T12:13:02Z

@samuelctabor fine as far as I can see. Needs to be merged by a Plane maintainer IMO, 'though - and that's not me :-)

I'll mark it DevCall so you have a deadline at least.

WickedShell · 2020-08-10T20:39:57Z

ArduPlane/ArduPlane.cpp

-        // nothing to do
-        break;
-    }
+    control_mode->navigate();


Why retain this wrapper at all? It's only called once from Plane::navigate so this is adding an indirection that isn't needed, and makes it harder to follow?

I agree, it is clearer without it

rmackay9 · 2020-08-10T23:32:44Z

As discussed on the dev call, @magicrub and @tridge will do a detailed inspection of this before merging.

tridge · 2020-08-11T00:54:31Z

ArduPlane/mode_avoidADSB.cpp

@@ -11,3 +11,8 @@ void ModeAvoidADSB::update()
    plane.mode_guided.update();
 }

+void ModeAvoidADSB::_navigate()
+{
+    plane.mode_loiter.navigate();


I'd like them to call plane.update_loiter() instead. The problem with calling into mode_loiter is it means we're calling a member function on a class wirthout ever calling the init function on the class. Right now this is safe, but if someone changes mode_loiter it could break other modes.
In general I don't like calls into non-static functions on a mode class without the class enter function having been called

yes, I totally agree with Tridge's comment.

Changed to call into plane.update_loiter().

tridge · 2020-08-11T00:55:38Z

I've checked for copy&paste errors and it looks good. I'd like someone to test all affected flight modes to look for correct behaviour in a manual SITL run

samuelctabor · 2020-08-12T22:07:02Z

Checked:

correct circle radius and location in takeoff, guided and loiter
auto mission works
flying around in cruise works
RTL works correctly with various RTL_AUTOLAND settings

ToDo:

having some trouble with avoid ADSB, I'll check with Tom tomorrow
RTL with quadplane

samuelctabor · 2020-08-13T11:56:46Z

RTL with quadplane works correctly
ADSB avoidance has other issues but this PR doesn't change them

samuelctabor · 2020-08-18T09:14:10Z

Perhaps we could discuss this on the EU dev call?

rmackay9 · 2020-08-18T11:25:46Z

Added the DevCallEU at @samuelctabor's request.

tridge

just remove the extra level of indirection, so plane.update_loiter() is called directly by the mode navigate functions, then can be merged when passes CI

…ate().

peterbarker · 2020-08-19T23:26:45Z

Rebased this on top of CI problem fixes.

samuelctabor · 2020-08-19T23:32:03Z

Thanks Peter, I did the same a few seconds later before I saw your comment :-)

peterbarker · 2020-08-20T03:02:24Z

Merged, thanks!

magicrub approved these changes Jul 31, 2020

View reviewed changes

peterbarker approved these changes Jul 31, 2020

View reviewed changes

samuelctabor force-pushed the pr/navigation-flight-modes branch from 8bf9258 to 598c4b1 Compare August 1, 2020 15:52

peterbarker added the DevCallTopic label Aug 7, 2020

WickedShell requested changes Aug 10, 2020

View reviewed changes

tridge self-requested a review August 10, 2020 23:25

CraigElder removed the DevCallTopic label Aug 10, 2020

tridge requested changes Aug 11, 2020

View reviewed changes

rmackay9 added the DevCallEU label Aug 18, 2020

tridge requested changes Aug 19, 2020

View reviewed changes

samuelctabor force-pushed the pr/navigation-flight-modes branch 2 times, most recently from b30793f to e795a7f Compare August 19, 2020 09:22

peterbarker force-pushed the pr/navigation-flight-modes branch from e795a7f to 9bced62 Compare August 19, 2020 23:26

samuelctabor added 4 commits August 20, 2020 00:26

Plane: Move navigation functions into flight mode classes.

c0e9dac

Plane: Call plane.update_loiter() rather than plane.mode_loiter.navig…

1edf0e0

…ate().

Plane: Remove _navigate indirection in Mode.

f30b057

Plane: Remove update_navigation() method.

a300997

samuelctabor force-pushed the pr/navigation-flight-modes branch from 9bced62 to a300997 Compare August 19, 2020 23:27

peterbarker merged commit 334fd4a into ArduPilot:master Aug 20, 2020

samuelctabor deleted the pr/navigation-flight-modes branch August 20, 2020 12:44

andyp1per removed the DevCallEU label Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plane: Move navigation functions into flight mode classes. #14959

Plane: Move navigation functions into flight mode classes. #14959

samuelctabor commented Jul 31, 2020

magicrub left a comment

magicrub commented Jul 31, 2020

samuelctabor commented Jul 31, 2020

peterbarker left a comment

peterbarker Jul 31, 2020

peterbarker Jul 31, 2020

samuelctabor Aug 1, 2020

samuelctabor commented Aug 1, 2020

samuelctabor commented Aug 7, 2020

peterbarker commented Aug 7, 2020

WickedShell Aug 10, 2020

tridge Aug 19, 2020

rmackay9 commented Aug 10, 2020

tridge Aug 11, 2020

rmackay9 Aug 11, 2020

samuelctabor Aug 12, 2020

tridge commented Aug 11, 2020

samuelctabor commented Aug 12, 2020

samuelctabor commented Aug 13, 2020

samuelctabor commented Aug 18, 2020

rmackay9 commented Aug 18, 2020

tridge left a comment

peterbarker commented Aug 19, 2020

samuelctabor commented Aug 19, 2020

peterbarker commented Aug 20, 2020

Plane: Move navigation functions into flight mode classes. #14959

Plane: Move navigation functions into flight mode classes. #14959

Conversation

samuelctabor commented Jul 31, 2020

magicrub left a comment

Choose a reason for hiding this comment

magicrub commented Jul 31, 2020

samuelctabor commented Jul 31, 2020

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelctabor commented Aug 1, 2020

samuelctabor commented Aug 7, 2020

peterbarker commented Aug 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmackay9 commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tridge commented Aug 11, 2020

samuelctabor commented Aug 12, 2020

samuelctabor commented Aug 13, 2020

samuelctabor commented Aug 18, 2020

rmackay9 commented Aug 18, 2020

tridge left a comment

Choose a reason for hiding this comment

peterbarker commented Aug 19, 2020

samuelctabor commented Aug 19, 2020

peterbarker commented Aug 20, 2020