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

Add BMI270 driver, and support for KakuteH7v2 and KakuteH7mini #20545

Merged
merged 32 commits into from
Jan 23, 2023

Conversation

j-chen-opteran
Copy link
Contributor

@j-chen-opteran j-chen-opteran commented Nov 4, 2022

Describe problem solved by this pull request

Following up on #20453, added code to read a BMI270 IMU at 1600Hz

Describe your solution

Based off the uncompleted pull request by @dagar, with some additional information pieced together from the datasheet and the Ardupilot BMI270 driver. Accounts for the fact that the data frames that the BMI270 outputs are not always the same length.

Test data / coverage

Tested and working with the Holybro Kakute H7 Mini V1.3, which is equipped with a BMI270 as well as a 1GB W25N01GVZEIG NAND flash chip in lieu of a SD card. A new board target is in the works that uses NuttX's SmartFS instead of a SD card driver, and will be pull-requested in due time.

@j-chen-opteran j-chen-opteran marked this pull request as draft November 4, 2022 11:20
@j-chen-opteran j-chen-opteran marked this pull request as ready for review November 4, 2022 13:42
@arguelle
Copy link

arguelle commented Nov 8, 2022

Thanks, I am glad to see progress on this!

I built and upload this to a Kakute H7 Mini V1.3 but the sensor is still not detected.
The only change I made to the board is specified in #20284
What other changes might be needed to get this running?

if (res == PX4_OK) {
RegisterWrite(Register::CONFIG1, 1);
PX4_DEBUG("Successfully uploaded initialization file onto BMI270");
px4_mdelay(150);
Copy link
Member

Choose a reason for hiding this comment

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

In general we try to avoid having any sleeps in these drivers because they'll completely block the bus (which is often shared with other sensors).

An mdelay(150) on NuttX is particularly bad because it will just spin for 150 milliseconds and starve everything lower priority.

@dagar
Copy link
Member

dagar commented Nov 9, 2022

Thanks for finishing, this mostly looks good other than a few problematic sleeps.

Do you have any testing? If you can share a ulg (even from the bench) I can do a quick sanity check.

@j-chen-opteran
Copy link
Contributor Author

@dagar @arguelle thanks for the prompt feedback. Made a new KakuteH7Mini target, and it should now work (with caveats) on the Kakute H7 Mini V1.3. I also have a MPU-equipped V1.1 on hand and will test when I have time. It seems though that the BMI270 doesn't initialize by itself even though I've listed it in rc_board_sensors and spi.cpp. Starting it via NSH with bmi270 start -s works.

With regards to the sleep code, I had a go at replacing px4_mdelay with ScheduleDelayed but unfortunately it doesn't behave correctly anymore. Any pointers to sort the sleep and initialization issues out would be greatly appreciated. The SPI init code isn't quite correct now either, as it still probes around for a SD card which this FC isn't equipped with. Removing the SD init code causes an avalanche of compilation errors.

In the long run I'd love to get the W25N01GV NAND flash on the V1.3 working. However unfortunately there doesn't seem to be NuttX drivers for that particular model of NAND flash, and my bodging using the included W25N driver (afaik a NOR flash driver) didn't work. Some guidance on getting that sorted would be great, as otherwise this board can't log apart from ulog streaming via MAVLink - which works quite alright with an ESP8266, providing ~24KB/s of throughput.

Please find the bench test ulog file here: https://review.px4.io/plot_app?log=9fdafdf1-b591-4b19-86bb-7e30fa85c769

@dagar
Copy link
Member

dagar commented Nov 9, 2022

With regards to the sleep code, I had a go at replacing px4_mdelay with ScheduleDelayed but unfortunately it doesn't behave correctly anymore.

ScheduleDelayed isn't a sleep, it schedules the next work queue cycle. That's why the main driver structure is a simple state machine. You set the desired state, schedule the next run, and leave (so other things on that bus can run).

@arguelle
Copy link

I managed to get the IMU working with your new target in de47855 I do lose RC for a moment when I load the driver though. In addition, in the calibration I have to set 270_deg yaw even though my FC is mounted with 0_deg yaw. I am excited to get flying soon :D

The latest commit does not work for me. When I load the driver the CPU usage goes too high and I get error messages for the gryo/accel (see image). It also makes my RC unreliable.
error_msgs
error_after_init_sensor

Also, as of 0e2b1ee my Kakute H7 Mini V1.3 no longer responds to the request for params. Is this worth opening up a separate issue for? It seems to be a Mavlink related commit.

@j-chen-opteran
Copy link
Contributor Author

Replaced the delays with a new state for loading the microcode. The sensor still doesn't start automatically though, can't see anything else apart from rc_board_sensors and spi.cpp determining which IMU to load. Running bmi270 start -s will start it just fine, is there something I'm missing?

@arguelle there is a hardcoded rotate in the sensor driver that accounts for IMU mounting, that could be wrong. Will test and check.

@j-chen-opteran
Copy link
Contributor Author

@dagar any thoughts on why the driver won't start automatically but starts fine when I do it via nsh?

@shupx
Copy link

shupx commented Nov 17, 2022

@j-chen-opteran. As for the rotation problem, maybe you can try bmi270 -R 6 -s start in rc.board_sensors with -R means rotation. I refer this from px4 user guide https://docs.px4.io/main/en/modules/modules_driver_imu.html#mpu6000

As for the auto-start of the imu, maybe you can exchange the sequence of bmp280 and bmi270 in rc.board_sensors? Or try other - comments after bmi270 start?

@shupx
Copy link

shupx commented Nov 17, 2022

@j-chen-opteran Maybe the spi is not well initialized in spi.cpp. The GPIO Port and Pin may be wrong? I guess kakute h7 and h7mini has different SPI connection?

@julianoes
Copy link
Contributor

@j-chen-opteran I hope it's ok that I've rebased this and added a few commits to make it work on the Kakute H7 V2.

@julianoes
Copy link
Contributor

I noticed that the accel calibration doesn't work yet, presumably because the scaling on the y axis doesn't look right.

While the values in x and z are between -9.8 and 9.8 m/s^2, the values in y are between -14 and 14 m/s^2!

@julianoes
Copy link
Contributor

I've added more commits, fixing zeros in gyro/accel reads, as well as the temperature. However, the range in the x and y axes is still wrong.

@dagar have you seen that before? Basically values between -14..14 instead of -9.8..9.8?

@j-chen-opteran
Copy link
Contributor Author

I've added more commits, fixing zeros in gyro/accel reads, as well as the temperature. However, the range in the x and y axes is still wrong.

Thanks for sorting this out! Does it start by itself on the Kakute H7 V2 without needing to do it manually now? Will have a look at the scaling again, there are some register bits that determine accelerometer scale and I'll double check.

if ! icm20689 -R 6 -s start
then
# The Kakute H7 v2 comes with a BMI270 instead
bmi270 -s start
Copy link
Member

Choose a reason for hiding this comment

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

I don't suppose there's any way to explicitly know if we're on a v1 or v2?

Copy link
Contributor

@julianoes julianoes Dec 3, 2022

Choose a reason for hiding this comment

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

@vincentpoont2 is there an easy way to check if we're on v1 or v2?

ver all says HW arch: HOLYBRO_KAKUTEH7

Copy link
Contributor

@vincentpoont2 vincentpoont2 Dec 5, 2022

Choose a reason for hiding this comment

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

@julianoes Physically, they look different, there is also silkscreen on the PCB.

Kakute ships with betaflight firmware, to use PX4, user need to flash PX4bootloader into it.
If they share the same bootloader I guess there is no way to distinguish them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have created separate build targets and device_ids now.

@julianoes
Copy link
Contributor

@j-chen-opteran and @dagar I've fixed the wrong accel range, check the last two commits.

It turns out that the FIFO readout was off and contained some 0s which magically lead to correct values in the y and z axes because the accel range/scale was also wrong.

@julianoes julianoes changed the title add BMI270 driver code Add BMI270 driver, and support for KakuteH7v2 and KakuteH7mini Dec 14, 2022
RegisterRead(Register::CHIP_ID);

// It takes 200us for the device to start SPI communication.
px4_usleep(200);
Copy link
Contributor

Choose a reason for hiding this comment

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

@dagar this is ok, right? It's just in the probe.

Instead of reading directly from the FIFO_DATA register, we read instead
from the FIFO_LENGTH_0 register. That way we read length bytes before
the FIFO data and hence didn't actually read enough data. This lead to
accel y and accel z data be 0 every so often which ended up matching the
range, even though it was wrong. (Spoiler: the range calculation is also
wrong.)

Signed-off-by: Julian Oes <julian@oes.ch>
Not sure where the range defines come from but they did not match the
datasheet.

Signed-off-by: Julian Oes <julian@oes.ch>
We now create a separate target for the KakuteH7v2.

Signed-off-by: Julian Oes <julian@oes.ch>
There will be a v2 as well.

Signed-off-by: Julian Oes <julian@oes.ch>
This syncs the kakuteh7mini target with changes that seem to have
happened in the meantime. Also, we now use a separate target and
device_id for it.

Signed-off-by: Julian Oes <julian@oes.ch>
This is required because v2 comes with NAND flash instead of an SD card.
By having a separate target we can handle that a bit more gracefully.

Signed-off-by: Julian Oes <julian@oes.ch>
This seemed wrong before.

Signed-off-by: Julian Oes <julian@oes.ch>
We have no SD card and no support for the NAND flash yet.

Signed-off-by: Julian Oes <julian@oes.ch>
We have no SD card and no support for the NAND flash yet.

Signed-off-by: Julian Oes <julian@oes.ch>
There is only NAND flash on this board.

Signed-off-by: Julian Oes <julian@oes.ch>
There is only NAND flash on this board.

Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
We need to wait 200us after telling the device to initialize SPI.

This change fixes the issue where the BMI270 did not initialize on cold
boot.

Signed-off-by: Julian Oes <julian@oes.ch>
Signed-off-by: Julian Oes <julian@oes.ch>
For some reason a comment saying "disable not set" enables it.

Signed-off-by: Julian Oes <julian@oes.ch>
We should only start publishing once the there is actually data in the
buffers.

Signed-off-by: Julian Oes <julian@oes.ch>
@julianoes
Copy link
Contributor

I just tested flashing using QGC which worked fine for me, both on H7v2 and H7mini, with QGC latest stable release and a newer daily build.

always Waiting for bootloader...

When you get that. Are you sure you don't have QGC open which occupies the port? Or the other thing would be if the board ID of the bootloader doesn't match the firmware version that you're trying to flash.

@julianoes
Copy link
Contributor

@dagar I think this is ready to be merged. I'm going to add the docs for it next, and then we need to make sure the links to the bootloader files are all correct.

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/kakuteh7-v1-paramters-not-saved/36845/2

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