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.

Sign up for GitHub

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

Jump to bottom

Px4 imu #1569

Merged
merged 12 commits into from
Mar 31, 2016
Merged

Px4 imu #1569

merged 12 commits into from
Mar 31, 2016

Conversation

kevindehecker
Copy link
Contributor

  1. Wrote a spi driver for the lsm303dlhc. (loosely based on the I2C driver, of which I'm 99.9% sure that it didn't work). May still need some streamlining, but because the I2C driver is such a mess that's not trivial.
  2. Created a PX4 imu, which consists out of
    a) lsm303dlhc (acc) + l3g (gyro) IMU, both on spi
    b) the MPU6050 (acc + gyro) IMU, on spi.
    c) 2 magneto's: one in the lsm303dlhc and an external i2c hmc5811 in the 3dr gps
  3. Changed the ms5611 baro to spi

(this PR builds on #1566, which was not merged yet)

@flixr
Copy link
Member

flixr commented Mar 22, 2016

Nice!
Will be great to have a SPI driver for the lsm303d :-)
But as you said I think this still needs some cleanup and refactoring (i.e. pulling out the common parts).

Also I don't understand why you would add the external hmc5811 mag to the imu driver?
You should use a module if you want to use the external mag...

Please also remove the qt_project.py as it's unrelated to this PR.

@kevindehecker
Copy link
Contributor Author

About the clean up, yeah I was considering removing the I2C stuff, because I don't have the hardware and I'm sure it did not work before anyway... (so apparently nobody is using it).

About the hmc, uhm I was following the imu_mpu6000_hmc5883 code as an example here... will figure out the module thing.

@flixr
Copy link
Member

flixr commented Mar 22, 2016

The imu_mpu6000_hmc5883 driver is for hardware modules that have the hmc included, on the pixhawk this is an optional and external module..

@flixr
Copy link
Member

flixr commented Mar 22, 2016

It would be nice if we could keep the lsm303 i2c driver, it was cherry picked from #625

@flixr
Copy link
Member

flixr commented Mar 22, 2016

Please also note that updating the global imu struct from multiple sources is not properly supported, i.e. you can't tell from which sensor it was last updated...
Also if you have multiple ABI accel/gyro/mag publishers, one should configure the consumers/subscribers to use only one specific IMU (filter on sender ID)...

@flixr
Copy link
Member

flixr commented Mar 22, 2016

Also right now you can have only one set of IMU sens/neutral defines, here it would apply the same to both IMUs...
I don't think enabling both sensors (MPU and LSM303D) makes much sense at this point...

@kevindehecker
Copy link
Contributor Author

Please also note that updating the global imu struct from multiple sources is not properly supported, i.e. you can't tell from which sensor it was last updated...

Hmm, that might explain some troubles I'm having calibrating the Iris...

if you have multiple ABI accel/gyro/mag publishers, one should configure the consumers/subscribers to use only one specific IMU (filter on sender ID).

Well. I do just that in the Iris xml right?
<subsystem name="ahrs" type="int_cmpl_quat" >
<define name="AHRS_ICQ_IMU_ID" value="IMU_PX4_ID" /> <!-- Meaning the lsm303 and l3g -->
<define name="AHRS_ICQ_MAG_ID" value="IMU_MPU6000_HMC_ID" /> <!-- Meaning the external magnetometer -->
</subsystem>

Anyway, to bad that doesn't cover all the cases. I guess I'll remove the mpu6000 from the PX4 imu for now then.

@flixr
Copy link
Member

flixr commented Mar 22, 2016

The AHRS is not the only subscriber to IMU messages... e.g. the aligner and INS also subscribe to IMU

@kevindehecker kevindehecker force-pushed the PX4_IMU branch 2 times, most recently from 261268e to 46059bc Compare March 30, 2016 06:48
@kevindehecker
Copy link
Contributor Author

I think I handled all concerns.

flixr
flixr reviewed Mar 30, 2016
View reviewed changes
conf/firmwares/subsystems/shared/imu_px4fmu_v2.4.makefile
include $(CFG_SHARED)/spi_master.makefile

IMU_PX4FMU_CFLAGS += -DIMU_TYPE_H=\"imu/imu_px4fmu_v2.4.h\"
IMU_CFLAGS = -DIMU_TYPE_H=\"imu/imu_px4fmu_v2.4.h\"
Copy link
Member

Choose a reason for hiding this comment

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

preferably all these variables have the imu name in them, so like IMU_PX4FMU_CFLAGS above...
Or at least the currently unused IMU_PX4FMU_CFLAGS above...

flixr
flixr reviewed Mar 31, 2016
View reviewed changes
conf/modules/mag_hmc58xx.xml
<define name="HMC58XX_CHAN_Y_SIGN" value="+|-" description="Reverse polarity of y axis (default: +)"/>
<define name="HMC58XX_CHAN_Z_SIGN" value="+|-" description="Reverse polarity of z axis (default: +)"/>
<define name="HMC58XX_CHAN_X" value="0|1|2" description="Channel id of x axis (default: 0)"/>
<define name="HMC58XX_CHAN_Y" value="0|1|2" description="Channel id of y axis (default: 0)"/>
Copy link
Member

Choose a reason for hiding this comment

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

default is 1 for y, 2 for z...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops :)

Fixed

@flixr
Copy link
Member

flixr commented Mar 31, 2016

Since #1577 is now merged, could you please rebase it on master again and fix the conflicts?

@kevindehecker
Copy link
Contributor Author

Done

flixr
flixr reviewed Mar 31, 2016
View reviewed changes
sw/airborne/subsystems/imu/imu_px4fmu_v2.4.h
#include "peripherals/lsm303dlhc_spi.h"

#ifndef IMU_PX4_DISABLE_MAG
#ifdef MODULE_HMC58XX_UPDATE_AHRS
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't check if it's defined, but if it's TRUE, so only #if MODULE_HMC58XX_UPDATE_AHRS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are very right ... tnx :)

@kevindehecker
Copy link
Contributor Author

Ow fyi, I have not tested the I2C lsm303dlhc driver, but I'm pretty sure it doesn't work perfectly. Maybe add a comment or something? I spend quite a lot of time on the I2C driver until I discovered it was never used (and probably non working) in the first place...

@flixr
Copy link
Member

flixr commented Mar 31, 2016

I don't think the driver was never used, apparently it was working fine for the guy who made the PR...
But printing a warning if that code is compiled is a good idea...

@flixr flixr merged commit b998261 into paparazzi:master Mar 31, 2016
@flixr flixr added the Driver label Mar 31, 2016
@flixr
Copy link
Member

flixr commented Apr 1, 2016

Actually I just realized that you used (and rewrote) the regs and driver for the LSM303DLHC, but you are actually using a LSM303D (those are not the same).
That's probably also why you think that the i2c driver was never working, since it was written for a slightly different sensor (which is also deprecated now).
Also there is https://github.com/paparazzi/paparazzi/blob/master/sw/airborne/peripherals/lsm303d_regs.h which should have had the correct register defs and useful enums already...

@kevindehecker
Copy link
Contributor Author

Hmmmm :)

@flixr flixr mentioned this pull request Apr 27, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Driver
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kevindehecker @flixr