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

[ins] convert ins subsystems to modules #1740

Merged
merged 1 commit into from Jun 25, 2016
Merged

[ins] convert ins subsystems to modules #1740

merged 1 commit into from Jun 25, 2016

Conversation

flixr
Copy link
Member

@flixr flixr commented Jun 17, 2016

and get rid of ins calls in main

@flixr
Copy link
Member Author

flixr commented Jun 17, 2016

@gautierhattenberger any good idea how to deal with the vectornav module?
The problem is again that the nps target doesn't have the ins_vectornav_event function...
But making another module just for this nps "implementation" is not so nice...

@kirkscheper
Copy link
Member

can you also update the ins_skeleton ap target to rotorcraft, atm it shouldn't work for fixedwing.

@flixr flixr force-pushed the ins_modules branch 2 times, most recently from babb86a to fd9d88f Compare June 17, 2016 21:29
@gautierhattenberger
Copy link
Member

why not make a ins_nps/sim module and autoload them (add make ins_vectornav only for ap) ?

@flixr
Copy link
Member Author

flixr commented Jun 19, 2016

Well - the combination of substituting an INS with IMU, no AHRS and INS gps_passthrough (and that with the nps bypass flags) is currently only used/needed in vectornav.
So basically I'm wondering if it even makes that much sense to provide an "automatic" INS replacement for the nps/sim targets in cases like vectornav.

And while we have a ins_gps_passthrough module, we only want to load it for the nps target in the case of vectornav, but in general that is also for the ap target... so we can't simply add it to the autoloaded modules...

@flixr
Copy link
Member Author

flixr commented Jun 19, 2016

Basically the question is again if we should either allow

  • to only autoload modules for specific targets
  • or to specify that some functions are only added for specific targets (as IIRC @fvantienen already asked about before)

But we have to be careful to balance added functionality/expressiveness with the increase in complexity. Not sure where the best middle ground is here...

@gautierhattenberger
Copy link
Member

If the only concerned module is vectornav (and actually also xsens, so it seems to be an external ins problem), I suggest to provide a specific wrapper for these modules for nps/sim target.
Adding target specific functions functionality is indeed possible, but maybe it will make configuration more complex in the end.

@flixr
Copy link
Member Author

flixr commented Jun 19, 2016

We'll have the same "problem" in a few modules and it seems to me now that allowing to "restrict" the autoload feature to specific targets could really be used in a lot of cases and make configuration easier in the end.
E.g. in that case we could just add

<autoload name="imu_nps"/>
<autoload name="gps_nps"/>
<autoload name="ins_gps_passthrough" target="nps"/>

although that still wouldn't set the NPS_BYPASS_x defines for the nps target..

@podhrmic
Copy link
Member

I agree here with @gautierhattenberger - making a specific "dummy" module for vectornav/xsens only seems to be the easiest. Btw how is currently Xsens handled in NPS?

@flixr
Copy link
Member Author

flixr commented Jun 19, 2016

see https://github.com/paparazzi/paparazzi/blob/4531e5767afcf428c0c8a78781ca3ee9ca414a4c/conf/modules/imu_xsens.xml
But that is not possible any more as soon as you have init/periodic/event functions that are specific to a target...

@gautierhattenberger
Copy link
Member

unless the dummy functions are actually calling the nps specific functions ?

@flixr
Copy link
Member Author

flixr commented Jun 20, 2016

dummy functions?

@flixr
Copy link
Member Author

flixr commented Jun 21, 2016

As soon as autoload is recursive, this should be fine...

@flixr flixr force-pushed the ins_modules branch 4 times, most recently from d505e13 to c83f7ee Compare June 21, 2016 18:00
@flixr
Copy link
Member Author

flixr commented Jun 21, 2016

So I directly added all module that should be autoloaded for now... can be simplified as soon as we have recursive autoloading...

Remaining problem is #1759

and get rid of ins calls in main and register functions
@flixr
Copy link
Member Author

flixr commented Jun 23, 2016

Done.

@gautierhattenberger
Copy link
Member

looks good for me

@flixr flixr merged commit b97e23b into master Jun 25, 2016
@flixr flixr deleted the ins_modules branch June 25, 2016 15:45
@flixr flixr mentioned this pull request Sep 20, 2016
12 tasks
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

4 participants