Add support for ICM20948 accelerometer

[phansel]

By popular request, adding support for the non-obsolete ICM20948 accelerometer by porting the MPU9250 codebase. There are several register changes which obstruct integration with the MPU9250 driver, e.g. automatically detecting that a chip is ICM20948 before initializing it.

Seems to work with an RP2040 Pico:

[mcu pico]
serial: /dev/serial/by-id/usb-Klipper_rp2040_abcdef0123-if00

[icm20948]
i2c_mcu: pico
i2c_bus: i2c0a

[resonance_tester]
accel_chip: icm20948
probe_points:
        100, 100, 20

[static_digital_output pico_3v3pwm]
pins: pico:gpio23

accelerometer values (x, y, z): 29103.915381, 15275.006592, -21317.971582
--------------
ACCELEROMETER_QUERY

I'm not sure if these values are scaled correctly. Are they?

Signed-off-by: Paul Hansel [email protected]

[JamesH1978]

Thank you for submitting a PR, please be aware that you need to sign off to accept the developer certificate of origin, please see point 3 in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md#what-to-expect-in-a-review

Thanks
James

[phansel]

Sign-off added

[KevinOConnor]

We can certainly add support for a new accelerometer, but it should be done by refactoring the existing mpu9250 code, not duplicating it. (Or, if no code changes are needed, by updating the documentation to indicate that the icm20948 is supported by the mpu9250 config.)

Thanks,
-Kevin

[phansel]

Hi @KevinOConnor,

Thanks for taking a look at the PR.

We can certainly add support for a new accelerometer, but it should be done by refactoring the existing mpu9250 code, not duplicating it. (Or, if no code changes are needed, by updating the documentation to indicate that the icm20948 is supported by the mpu9250 config.)

Thanks, -Kevin

Around 90% of the register addresses and values are different, so ICM20948 is not supported by the MPU9250 config. The sample frequency is also different: 4.5 kHz instead of 4 kHz.

• Functions achieved by one register on MPU9250 are set by multiple registers on ICM20948
• Functions configured by two registers on MPU9250 are set by one on ICM20948
• FIFO format and I2C address is the same

There are two ways this could be addressed on the front-end:

• A: mpu9250.py and sensor_mpu9250.c silently detect that a particular sensor is an ICM and not an MPU, set registers correctly
• B: the user simply enters icm20948 in printer.cfg instead of mpu9250 and the duplicated code is retained.

The branch as of 020f250 does work, at least for the ICM20948. I don't have an MPU9250 to test. The data are here:

In the spirit of not copy-pasting, I took a poke at Option A: a78b650

However, something is lacking in my understanding of how I2C is accessed through klipper. It doesn't work. I'll concede that I'm stuck.

Unhandled exception during connect
Traceback (most recent call last):
  File "/home/prusa/klipper/klippy/klippy.py", line 130, in _connect
    self._read_config()
  File "/home/prusa/klipper/klippy/klippy.py", line 123, in _read_config
    self.load_object(config, section_config.get_name(), None)
  File "/home/prusa/klipper/klippy/klippy.py", line 112, in load_object
    self.objects[section] = init_func(config.getsection(section))
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/prusa/klipper/klippy/extras/mpu9250.py", line 258, in load_config
    return MPU9250(config)
           ^^^^^^^^^^^^^^^
  File "/home/prusa/klipper/klippy/extras/mpu9250.py", line 93, in __init__
    dev_id_icm = self.read_reg(REG_DEVID_ICM20948)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/prusa/klipper/klippy/extras/mpu9250.py", line 122, in read_reg
    params = self.i2c.i2c_read([reg], 1)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/prusa/klipper/klippy/extras/bus.py", line 196, in i2c_read
    return self.i2c_read_cmd.send([self.oid, write, read_len])
           ^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'send'

[KevinOConnor]

Ah, I thought your earlier comment was indicating that this chip was mostly compatible with the mpu9250. If they are totally different chips with different protocols then it would not make sense to use the same code for both.

Cheers,
-Kevin

[phansel]

I see - I should have been more precise! It was a copy paste but one followed by quite a few breaking changes. I'd agree that the incompatibility justifies a new file.

[Wulfsta]

@phansel you might want to take a look at my recent changes in #6715 - the situation is similar there. There is a lot of difference between the registers of these chips, but many of the functions are similar or the same... I am not sure if it is truly desirable to try to consolidate the code in this case, but is probably doable?

[phansel]

@Wulfsta Thanks for the comments!

In the specific case of the ICM20948 vs. MPUxxxx, the max sample rate isn't even the same, so a few sequence-dependent components of the init are broken by having to probe the I2C to identify the device. To make matters worse, the DEVICE_ID register doesn't even have the same address.

Someone more familiar with the klipper codebase could probably figure that first issue out, but the net savings are only a few hundred lines of Python.

Since these drivers generally will only be used once or twice, and there are only so many accelerometers, I generally prefer to keep them in separate files to (a) reduce risk of breaking behavior of Chip A by modifying Chip B's driver and (b) improve readability. A bunch of switches in the middle of the driver can be quite confusing, e.g. gpio-pca953x.c.

It's ultimately up to the owners of the repo. I'm happy to help but don't have access to an MPU9250 for regression testing and don't have unlimited time to figure out how klippy/extras/bus.py works.

[Wulfsta]

The only reason I see to consolidate these is if we are concerned about binary size (irrelevant to the Python side, of course) - which should not be an issue anyways, since there is a mechanism in place to optionally include sensors for chips that are sensitive to this. As I said, I am not sure it is desirable to consolidate in this case, I just wanted to provide another resource to consider. 🙂

[phansel]

@KevinOConnor I've updated this PR to match the new CONFIG_WANT_[sensor] format.