Support Sparkfun ICM20948 Breakout board (SPI only, I2C TBD)

[atinm]

I have only tested SPI and 9DOF, I didn't try I2C at all. I also couldn't make the Adafruit ICM20948 board do SPI, there seems to be a hardware limitation. I pulled in the whole Sparkfun code with some minor edits (include strings.h, uncomment DMP usage) - it is MIT licensed. I don't really like wholesale copying of external repos and locally I use submodules, but I don't know how that would work with the Arduino IDE and pulling in madflight as a library.

I also had to change computeAngles() for NED and flip the sign so that clockwise around Z would be positive. I am not sure what that would do to other IMUs, it seems to be required to have clockwise be positive.

[qqqlab]

Hi @atinm, many thanks for your PRs!

I'm currently in holiday mode, and it will take some time before I'll return to my computer keyboard. My comments below are based only on reading your code.

I'm not a big fan of copying libs either, but as Arduino IDE has no option to specify a specific lib version I think copying is the way to go to make sure madflight will compile in the future.

Could you please trim down the copied sparkfun files: delete the folders: examples, .vscode, img and delete the files in the lib root except licence.md and readme.md

For the Arduino IDE we probably need to move the files in SparkFun_ICM-20948_ArduinoLibrary/src one level down to SparkFun_ICM-20948_ArduinoLibrary, or make the util folder a subfolder of src. Otherwise #include "util/ICM_20948_C.h" in src/imu/ICM20948/SparkFun_ICM-20948_ArduinoLibrary/src/ICM_20948.h will fail (I think...)

[atinm]

I am going to try building it exactly as I have it rather than via submodules and see if it works and fix any compile issues.

[atinm]

I just realized why you want to remove the examples etc, it fails your github workflow checks the way it is. I will reorganize the files to fit the madflight style.

[qqqlab]

Removing the folders is required for Arduino, as Arduino compiles all source files in the src folder. At least that is what I thought, I'm surprised to see that for R2040 it compiles...

But anyway, these examples (and the other folders) are not required for madflight. If somebody wants to dive into this particular driver, they can look at the sparkfun repo.

With ESP32 we get this error: imu.h:35:5: error: 'PinStatus' does not name a type.

Apparently PinStatus is not defined for this Arduino flavor. Probably use: bool has_interrupt_rising _edge = false; to make it portable.

Also, I would make has_interrupt_rising_edge a property of ImuGizmo, and not of ImuConfig. I.e. just like has_sensor_fusion, which is also not user configurable, both properties are set by the driver.

[atinm]

@qqqlab actually, not a bad idea removing it from config, but I think I still have to deal with the call to attachInterrupt taking either a PinStatus or an int depending on which board we are using.

[atinm]

Done - moved the interrupt mode to gizmo, default to RISING.

[qqqlab]

Wow, starts to look really good! Thanks!

Apparently I was not clear enough, this is what I was thinking of:

if(has_interrupt_rising _edge) {
attachInterrupt(digitalPinToInterrupt(interrupt_pin), _imu_ll_interrupt_handler, RISING);
}else{
attachInterrupt(digitalPinToInterrupt(interrupt_pin), _imu_ll_interrupt_handler, FALLING);
}

Will work for any arduino flavor, and we get rid of #if defined(ARDUINO_ARCH_MBED) || defined(ARDUINO_ARCH_RP2040).

[atinm]

Done - pass along a bool (still need to change _imu_ll_interrupt_handler's prototype as it doesn't have gizmo available), and removed the #if defined ARDUINO_ARCH_MBED etc.

[qqqlab]

Great! I think the code looks very good! I still have to study it in more detail and learn some more C++ as your applyQuatCorrection construct is new to me :-)

Maybe you could write up some documentation on this PR? As this PR introduces on-chip DMP which involves both IMU and AHR modules it will be helpful to have some guide for the users on how it works. You can post it here and I'll copy/paste it, or create a PR for the madflight-docs repo.

[atinm]

Added comment explaining applyQuatCorrection above it so it is in the code itself and fixed the yaw radian calculation sign to be the same as computeAngles().

[qqqlab]

The code looks good from my perspective. It was a pleasure working with you on this, many thanks!

I do not have this sensor, so I can't test in real life. Did you fly with it?

Let me know if you're ready to merge, then I'll go ahead.

[atinm]

I might try one more thing by adding a config option for magnetometer so I try it with both on and off (don’t need magnetometer for FPV) as the ICM20948 supports both 9DOF and 6DOF sensor fusion and it should be configurable rather than hardcoded which to use.

[qqqlab]

Let's put this in a new PR - I think this will need some additional considerations...

Some (random) thoughts:

Rather add a new imu_gizmo mf_ICM20948_6DOF than a new parameter. (I try to keep number of parameters as low as possible)

If adding a parameter:

New config params should be added at the end of the array - this allows users to upgrade without eeprom corruption.

I would use value 0 or 1 (not <=0 or 1)

Default value 1 makes more sense to me - use mag if we have it.

Rather name the param and related vars imu_use_mag, and reserve the "has" properties for driver capabilities.

Not consistent with current 9dof drivers...

[atinm]

Yeah, not consistent with existing drivers - made it use_mag and moved to bottom. Only really used by ICM20948 for now, could be used by MPU9250 if it supports 6DOF. But left others alone to have minimal code changes.

[atinm]

In the meantime, my quadcopter crashed and burned badly - need to debug it, it stopped listening to IBUS and didn't automatically disarm itself when it lost contact!

[qqqlab]

my quadcopter crashed and burned badly

ouch!

[atinm]

I seemed to have lost some code when I split Icm20948 and ibus code so putting it back - related to calibration and quaternions

[qqqlab]

Hi @atinm, please let me know when you've added all the features you want to have for a PR, and you're confident to merge the PR, then I'll do a final code review. It's more efficient for me than doing many partial reviews, thanks.

Specific PR related discussions and questions are always welcome of course.

[atinm]

Hold off while I get my copter flying with this new code!