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

convert GPS subsystems to modules #1625

Merged
merged 11 commits into from
Jun 17, 2016
Merged

convert GPS subsystems to modules #1625

merged 11 commits into from
Jun 17, 2016

Conversation

flixr
Copy link
Member

@flixr flixr commented Apr 21, 2016

Converting the GPS subsystems should have the following benefits:

  • get rid of need for GPS registering (was introduced/needed for multi GPS support)
  • call init/event functions of each implementation from module instead of dispatcher in main
  • better documentation for each module (including configure/define options)
  • potentially add implementations specific settings that are autoloaded (gps reset?)

PRIMARY_GPS (and SECONDARY_GPS) now needs to be UPPERCASE (the first part of the ABI id, e.g. GPS_UBX)
This is now only used/needed to resolve GpsId(PRIMARY_GPS) to e.g. GPS_UBX_ID

Also:

  • remove obsolete/non-functional gps_ublox_hitl and gps_ardrone2

Next steps?:

  • make gps_sim and gps_nps modules and remove those targets from the "normal" modules
  • add the sim modules as dependencies so they can be auto-loaded with the new autoload feature?
  • call the gps_periodic_check from the modules for each implementation
  • get rid of gps_init in main?

Feedback appreciated!

@gautierhattenberger
Copy link
Member

This is definitely the way to go I think. I'll help on this when I have some time.

@gautierhattenberger
Copy link
Member

@flixr After some thoughts, I don't see how to make it work with a dependency. Because, if the initial module is unloaded for a target (like gps_ubx for other targets than ap), all its dependencies will be unloaded as well.
The initial design of modules was to implement target specific function for each modules. Something like gps_ubx_sim.c and gps_ubx_nps.c that implements functions from gps_ubx.h (even if they are actually calling functions from a generic library like gps_sim.[ch]).
This would make a lot of (annoying) extra files. Do you think this solution is acceptable ? Do you see other options ?

@flixr
Copy link
Member Author

flixr commented May 18, 2016

Hmm... that is true. But I really don't want to add additional files for simulation for each GPS implementation. We would need the same for IMU, AHRS, etc. as well...
I would rather require to add the gps modules for sim and nps targets in the airframe file explicitly, but that is also not that nice...

In the end I would like to have some required modules for each firmware (like stabilization, control, etc...) but GPS is not required...

@flixr
Copy link
Member Author

flixr commented May 18, 2016

@gautierhattenberger how about adding something like a autoload node to the module xml where you can specify a list of additional modules to load?
Similar to the depends but which only adds the module as if it had been explicitly specified in the airframe file (so without unloading the dependencies if the module itself is unloaded due to target/firmare mismatch).

@gautierhattenberger
Copy link
Member

I add a similar idea that would be to add alternate list to the modules' dependency, so that if a module can't be loaded, it will try the other ones until one is working, or fail. What do you think ? Not sure it is exactly the same than your autoload.

@flixr
Copy link
Member Author

flixr commented May 18, 2016

I think the autoload method is a bit more generic and easier to understand... and at least in the use cases I can think of now, there would be no additional benefit for an alternate method like you described.

@gautierhattenberger
Copy link
Member

Ok, I'll let my colleague know about this. Not sure what he did so far.

and remove obsolete/non-functional gps_ublox_hitl and gps_ardrone2

GPS functions still need to be called from main...
and get rid of need for GPS registering.

PRIMARY_GPS (and SECONDARY_GPS) now needs to be UPPERCASE (the first part of the ABI id, e.g. GPS_UBX)
This is now only used/needed to resolve GpsId(PRIMARY_GPS) to e.g. GPS_UBX_ID

sim and nps are now failing, since the init and event functions are wrong for those targets.
Next step is to make gps_sim and gps_nps modules and remove those targets from the "normal" modules.
But maybe add the sim modules as dependencies so they can be auto-loaded?
from the "normal" gps modules so that you don't have to add them manually in your airframe to get the simulation running
@flixr
Copy link
Member Author

flixr commented Jun 15, 2016

Updated to autoload the gps modules for the simulators...

Regarding getting rid of explicitly calling gps_init in main:
We don't want to run it multiple times if we have more than one GPS (we want to bind the ABI message only once and call the register_periodic_telemetry also only once).
Should we add a static variable in gps.c that remembers if gps_init has already been called or does any one have a better idea?

@gautierhattenberger
Copy link
Member

One option would be to have a common gps module autoloaded from each gps implementation. At the end, modules are singletonized, so it should work.
This could eventually be converted to a regular dependency when this will be ready.

this should be added to each GPS implementation separately (where appropriate)
@flixr
Copy link
Member Author

flixr commented Jun 15, 2016

Wanted to make a "generic" gps module that basically just calls gps_init, autoloads the sim modules and has a setting for the multi_gps_mode.
But it seems that recursive autoloading is not working (gps_x autoloads gps which autoloads gps_sim/nps).

@gautierhattenberger
Copy link
Member

true, autoloaded modules are not parsed for their own autoloaded stuff. I'll see if I can make it recursive. But I'm not sure it is what should be done here. It would mean that any gps implementation have to rely on gps_sim/nps modules for simulation (which is the case now) and can't provide their own sim/nps implementation (for whatever reasons).
Actually, I think generic gps should be a dependency (when available) since you don't what it to be loaded if no gps is needed for a given target, while gps_sim/nps should be autoloaded (so we try to load them even if parent module is unloaded).

@flixr
Copy link
Member Author

flixr commented Jun 16, 2016

Good point.
But making it autoload the gps module now as dependencies are not autoloaded yet.

- remove gps_init from main
- all actual gps_x implementations autoload the generic gps module
- generic GPS module has setting for multi_gps_mode
@flixr
Copy link
Member Author

flixr commented Jun 16, 2016

Done from my point of view...

@flixr
Copy link
Member Author

flixr commented Jun 16, 2016

@gautierhattenberger @dewagter @kirkscheper or anyone else got any feedback/questions here before we merge this?
Did I miss something or does it look good?

@gautierhattenberger
Copy link
Member

It's all good for me

@kirkscheper
Copy link
Member

What about gps_datalink and gps_optitrack makefiles?

@flixr
Copy link
Member Author

flixr commented Jun 17, 2016

@kirkscheper thx, forgot about that one... done.

@gautierhattenberger gautierhattenberger merged commit a55e112 into master Jun 17, 2016
@gautierhattenberger gautierhattenberger deleted the gps_modules branch June 17, 2016 13:15
@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

3 participants