[navigation][flight plan] mandate call_once for functions that should only run once #1967

flixr · 2016-12-14T20:43:47Z

The call primitive of the flight plan can be used to repeatedly call a function until it returns false.
In most cases the function is only supposed to be called only once though...

This has led to a state where a lot of functions that are supposed to be called from the flight plan have an additional return false to make them call-able.

From just looking at the flight plan it is hard to determine if a function will indeed be only called once or is actually called each time the flight plan navigation is run until it is "done" (returns false).
To make this distinction more explicit and hopefully more clear (and the code cleaner), one should use call only when really needed and call_once otherwise.

This PR removes the "dummy" return false from a lot of functions, hence mandating that call_once is used for them. Eventually we shouldn't have any cases like that anymore...
All flight plans were updated accordingly.

The most common case where call still has to be used are navigation routines a la nav_x_run.

This should be "backwards safe" (not backwards compatible) in the sense that if you try use call for a function that needs to be called with call_once (since it now returns void), you will get an "invalid use of void expression" error:

In file included from firmwares/rotorcraft/navigation.c:40:0:
/home/flixr/code/paparazzi/var/aircrafts/Quad_LisaMX/ap/generated/flight_plan.h: In function 'auto_nav':
/home/flixr/code/paparazzi/var/aircrafts/Quad_LisaMX/ap/generated/flight_plan.h:129:9: error: invalid use of void expression
         if (! (NavKillThrottle())) {
         ^

just that they can be used with `call` instead of `call_once`

This was just so that they could be called with `call`, you should use `call_once` instead. Also change some macros to static inline functions for better debugging.

gautierhattenberger · 2016-12-14T23:23:16Z

sw/airborne/modules/nav/nav_survey_poly_rotorcraft.c

@@ -72,7 +72,6 @@ bool nav_survey_poly_setup_towards(uint8_t FirstWP, uint8_t Size, float Sweep, i
  //if values passed, use it.
  if (Size == 0) {Size = Poly_Size;}
  if (Sweep == 0) {Sweep = Poly_Distance;}
-  return nav_survey_poly_setup(FirstWP, Size, Sweep, ang);


Shouldn't this function still be called before the return ?

ups, yes... good catch!

flixr · 2016-12-20T15:06:44Z

What do you guys think about this?

gautierhattenberger · 2016-12-20T16:49:13Z

sw/airborne/modules/nav/nav_survey_poly_rotorcraft.c

@@ -155,7 +155,7 @@ bool nav_survey_poly_setup(uint8_t EntryWP, uint8_t Size, float sw, float Orient
  CSurveyStatus = Init;

  if (Size == 0) {
-    return true;
+    return;


here the behavior is different than before, as previously it would stay stuck in init function while now it is silently failing (at least skipping the end of init)

I updated the run function to return without doing anything if the size is zero...

The OK as for rotorcraft, quick glance fixedwing look OK, Thx. Note that The call_once (or call for that matter ) was discussed quite some time ago. For now to go forward i think it is fine. In future I rather see a nice non tech flightplan descriptive language and users writing flightplans should just be able to write e.g simply <SetGroundReferenceHere"/>. But that is another discussion. Yes I know, one could write a wrapper for nice flighplan language encapsulating current implementation.

BTW as for SurveySize returning if 0, well if I did not arrive at my survey area one could wish to set it's values as moving toward the survey entry point while gathering data to determine the survey or e.g. let it size depend on fuel drain along the way. With this change as far a quickly understood, one now cannot start move towards even. Anyhow if ever needed it that way myself, I'll file a new issue ;)

BTW not testflown and simmed yet, will do, with finger on TX Mode switch ;) when it lnds in master and hopefully not -20 deg C.

gautierhattenberger · 2016-12-20T16:51:17Z

On the general idea, I'm fine with these changes

flixr force-pushed the nav_macro_cleanup branch 2 times, most recently from 157e6fe to 8757f4a Compare December 14, 2016 22:00

flixr requested review from martinmm, gautierhattenberger, dewagter and podhrmic December 14, 2016 22:48

flixr force-pushed the nav_macro_cleanup branch from 8757f4a to a9314a5 Compare December 14, 2016 22:50

flixr added 8 commits December 14, 2016 23:51

[nav] don't return false from nav functions

ca54625

just that they can be used with `call` instead of `call_once`

[rotorcraft][nav] refactor send functions

737a561

[conf] use call_once in rotorcraft_basic flight plan

d49670e

[rotorcraft]move built in nav routines around for clarity

0a3b270

[rotorcraft] move more nav stuff around for clarity

eb07c39

[rotorcraft][nav] remove dummy return false from some macros

6b5708e

This was just so that they could be called with `call`, you should use `call_once` instead. Also change some macros to static inline functions for better debugging.

[conf] update flight_plans to use call_once

995a9ed

allow usage of call_once in procedures

6bbe7d7

flixr force-pushed the nav_macro_cleanup branch from a9314a5 to b8160fa Compare December 14, 2016 22:51

gautierhattenberger reviewed Dec 14, 2016

View reviewed changes

[modules][nav] return void for setup routines

817062d

flixr force-pushed the nav_macro_cleanup branch from b8160fa to 817062d Compare December 14, 2016 23:38

flixr added 2 commits December 16, 2016 15:53

[fixewing][nav] remove return false from some more functions

55addf6

[fixewing] remove DownlinkSendWp

72f0a00

flixr force-pushed the nav_macro_cleanup branch from 6a4295e to 72f0a00 Compare December 16, 2016 15:29

gautierhattenberger reviewed Dec 20, 2016

View reviewed changes

[nav] survey_poly: don't run if SurveySize == 0

34b35f7

OpenUAS approved these changes Dec 20, 2016 •

edited

Loading

View reviewed changes

gautierhattenberger approved these changes Dec 24, 2016

View reviewed changes

gautierhattenberger added this to the v5.12 milestone Dec 24, 2016

gautierhattenberger added the Airborne label Dec 24, 2016

gautierhattenberger added the Flight Plan label Dec 24, 2016

flixr merged commit 206a99f into master Dec 27, 2016

flixr deleted the nav_macro_cleanup branch December 27, 2016 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[navigation][flight plan] mandate call_once for functions that should only run once #1967

[navigation][flight plan] mandate call_once for functions that should only run once #1967

flixr commented Dec 14, 2016 •

edited

Loading

gautierhattenberger Dec 14, 2016

flixr Dec 14, 2016

flixr Dec 14, 2016

flixr commented Dec 20, 2016

gautierhattenberger Dec 20, 2016

flixr Dec 20, 2016

OpenUAS Dec 20, 2016 •

edited

Loading

gautierhattenberger commented Dec 20, 2016

[navigation][flight plan] mandate call_once for functions that should only run once #1967

[navigation][flight plan] mandate call_once for functions that should only run once #1967

Conversation

flixr commented Dec 14, 2016 • edited Loading

gautierhattenberger Dec 14, 2016

Choose a reason for hiding this comment

flixr Dec 14, 2016

Choose a reason for hiding this comment

flixr Dec 14, 2016

Choose a reason for hiding this comment

flixr commented Dec 20, 2016

gautierhattenberger Dec 20, 2016

Choose a reason for hiding this comment

flixr Dec 20, 2016

Choose a reason for hiding this comment

OpenUAS Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

gautierhattenberger commented Dec 20, 2016

flixr commented Dec 14, 2016 •

edited

Loading

OpenUAS Dec 20, 2016 •

edited

Loading