-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
ICM20602 and ICM20608g: new standalone driver (8 kHz gyro, 4 kHz accel) #13103
Conversation
Awesome! |
2b9eb33
to
00cc503
Compare
d0fa851
to
92a81bb
Compare
src/systemcmds/top/CMakeLists.txt
Outdated
@@ -33,7 +33,7 @@ | |||
px4_add_module( | |||
MODULE systemcmds__top | |||
MAIN top | |||
PRIORITY "SCHED_PRIORITY_MAX" | |||
PRIORITY "SCHED_PRIORITY_DEFAULT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was set to max on purpose. I had several cases where this helped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to follow up with you. At SCHED_PRIORITY_MAX top can interfere with driver scheduling enough to cause an occasional fifo overflow and reset. I'd like to see if we can find a line in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about an argument to optionally run at max priority, but by default at least lower than the sensor bus WQ threads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time top was bumped to SCHED_PRIORITY_MAX the IMU drivers and a few other things were running out of ISRs (hrt_call_every).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continued in #13115.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: is a fifo overflow handled gracefully?
That sound as huge potential for improvement of the flight control! cool. |
Updated with some background information (thanks to @priseborough). |
5283f7b
to
a398a86
Compare
I did a quick test of this branch with sensor comparison logging on an earlier version of the pixracer with a icm20608g IMU. The test involved shaking the board vertically with a low amplitude acceleration at three frequency points. The first was a 900 -> 1100 Hz, followed by 3900 => 4100 and then 7900 ->8100. The following shows the result obtained with the legacy code (log here: https://review.px4.io/plot_app?log=1187443a-c497-4966-93ab-60d99c60ee81) Both IMU's aliased in both the 1kHz. and 4kHz band. The first third is the 1kHz band testing, the middle third is 4kHz and the last third the 8kHz. There was minimal aliasing at 8kHz. Repeating the test using the code from this PR (log here https://logs.px4.io/plot_app?log=176fec85-661c-441f-bdf5-07a41811b4e1) shows that IMU0 now no longer aliases at 1kHz, eg Both continue to alias at 4kHz as expected. The accelerometers on both IMU's lack effective internal anti-aliasing protection, highlighting why a high sampling rate and mechanical vibration isolation is desirable. |
4bbf41c
to
9f55fec
Compare
ae6420f
to
e00454c
Compare
Tested on PixRacer V4: Position Mode: Good. - Procedure Notes: Log: https://review.px4.io/plot_app?log=44f75460-10ed-4b3a-aaeb-1f3eecc5d0a9 |
e00454c
to
0ef4725
Compare
Tested on PixRacer V4:Modes Tested Position Mode: Good. Procedure: Notes: Log: https://review.px4.io/plot_app?log=0511e98f-b0d4-456f-b79a-181676f7a9a9 Tested on Pixhawk Pro V4:Modes Tested Position Mode: Good. Procedure: Notes: Log: https://review.px4.io/plot_app?log=aff566df-40f7-41fa-92d7-75e9992c9fa3 |
0ef4725
to
782564b
Compare
@@ -269,6 +269,10 @@ | |||
#define GPIO_SPI1_MOSI (GPIO_SPI1_MOSI_1|GPIO_SPEED_50MHz) | |||
#define GPIO_SPI1_SCK (GPIO_SPI1_SCK_1|GPIO_SPEED_50MHz) | |||
|
|||
#define DMACHAN_SPI1_RX DMAMAP_SPI1_RX_1 | |||
#define DMACHAN_SPI1_TX DMAMAP_SPI1_TX_2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one conflicts with DShot (TIM1_UP). We can use DMAMAP_SPI1_TX_1
and move the SDIO to stream 6 (DMAMAP_SDIO_2
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I haven't really looked at where things settled after dshot came in. We need to do a pass to get this in place for most of the boards. #12436
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started capturing the full mapping in board_dma_map.h (next to board.h).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current state is here: https://docs.google.com/spreadsheets/d/1mFul5rEZUhfWSB8qSNtGBibBrFA8MPpm0KkxnSyVReI/edit#gid=0. Moving this in-tree is fine with me, it should just be easy to see what are the alternative options.
cefbe93
to
b435a13
Compare
b435a13
to
b79fac8
Compare
Ok new plan. Let's get these drivers in on the side inactive by default and without the ecl clipping changes. They're still useful for analyzing vibration and we can switch over later once we can better handle clipping and get a lot more testing. Would anyone care to review? |
fe029cd
to
bcf01d3
Compare
1917774
to
a87f5cc
Compare
- uses the FIFO and SPI DMA - currently implemented on px4_fmu-v4 only - clip counters - new low pass filter that operates on an array of data (LowPassFilter2pArray) - new sensor messages for better visibility - sensor_{accel, gyro}_fifo: full raw data for optional logging and analysis - sensor_{accel, gyro}_status: metadata, clipping, etc - sensor_{accel, gyro}_integrated: delta angles and delta velocities - eventually these will replace the existing sensor_{accel, gyro} monolith
a87f5cc
to
99e1c75
Compare
Merging this to enable further testing and iteration. Note, these new drivers must be manually started (with the existing mpu6000 stopped). nsh> mpu6000 stop
nsh> icm20602 -R 8 start
icm20602 on SPI bus 1 at 5 (10000 KHz)
nsh> listener sensor_gyro_fifo
TOPIC: sensor_gyro_fifo
sensor_gyro_fifo_s
timestamp: 43686752 (0.000431 seconds ago)
timestamp_sample: 43685585
device_id: 3671306 (Type: 0x38, SPI:1 (0x05))
dt: 125.0000
scale: 0.0010
x: [-27, -21, -33, -28, -25, -33, -16, -15]
y: [-7, -25, -33, -21, -30, -10, 0, -7]
z: [-8, -6, -4, -8, 10, -7, 3, 8]
samples: 8
nsh> listener sensor_accel_status
TOPIC: sensor_accel_status
sensor_accel_status_s
timestamp: 50664663 (0.000564 seconds ago)
error_count: 0
clipping: [0, 0, 0]
device_id: 3605770 (Type: 0x37, SPI:1 (0x05))
temperature: 46.9094
full_scale_range: 156.9064
measure_rate: 1000
sample_rate: 4000
rotation: 8 |
What is the plan for the ICM support currently in the mpu6000 driver? |
Nothing until these are complete replacements. At that point the mpu6000 itself will be stripped down to be nothing but an optimized mpu6000 driver. The real blockers for full deployment are:
In the testing so far during periods of accelerometer clipping the immediate results are quite bad (enormous velocity estimate), and appear to be worse than the behavior of the current driver where the DLPF is enabled and the clipped accelerometer readings are obscured. |
Hi I am looking for ICM20608-G sensor driver code base migration can anyone help me with the github link to do the same? |
The new icm20608g driver is here. https://github.com/PX4/Firmware/tree/master/src/drivers/imu/invensense/icm20608g |
This is a new driver for the Invensense ICM20602 that uses the sensor's FIFO and SPI DMA (on px4_fmu-v4) to transfer raw gyro data at 8 kHz and accel at 4 kHz. It can also do gyro at 32 kHz, but at that point the bus utilization is a bit too high to peacefully coexist with other sensors.
The intent is to keep these opinionated drivers simple and optimized to the point where it'll likely be easier to carry separate drivers for each sensor with minor differences rather than trying to support everything in one place (see current mpu6000 mess). We'll see how this actually pans out for the ICM20608, ICM20689, etc.
Originally I was going to do a pass and enable SPI DMA everywhere first (#12436), but given various problems and delays in that area I think we can move forward incrementally like this.
Features
Background (notes from @priseborough)
IMU Signal Processing
Failure to detect vibration induced inertial navigation failures early enough during system integration or operation has contributed to a number of vehicle loss incidents. In the majority of cases this has been caused by structural resonances excited by motor and propeller blade bassage frequency vibration.
The current practice of using the IMU’s internal DLPF is a major blocker for the following reasons:
Experience working with companies using PX4 has shown that the majority do not understand how propulsion system forcing, structural dynamics, IMU sampling and vibration isolation mount design interact to affect navigation system performance and for those that do, the inability to log the required data prevents the use of normal analytical methods.
The signal chain for autopilot feedback and navigation needs to be split for the following reasons: