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

Baro event handling using ABI interface #525

Closed
wants to merge 61 commits into from
Closed

Conversation

@gautierhattenberger
Copy link
Member

gautierhattenberger commented Sep 2, 2013

Main benefits:

  • less crappy handler functions in main.c files
  • possibility to use multiple sources of sensors (integrated to the board or not)
  • the filters don't need to know who is sending the raw values (not completely true with the old alt_float filter yet)
  • the pressure are now standardized in Pascal and the standard atmosphere model is used
    • The INS_BARO_SENS is hence not needed anymore.
  • onboard baros are automatically available in fixedwing firmware as well (use same code as rotorcraft now)
    • disable onboard baro with <configure name="USE_BARO_BOARD" value="FALSE"/>

Some stuff to do before using: find the default conversion factor to convert ADC values to Pascal (mostly older boards/baro, recent digital sensors can output pressure in Pascal already)

Baro sensors/drivers that are checked to output pressure in Pascal:

  • bmp085
  • ms5611
  • mpl3115
  • Umarim analog (MPXA6115 pressure sensor + ADS1114 16bit ADC)
  • Booz analog
  • Lisa/L analog
  • pbn (pressure board navarro, not currently used)
  • amsys
  • ets
  • ms5534
  • hca
  • scp
gautierhattenberger and others added 18 commits Aug 22, 2013
#define PPRZ_ISA_GRAVITY 9.80665
#define PPRZ_ISA_AIR_GAS_CONSTANT (8.31447/0.0289644)

#define PPRZ_ISA_M_OF_P_CONST (PPRZ_ISA_AIR_GAS_CONSTANT*PPRZ_ISA_SEA_LEVEL_TEMP/PPRZ_ISA_GRAVITY)

This comment has been minimized.

Copy link
@flixr

flixr Sep 2, 2013

Member

would probably make sense to make this a real static const, so that this value does not need to be computed every time...

This comment has been minimized.

Copy link
@gautierhattenberger

gautierhattenberger Sep 4, 2013

Author Member

I will change it

@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 3, 2013

I guess with ABI we can remove the subsystems/sensors/baro_ms5611_x files and use the modules files for those boards with that sensor instead as the baro struct is not needed anymore...

What about the case where you have two baros? And you e.g. want to actually use only one of them for the altitude estimation in the INS, but still want the qfe and altitude measurements of the other one for reference?
I think it would make a lot of sense to have generic functions/wrappers for a pressure sensor which provides filtering (optionally), setting offset (QFE) and conversion to altitude.

Also it probably doesn't make much sense anymore to compute any offset/altitude in the modules (like in e.g. the ets or ms5611 drivers)? Or is that still needed for the use case mentioned above?

What was your idea with air_data? It currently only holds the data to downlink BARO_RAW message, otherwise totally unused. The INS is directly updated from the ABI BARO_ABS message in a callback.

About ABI in general:

  • as mentioned before... still not a big fan of the ABI IDs distributed over all files... you have to manually check all files that you don't assign the same ID again.
  • since ABI is based on callbacks and these are usually called from the event loop, we have to be careful about what we run there... especially in light of the timing problems that @podhrmic brought into focus again. Did you plan on going through the air_data interface there and then handling this from periodic tasks?
@gautierhattenberger

This comment has been minimized.

Copy link
Member Author

gautierhattenberger commented Sep 4, 2013

I guess with ABI we can remove the subsystems/sensors/baro_ms5611_x files and use the 
modules files for   those boards with that sensor instead as the baro struct 
is not needed anymore...

I guess you can do that, but you will have to change the airframe files to load the module then

What about the case where you have two baros? And you e.g. want to actually use 
only one of them for the altitude estimation in the INS, but still want the qfe 
and altitude measurements of the other one for reference?

If the INS wants to use only one source for altitude, you have to set the correct sender id in the INS bind function (default is broadcast, so all values are used). What do you mean by reference ? Do you want the INS to have these information or only the operator ? If it is the operator, you can send the pressure/alt in telemetry messages from your second sensor. If it as to be used by the INS, when you need to improve the current implementation to do that (by filtering the incoming values based on the sender ID, or by making a second binding specific to the QFE computation for instance).

I think it would make a lot of sense to have generic functions/wrappers 
for a pressure sensor which provides filtering (optionally), setting offset 
(QFE) and conversion to altitude.

The conversion function between pressure and altitude are already in math/pprz_isa.h and we can add some more if needed. Concerning the general filters, when could be added to the filter folder, but this is not only related to the baro implementation since all "big" filters (AHRS, INS) could benefit to have a library of "small" filters (average, std, low/high pass...).

Also it probably doesn't make much sense anymore to compute any offset/altitude
in the modules (like in e.g. the ets or ms5611 drivers)? Or is that still needed for the
use case mentioned above?

This pull request only adapt the existing baro drivers to the ABI interface. Cleaning the modules from all the unneeded stuff (e.g. alt computation) can be done in an other step since it has no impact on the system. The difficult task is to find the correct default sensitivity and offset so that the values received by the INS are in Pa. It is easy with recent digital sensor, but not with old analog+adc+ampli implementation. The worst of all being the booz baro with its variable offset.

What was your idea with air_data? It currently only holds the data to downlink 
BARO_RAW message, otherwise totally unused. The INS is directly updated from 
the ABI BARO_ABS message in a callback.

The BARO_RAW needs a "central" value anyway. Also, the implementation of air_data is not finished. Currently the sensors providing airspeed or angle of attack are directly updating the state interface, which means that if you have multiple sources, they will erase each other. The idea is that these sensors will send ABI messages for which air data have some bindings. The selection/filtering of the sensor values will be done in air data subsystem that will finally update the state interface. It is then similar to the INS or AHRS receiving sensor data and output a single value for the rest of the system.

since ABI is based on callbacks and these are usually called from the event loop,
we have to be careful about what we run there... especially in light of the timing 
problems that @podhrmic brought into focus again. Did you plan on going through
the air_data interface there and then handling this from periodic tasks?

For now, it is not different from what we have now. The event function are doing the same, it is just easier to have more handlers. So of course, if you add to many handlers, you may have performance issues. But at least is not under interrupts... What do you mean by going through air data ? If this subsystem needs complex filtering in the future, we will have to think what should be done in the handler and what should be done in the periodic call. It is not different than AHRS/INS.

as mentioned before... still not a big fan of the ABI IDs distributed over all files... 
you have to manually check all files that you don't assign the same ID again.

I see only three options for now:

  • no sender ID, then a binding will receive the values from all sources, which can be annoying
  • an integer ID (current implementation), which allow a light implementation and a fast processing, but requires to have unique IDs per sender for a given message (similar to i2c address, and maybe also component ID in Mavlink ?)
  • a string ID, which make easier to have unique IDs (just the name of the sensor/system) but is slower to process (most of the time, broadcast ID will be enough so checking a name against an empty string should be fast)
@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 4, 2013

I guess with ABI we can remove the subsystems/sensors/baro_ms5611_x files and use the
modules files for those boards with that sensor instead as the baro struct
is not needed anymore...

I guess you can do that, but you will have to change the airframe files to load the module then

While this is not what I meant, it would be of course also be an option.
I meant that the baro "board" drivers using the ms5611 (e.g. on lisa/m with aspirin2.2) could use the source files from modules/sensors/ instead of the ones in subsystems/sensors as for there is not need for the baro struct anymore.
In any case the subsystems/sensors/baro_ms5611 either still need to be updated (baro struct removed, etc...) or the source files from modules taken instead.

If the INS wants to use only one source for altitude, you have to set the correct sender id in the INS bind function (default is broadcast, so all values are used). What do you mean by reference ? Do you want the INS to have these information or only the operator ? If it is the operator, you can send the pressure/alt in telemetry messages from your second sensor.

I mean for the operator, e.g. you might want to compare a second baro to the onboard baro, or not use the baro in the INS, but only get the alt from it for comparison.
I don't think it makes sense to then do the qfe and alt calculation in each baro module separately again. We also don't want to add this information to the raw baro messages (e.g. BARO_MS5611)

The BARO_RAW needs a "central" value anyway.

I'm not convinced that it even makes sense to keep the BARO_RAW message... If you need raw data rather use the baro specific messages... probably much more usefull to add a new baro message that contains pressure, qfe and alt (possibly also an id if you want that info for multiple baros).

For now, it is not different from what we have now. The event function are doing the same, it is just easier to have more handlers. So of course, if you add to many handlers, you may have performance issues. But at least is not under interrupts...

Yes, it is not different from now... was more thinking about how the design of ABI might impact flexibility in this regard in the future.

What do you mean by going through air data ?

Not directly subscribing to BARO_ABS in the INS, but rather only in air_data. Then maybe have the option to prefilter there, and use that value in the INS. I guess this way we could also do the update in a periodic call if we wanted (i.e. just copying the pressure in air_data and setting a flag that the INS would check). So I think the possible problem mentioned above is not really an issue....

About the ABI IDs, they only need to be unique for a specific ABI message, right?

@gautierhattenberger

This comment has been minimized.

Copy link
Member Author

gautierhattenberger commented Sep 4, 2013

While this is not what I meant, it would be of course also be an option.
I meant that the baro "board" drivers using the ms5611 (e.g. on lisa/m
with aspirin2.2) could use the source files from modules/sensors/
instead of the ones in subsystems/sensors as for there is not need for
the baro struct anymore.

It looks like the code is almost the same, except for the function
names. So it might be a problem, it is not nice to have generic names in
modules (like baro_event) while it is needed to use something like the
baro interface (which is roughly just calling generic
init/periodic/event function).
Why the module is using 2 periodic functions while the subsystem has
only one ?

In any case the subsystems/sensors/baro_ms5611 either still need to be
updated (baro struct removed, etc...) or the source files from modules
taken instead.

Sorry, I forgot those two...

I mean for the operator, e.g. you might want to compare a second baro
to the onboard baro, or not use the baro in the INS, but only get the
alt from it for comparison.
I don't think it makes sense to then do the qfe and alt calculation in
each baro module separately again. We also don't want to add this
information to the raw baro messages (e.g. BARO_MS5611)

Modules should be kept as simple as possible: get a raw value, convert
it to pressure and send it with ABI and telemetry with a custom message
if needed (so it is possible to compare several baro value.

I'm not convinced that it even makes sense to keep the BARO_RAW
message... If you need raw data rather use the baro specific
messages... probably much more usefull to add a new baro message that
contains pressure, qfe and alt (possibly also an id if you want that
info for multiple baros).

I agree that BARO_RAW in its current state is not that useful. If
air_data is kept, it would be nice to have a message for it sending all
its values, including baro value.

Yes, it is not different from now... was more thinking about how the
design of ABI might impact flexibility in this regard in the future.

Even if it is not the best solution, it looks a bit more flexible to me
with ABI than with the current state.
What kind of impact do you see here ? To many bindings with big handlers ?

Not directly subscribing to BARO_ABS in the INS, but rather only in
air_data. Then maybe have the option to prefilter there, and use that
value in the INS. I guess this way we could also do the update in a
periodic call if we wanted (i.e. just copying the pressure in air_data
and setting a flag that the INS would check). So I think the possible
problem mentioned above is not really an issue....

If possible, I would prefer to have INS independent of air_data, but
make some nicer tools and generic filters that could be used as
libraries by the INS/AHRS. The idea is to avoid to rely on global
variables as much as possible.

About the ABI IDs, they only need to be unique for a specific ABI
message, right?

Yes


Reply to this email directly or view it on GitHub
#525 (comment).

@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 4, 2013

It looks like the code is almost the same, except for the function
names. So it might be a problem, it is not nice to have generic names in
modules (like baro_event) while it is needed to use something like the
baro interface (which is roughly just calling generic
init/periodic/event function).

The module event fuction is called baro_ms5611_event... so that should be fine, right?

Why the module is using 2 periodic functions while the subsystem has
only one ?

The subsystem also calls both in the convenience periodic function... The read function triggers a new read and the periodic_check ensures that you wait long enough for a conversion to be made. Ideally to get a minimum delay you would call the read at your desired freq and the periodic check faster (<100Hz) to not delay longer than needed.
See also http://paparazzi.github.io/docs/latest/ms5611__i2c_8c.html#a45123afce6b944dbe03552035e6b6e5e
This was easy to do in the module, but for the "board" baro subsystem you have only one periodic call...

Modules should be kept as simple as possible: get a raw value, convert
it to pressure and send it with ABI and telemetry with a custom message
if needed (so it is possible to compare several baro value.

Yes, they should not do more than read the pressure/temp and send it via ABI.
My point is that you can only send/compare raw values then... but not altitude, which is annoying.
I can bet on it that people will add altitude calc to the baro modules again because of this...

@gautierhattenberger

This comment has been minimized.

Copy link
Member Author

gautierhattenberger commented Sep 4, 2013

The module event fuction is called baro_ms5611_event... so that should
be fine, right?

True for event, but init and periodic have different names, so if you
compile module C code with USE_BARO_BAORD defined the link will fail for
baro_init and baro_periodic. Am I wrong ?

The subsystem also calls both in the convenience periodic function...
The read function triggers a new read and the periodic_check ensures
that you wait long enough for a conversion to be made. Ideally to get
a minimum delay you would call the read at your desired freq and the
periodic check faster (<100Hz) to not delay longer than needed.
See also
http://paparazzi.github.io/docs/latest/ms5611__i2c_8c.html#a45123afce6b944dbe03552035e6b6e5e
This was easy to do in the module, but for the "board" baro subsystem
you have only one periodic call...

Ok

Yes, they should not do more than read the pressure/temp and send it
via ABI.
My point is that you can only send/compare raw values then... but not
altitude, which is annoying.
I can bet on it that people will add altitude calc to the baro modules
again because of this...

If you want to compare altitude, pprz_isa function can do that for you
with a limited amount of code. So it should be enough for people who
wants to have a rough idea of the altitude from their sensor's specific
message.

@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 4, 2013

True for event, but init and periodic have different names, so if you
compile module C code with USE_BARO_BAORD defined the link will fail for
baro_init and baro_periodic. Am I wrong ?

Right... sorry...
The question is also if we even want to keep the USE_BARO_BOARD?

  • We only have that for rotorcrafts
  • Maintaining that in the rotorcraft.makefile with lots of checks about which board is not nice
  • You can't really turn it off without modifying source/makefile

But having to add a module (including the need to set proper configuration values for I2C/SPI/slave dev is not a good solution either... so currently it is really annoying to use a board that already has a baro with the fw firmware.
In any case we need to end up with the same solution for fw and rotorcrafts!

If you want to compare altitude, pprz_isa function can do that for you
with a limited amount of code. So it should be enough for people who
wants to have a rough idea of the altitude from their sensor's specific
message.

The point is that you (or at least I) don't want to do that in postprocessing... I would very much like to directly get the altitude from serveral baros. E.g. to compare an external baro to the one on the boards.
Maybe have the possibility to add multiple baros separately to air_data?
But I guess that doesn't have top priority right now...

@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 4, 2013

Hm.. another thing to consider with the baro module vs. board subsystem:
While you can have several peripherals of the same type (e.g. the ms5611), you can't have the same module twice... So with that in mind it might make sense to have the onboard baros treated separately. Then you could connect a baro of the same type externally (e.g. with cleaner power supply) and compare both.
On the other hand that might be such a corner case where you could easily and quickly write another baro module (as that is a simple wrapper around the peripheral).

In any case we need to be able to provide easy configuration of baros that are on boards and have the possibility to switch them off.

@gautierhattenberger

This comment has been minimized.

Copy link
Member Author

gautierhattenberger commented Sep 4, 2013

The question is also if we even want to keep the USE_BARO_BOARD?

  • We only have that for rotorcrafts
  • Maintaining that in the rotorcraft.makefile with lots of checks
    about which board is not nice
  • You can't really turn it off without modifying source/makefile

But having to add a module (including the need to set proper
configuration values for I2C/SPI/slave dev is not a good solution
either... so currently it is really annoying to use a board that
already has a baro with the fw firmware.
In any case we need to end up with the same solution for fw and
rotorcrafts!

My modification includes FW support of baro board. In the end, it is
only a way to call the on-board baro without loading the correct module.
Until we completely change firmware configuration, looks like it is the
best for user. And it is not too difficult I think to make it
configurable so the user can disable it from airframe file. Not the most
common case, I would say...

The point is that you (or at least I) don't want to do that in
postprocessing... I would very much like to directly get the altitude
from serveral baros. E.g. to compare an external baro to the one on
the boards.
Maybe have the possibility to add multiple baros separately to air_data?
But I guess that doesn't have top priority right now...

Processing the altitude on-board can be done only if you also send the
telemetry message (debug, monitoring). Otherwise only pressure is useful
internally.

@gautierhattenberger

This comment has been minimized.

Copy link
Member Author

gautierhattenberger commented Sep 4, 2013

On the other hand that might be such a corner case where you could
easily and quickly write another baro module (as that is a simple
wrapper around the peripheral).

Correct. Since latest peripherals driver have been written as pure
libraries, it is not a big deal to make another module for the same
sensor if you really need one.

flixr added 9 commits Sep 10, 2013
get fixes from master for further testing
e.g. the ardrone navdata fixes
get fixes from master:
  TELEMETRY_FREQUENCY defaults to PERIODIC_FREQUENCY or 60Hz
  [nps] ups, add missing nps_electrical files
- set data_available to false after it is read
- use pprz_isa_altitude_of_pressure
@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 15, 2013

When you have several baros (e.g. several baro modules on meteo planes or just a second one for reference) it probably won't work as you would expect since all baros publish the BARO_ABS ABI message and by default the INS will take any sender id. Then you have two baros used in the INS, really not what you want....
You have to explicitly set the INS_BARO_ID to one baro id, but e.g. <define name=""INS_BARO_ID" value=" BARO_AMSYS_SENDER_ID"/> won't work as that define is in the c file and not the header...
Currently you would have to set the numeric id explicitly, e.g. <define name="INS_BARO_ID" value="16"/>

So the question is if we want all baro modules to publish BARO_ABS by default or and have users explicitly select one in the INS or rather enable sending of BARO_ABS on a per module basis...

Also we need to get rid of the USE_BARO_x things in ins_alt_float...

@gautierhattenberger

This comment has been minimized.

Copy link
Member Author

gautierhattenberger commented Sep 15, 2013

Not sure adding the ID to the headers will solve the problem, since you don't want to include them to ins or other filters.
An easy solution would be to add the list of ID to abi.h
An other option is to generate the list from an xml file (messages.xml ?) -> not sure it is really interesting compared to the static list.
As I suggested before, we could use string instead of integers, but computation cost is higher -> maybe is is worth trying this solution to see if it makes any sensible difference.

@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 15, 2013

Well, the baro.h with the baro bord file is already included in the ins... and you could just include generated/modules.h as well...

But I think the more important question is still we we should enable publishing in the modules by default...
I can see the following scenario happening: someone has a nicely flying aircraft which already has a baro (either on the board or external), but wants to add a baro module for logging environment data.
Suddenly the whole aircraft doesn't fly properly anymore.
I think this is really bad and having to explicitly change the INS_BARO_ID in cases like that is not a good solution...
So maybe we should disable the publishing for the modules by default and have the board baros enabled...
If you then want to use a baro module with the INS you have to explicitly enable the BARO_ABS publishing in the module.

@OpenUAS

This comment has been minimized.

Copy link
Contributor

OpenUAS commented Sep 25, 2013

INS independent of air_data, yes plz, and indeed the "disable the publishing for the modules by default" sounds sane and clean to me. Also disableing main board baro should be just a simple USE_BASE_BARO or so.

@flixr

This comment has been minimized.

Copy link
Member

flixr commented Sep 30, 2013

@OpenUAS as already stated in the pull request description you can simply disable the on-board baro with <configure name="USE_BARO_BOARD" value="FALSE"/>

flixr and others added 10 commits Oct 7, 2013
Conversion of ADC to Pascal should be roughly correct now, not tested though.
as a temporary workaround to get baro simulation in ocaml sim
currently, the dtd is the same, but it can be changed in the future to
be more adapted to ABI messages
Same BARO_BOARD_SENDER_ID for all baro_board implementations.
Since there can only be one baro_board at the same time and
this provides a good default for INS_BARO_ID.

The abi_sender_ids.h can be considered a temporary solution,
we might come up with a different scheme later...
@flixr

This comment has been minimized.

Copy link
Member

flixr commented Oct 10, 2013

So for now all the ABI BARO_x_SENDER_IDs are defined in abi_sender_ids.h. We can still change that later if we come up with a better scheme...
Also all baro_board implementations use the same BARO_BOARD_SENDER_ID, since there can only be one baro_board at the same time and this provides a good default for INS_BARO_ID.
So if you want to use an onboard baro the INS_BARO_ID is already ok (also if you have additional baro modules). To use a baro module for INS: <define name="INS_BARO_ID" value="BARO_x_SENDER_ID"/>

Is there anything else needed before we merge this? Looks ok from my end so far (IMHO we can test/validate the baro scale for the remaining and not so much used sensors later in master).

@gautierhattenberger

This comment has been minimized.

Copy link
Member Author

gautierhattenberger commented Oct 11, 2013

I'm ok for merging like this, so we can start with other sensors

@flixr flixr closed this in f9a3c2f Oct 11, 2013
@flixr flixr mentioned this pull request Oct 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.