search

Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.99k stars 5.17k forks source link

Fixes to mpu9250 #6433

Closed KevinOConnor closed 5 months ago

KevinOConnor commented 6 months ago

I noticed a couple of errors in the mpu9250 code.

The first is the use of time.sleep() in the host code, which isn't valid - it can causes glitches in the host code processing and doesn't actually guarantee a pause at the mcu.

The second is incorrect timing in the command_query_mpu9250_status() implementation. To get good measurement timing, that function needs to report the number of samples in the mpu9250 chip at the time of the call - so it should not use a cached value of the fifo level.

I don't have an actual mpu9250 (nor similar) so I can't test these changes directly.

@mattaw , @bluesforte - fyi.

-Kevin

mattaw commented 6 months ago

Great you looked into it. I'll check it out this week.

mattaw commented 6 months ago

See comment https://github.com/Klipper3d/klipper/pull/6433#issuecomment-1861761279

Just tested it on my RPi 3B+, failed with MCU connection error in MEASURE_AXES_NOISE. MCU doesn't respond to FIRMWARE_RESTART either, so I'll have to dig out the oscilloscope to see what it is doing.

[At a guess the issue on RPi3 & Linux is that the increase in the number of I2C bus messages is a problem. Specifically, the Linux userspace i2c_dev driver on a RPi 3 (anecdotally the RPi4 too) leaves huge gaps on the bus between commands, regardless of what settings are used. I'll update here as I learn more.]

KevinOConnor commented 6 months ago

That's strange - the changes don't increase the number of i2c messages - the command_query_mpu9250_status() code was always sending an i2c message, it just wasn't using the results.

A useful debugging test would be to see which of the three commits in this PR is introducing the regression. Each commit makes a minor change.

-Kevin

mattaw commented 6 months ago

I'm an idiot - I have not applied your PR to test it yet, it failed running master. Not a good sign for either my hardware or my skills! I'll report in when I have got it working properly, tested and I'll start debugging my previous issue. (sigh, end of semester woes)

Firstly, I take back what I said about the patch introducing more I2C bus transactions, I'll need to recheck on the OSC what is going on properly. It might be a newer kernel version or similar that is causing the issue.

To be honest, my fix to the code was somewhat band-aiding it, and I never fully understood what klippy was doing with the bus messages. Could you flag the dev docs for (or spare 15 mins to talk me through) the accelerometer code path from klippy.py thru the MCU, and its algo? Then I could have a better stab at properly addressing the issue, maybe adding in a way of detecting failures and recovering etc.

KevinOConnor commented 6 months ago

FYI, I added an additional change proposal to this PR. The current code queries the INT_STATUS register and reports an overflow condition only upon stopping the sensor. Unfortunately, the host code currently ignores messages after initiating a stop, so an overflow detected at that point doesn't get propagated to the ultimate user reports.

The additional change is to query the INT_STATUS register (and check for overflows) on every query_mpu9250_status command. The host calls that command once every 100ms, so this change does introduce some additional i2c messages, but it should be a trivial increase.

-Kevin

KevinOConnor commented 6 months ago

FYI, I have rebased this branch on top of the latest changes in the master branch (which now includes the merged PR #6432).

-Kevin

KevinOConnor commented 5 months ago

@mattaw - do you think you will be able to check the code on this branch (and branch #6437)? If not, could you recommend someone that could?

Thanks, -Kevin

dockterj commented 5 months ago

@KevinOConnor I was working on this a while ago but never got it working and haven't picked it up since Mattaw made his fixes. I have an MPU6050 attached to an arduino nano and it seems to be working reliably now (I added pull up resistors). The axes noise is high (xy around 270 and z around 500). This is on a makerbot replicator 2 (cartesian). I was getting i2c errors without the pullup resistors (I should make a PR to change the Klipper documentation to reference them).

orig files are with the main branch. new files are from dropping in the 2 files in this PR to my environment.

They look close to me. Let me know if there is more that I can test or verify.

shaper_calibrate_y-new shaper_calibrate_x-new shaper_calibrate_y-orig shaper_calibrate_x-orig

KevinOConnor commented 5 months ago

Thanks for testing! If there are no further comments I'll look to commit in a couple of days.

-Kevin