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.

[github-actions]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
   If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[nefelim4ag]

there are i2c_write_wait_ack which can be used above
Which ensures that that request is acknowledged by the device

[phansel]

I'll swap that in. Thanks for the suggestion @nefelim4ag

[nefelim4ag]

Those will reorder writes, cause your next set_reg will execute with minclock=0

---

BTW, you should be able to just write 2 bytes at address of DIV1
Same for power management

[phansel]

This is ultimately duplicated from klippy/extras/mpu9250.py. I can change icm20948.py to import functions from there where they're unchanged.

[nefelim4ag]

Yes, I already found that, but this is your code now :D
I just suggest you not mindlessly copy things
minclock=0/reqclock=0 is the default behavior anyway

With wait_ack() you can just add a pause there, before write, or order other writes by minclock
If you merge coupled registers write, that will also make things cleaner, like:

SAMPLE_RATE_DIVS = { 4500: [0x00, 0x0] }
i2c_write_wait_ack([REG_ACCEL_SMPLRT_DIV1] + SAMPLE_RATE_DIVS[self.data_rate])
# or
set_reg(REG_ACCEL_SMPLRT_DIV1, SAMPLE_RATE_DIVS[self.data_rate])

[nefelim4ag]

According to the datasheet, there is a 20ms delay between the chip wake and accelerometer ready (how ready, what does it mean?) but looks like all registers can be just written

[nefelim4ag]

oid is not used anywhere