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

Autopilot generated integration #1975

Merged
merged 11 commits into from
Dec 29, 2016
Merged

Autopilot generated integration #1975

merged 11 commits into from
Dec 29, 2016

Conversation

gautierhattenberger
Copy link
Member

support generated autopilot, based on rotorcraft firmware

  • By default the original static autopilot is used. A config flag can enable the use of the generated AP code.
  • A basic autopilot description is provided (4 modes + failsafe modes).
  • The server is capable of using the list of generated mode to properly display mode names.
  • Tested in NAV and GUIDED mode in sim, and direct attitude control on real aircraft.

Some extra features:

  • add --norc option to NPS simulator
  • group all system (generated) settings (AP, telemetry, modules, flight plan) under the same tab (first position)

By default the original static autopilot is used. A config flag can
enable the use of the generated AP code.
A basic autopilot description is provided (4 modes + failsafe modes).
The server is capable of using the list of generated mode to properly
display mode names.
Tested in NAV and GUIDED mode in sim, and direct attitude control on
real aircraft.
@hooperfly
Copy link
Contributor

@gautierhattenberger in the case of autopilot generated modes, will there be a mechanism for finding mode/behavior indexes/offsets? For example, in the current static definition, 13 decimal is NAV and 19 decimal is GUIDED. Given that they are currently static, I use those values when binding behavior via the Flying Robot Commander. What's the plan for obtaining dynamic mode information for binding purposes?

@gautierhattenberger
Copy link
Member Author

For now, you have to get the xml file (from the airframe file) to know the list of modes. This is how I get them for the server. For you FRC, one solution is to do the same, or if you only need a limited number of extra functionalities, we could consider adding them to the system, like changing a mode based on its name rather than its index.

#include "subsystems/commands.h"

/** Set descent speed in failsafe mode */
#ifndef FAILSAFE_DESCENT_SPEED
Copy link
Member

Choose a reason for hiding this comment

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

This should either be set here or in the C files, not on both locations...

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -127,4 +145,25 @@ extern bool guidance_v_set_guided_th(float th);
guidance_v_z_sum_err = 0; \
}

#define GuidanceVSetRef(_pos, _speed, _accel) { \
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed in the header?
I would rather put this and guidance_v_z_enter in the c file...

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, old stuff I forgot there

@flixr
Copy link
Member

flixr commented Dec 20, 2016

Regarding the syntax of the autopilot xml file:

  • The statements in start are separated by |. What is the advantage of this instead of simply using ;?
  • call actually seems to be more equivalent to call_once in the flight plan...
    Maybe it would even make sense to choose a different name here, to make it clear that any statement can be used...

<control freq="32">
<mode name="NAV" start="guidance_h_nav_enter()|stabilization_attitude_enter()|guidance_v_z_enter()">
<select cond="RCMode2() && DLModeNav()" exception="HOME"/>
<control freq="NAV_FREQ">
<call fun="nav_periodic_task()"/>
Copy link
Member

Choose a reason for hiding this comment

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

So in the generated version nav_periodic_task is now only called in NAV mode, whereas in the static version it is always called (except if you are in HOME mode).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just lazy, I can add it again, as long as guidance is not set from nav it's fine

@gautierhattenberger
Copy link
Member Author

gautierhattenberger commented Dec 20, 2016

For the | in init, I guess it was to make it look less "computer science". Also I was hopping to have much less functions. Maybe I could have a special child node start with one call per line.

I don't mind changing call if you have a better name.

@flixr
Copy link
Member

flixr commented Dec 20, 2016

For the | in init, I guess it was to make it look less "computer science". Also I was hopping to have much less functions. Maybe I could have a special child node start with one call per line.

| is IMHO not more readable and has the additional drawback that you can't use that operator in any of the start statements anymore...

I don't mind changing if you have a better name.

Not sure what a good name would be here, but I don't like that we use the same keyword as in the flight plan, but with different behavior.

@flixr
Copy link
Member

flixr commented Dec 20, 2016

Also maybe rename start to on_enter or so?

@hooperfly
Copy link
Contributor

@gautierhattenberger with respect to getting a limited modes set from all modes in an autopilot, the ability to get mode by name seems like a nice feature. We just have to manage the mode name space and keep it consistent for modes across all autopilots. That seems reasonable.

…nterface

It can makes it easier to handle generated autopilot. Names are not
matching exactly, but this can be improved in the future.
An example is provided with gb2ivy program.
@gautierhattenberger
Copy link
Member Author

gautierhattenberger commented Dec 24, 2016

@hooperfly I have added a function to fill the value index based on the 'values' list of a setting. It can makes it a bit easier to access modes. This could be improved in the future by choosing the same name and case for static and generated autopilots.

@gautierhattenberger
Copy link
Member Author

Do you think this is fine to merge ? It should not change anything for regular users at the moment.

@flixr
Copy link
Member

flixr commented Dec 29, 2016

Except for the different semantics of call, I'm happy with this...

@gautierhattenberger
Copy link
Member Author

Maybe the problem is FP naming after all (call -> call_loop).

Otherwise I'm suggesting for autopilots:

  • call -> block (I don't like this one, but it is closer to a "graphical" representation)
  • call -> control and previous control -> controls (why not)
  • call -> call_control (I think I would choose this one)
  • call fun=... -> fun name=... (maybe not very explicit)
  • call -> call_once (coherent, but one might wonder if it means that calling several time is possible)

@flixr
Copy link
Member

flixr commented Dec 29, 2016

Yeah, naming is hard... e.g. call_control is a bit weird for the on_enter functions, not sure what we should do here...
Or we merge it like it is and try to find a clearer/cleaner naming later...?

@gautierhattenberger
Copy link
Member Author

I think the best here is call. And it is not the worst inconsistent naming (the worse being module in settings I think...).
Once the new architecture is stabilized (modules, autopilots,...) we could make a global harmonization.

@flixr flixr merged commit 880187d into master Dec 29, 2016
@flixr flixr deleted the autopilot_generated_integration branch December 29, 2016 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants