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

CH Robotics UM6 IMU/AHRS driver #347

Closed
wants to merge 5 commits into from
Closed

CH Robotics UM6 IMU/AHRS driver #347

wants to merge 5 commits into from

Conversation

podhrmic
Copy link
Member

Drivers for CH Robotic UM6 IMU/AHRS added (see www.chrobotics.com). Generic AHRS for all external AHRS systems (such as GX3 etc) added too.

The IMU driver is responsible for initializing and communicating with the IMU (e.g. getting new measurements) and updating AHRS with imu rate and estimated attitude quaternions.
AHRS subsystems in this case only updates the vehicle attitude according to given IMU rate and quaternions.

Use as

AHRS can be renamed from "Extern_euler", I just couldnt figure out a better name.

…eneric AHRS for all external AHRS systems

(such as GX3 etc) added.
endif

IMU_UM6_CFLAGS += -DUSE_IMU
IMU_UM6_CFLAGS += -DUSE_UM6
Copy link
Member

Choose a reason for hiding this comment

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

USE_UM6 is not used anywhere so it can be removed...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, it was for debugging and I forgot to remove it.

@flixr
Copy link
Member

flixr commented Jan 16, 2013

If you really really want to release that under Creative Commons, specify exactly which one (with the link to it).
However it is not recommended to use Creative Commons licenses for software (they are also not GPL compatible) http://wiki.creativecommons.org/FAQ#Can_I_use_a_Creative_Commons_license_for_software.3F
So I'm not even sure we can combine them here. If you insist on Creative Commons we would have to check this and reject it if necessary.

#define IMU_GYRO_Q_SIGN 1
#define IMU_GYRO_R_SIGN 1
#endif
#if !defined IMU_GYRO_P_NEUTRAL & !defined IMU_GYRO_Q_NEUTRAL & !defined IMU_GYRO_R_NEUTRAL
Copy link
Member

Choose a reason for hiding this comment

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

Neutrals are already default to zero in imu.h, so you don't have to repeat that here...

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually sends the scaled values of the sensors, so I just replaced the scaling functions with dummy ones to keep the code consistent.

@flixr
Copy link
Member

flixr commented Jan 16, 2013

Please add at least a doxygen block to each source file after the license header:
http://paparazzi.github.com/docs/latest/styledoxygen.html

UM6Running
};

static inline bool_t UM6_verify_chk(uint8_t packet_buffer[], uint8_t packet_length) {
Copy link
Member

Choose a reason for hiding this comment

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

Functions that only used "internally" should not go into the header but only the c file as everything in the header will be indirectly included by everything using imu.h
Same goes for defines and variables...

@flixr
Copy link
Member

flixr commented Jan 16, 2013

Eventually we will have to settle on a common method on how to use external IMU/AHRS/INS sensors like this.
E.g. the xsens and chimu are written as modules that directly implement the imu, ahrs or ins interfaces as appropriate (without an intermediate ahrs_extern_euler).
But that we can discuss separately...

@dewagter
Copy link
Member

This is indeed worth a discussion.

e.g. in master it is not possible to run the ALT_KALMAN on raw XSens GPS
data anymore when you have bad pressure data corrupting the filter in the
XSens. Should I then convert it into a ahrs_xsens ?
e.g. Chimu is a module because subsystems don't (didn't) have a convenient
way to define periodic functions (also the ucenter-module could go into the
driver if it had a periodic function)

-Christophe

On Wed, Jan 16, 2013 at 11:26 AM, Felix Ruess notifications@github.comwrote:

Eventually we will have to settle on a common method on how to use
external IMU/AHRS/INS sensors like this.
E.g. the xsens and chimu are written as modules that directly implement
the imu, ahrs or ins interfaces as appropriate (without an intermediate
ahrs_extern_euler).
But that we can discuss separately...


Reply to this email directly or view it on GitHubhttps://github.com//pull/347#issuecomment-12312420.

Removed unused USE_UM6 variable (previously defined for debugging purposes).
Cleanup of header and source files.
@podhrmic
Copy link
Member Author

Doxygen added - is it sufficient like that?

The idea behind was that it seems to make more sense to have a "generic" AHRS subsystem which only updates the airframe states with measured attitude from IMU, rather than having a separate IMU/AHRS subsystem for each IMU where lots of code would be duplicated.

I chose to use quaternion representation, as it seems to be the most stable solution (no singularities etc). So the IMU subsystem should either output quaternions directly or convert the attitude estimation from IMU into quaternion representation.

What do you think?

@flixr
Copy link
Member

flixr commented Jan 18, 2013

About the doxygen comments, please specify the path relative to sw/airborne (same path that is used to include the files), so e.g. @file subsystems/ahrs/ahrs_extern_euler.c

@flixr
Copy link
Member

flixr commented Jan 18, 2013

Ok, first some comments on the general situation with IMU/AHRS, etc...
(maybe we should discuss this on the mailing list though?)

With the new state interface that we have now in master most of the ahrs interface is actually gone now (except the function declarations).

In general it seems like for sensors like these which are essentially already an AHRS and not only an IMU, the imu interface would not be needed and it would make more sense to implement them as an AHRS directly. (Disregarding that we still require calls to imu directly from the main.c for the moment).

We definitely have to discuss how to structure this now (e.g. the imu subsystem should only be mandatory if you want to run your own ahrs algorithm on the IMU measurements). The only places we use the imu values directly, are the when the actual acceleration measurements in imu/body frame are needed: nav_catapult, energy_ctrl and INS.
This all ties into the more general modules vs. subsystems discussion..

About the ahrs_extern, why is it called ahrs_extern_euler? Since you use quaternions anyway which is good :-)

It might make sense to use the new generic orientation representation in the ahrs_extern.
Then it would be better to use as a wrapper for other ahrs/imus as well (if we want to go that way...) Or we add this to the main ahrs struct (which only has the status left now after we have the state interface)?
Anyway... it could be something along these lines:

struct AhrsExtern {
  struct OrientationReps ltp_to_imu_orientation;
  ..
};
struct AhrsExtern ahrs_impl;

receive_attitude() {
  orientationSetQuat_f(&ahrs_impl.ltp_to_imu_orientation, &UM6_quat);
}

Regarding your implementation. You get the values as integers, convert to float, scale and convert back to int? With the above the values could be set in float directly instead of converting to int first by hand... (or scale and set in int to save some computing power...). Then you get them any desired format when needed (e.g. in your case ahrs_propagate to rotate to body frame and push to the state interface.

In any case, if you want we can also merge this now as-is and continue the discussion on how to better handle different IMU/AHRS use-cases. (hoping you will still be around to implement and test changes where it affects your implementation).

@podhrmic
Copy link
Member Author

I moved the discussion to the mailing list. Some comments about the current code though:

  1. In my understanding the IMU subsystem should gather sensor data so in case of AHRS sensors it should manage the communication, packet parsing etc. And AHRS would only update states. But once the IMU subsystem wont be mandatory, I am more than happy to have just AHRS for each specific sensor with some general interface as you suggested.
  2. I renamed it to ahrs_extern_quat, it is probably more appropriate
  3. UM6 stores data internally as float, but send it as int16 and you have to multiply it afterwards to get back to a correct value (float). I used converting back to int because the integer version of attitude stabilization and control seemed to be more mature and tested, but with the new state interface it probably doesn't matter anyway, right?

I would rather merge the code as is now, and then update it later - depending on the output of the discussion, as I ll be around for a bit.

flixr pushed a commit to flixr/paparazzi that referenced this pull request Jan 18, 2013
…chrobotics.com).

Generic AHRS for all external AHRS systems (such as GX3 etc) added.
closes paparazzi#347
@flixr
Copy link
Member

flixr commented Jan 18, 2013

ok, let's continue on the mailing list...
merged with 3789ea8

@flixr flixr closed this Jan 18, 2013
dewagter pushed a commit to tudelft/paparazzi that referenced this pull request Feb 5, 2013
…chrobotics.com).

Generic AHRS for all external AHRS systems (such as GX3 etc) added.
closes paparazzi#347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants