klipper icon indicating copy to clipboard operation
klipper copied to clipboard

mpu9250: add MPU6500

Open dingyifei opened this issue 1 year ago • 22 comments

Signed-off-by: Yifei Ding[email protected]

dingyifei avatar Sep 10 '22 02:09 dingyifei

I have tested MPU6500 with a pico, the results are beautiful

Additionally, MPU9250 and 6050 are obsolete parts, and most of them on the market have a really high failure rate.(especially with 9250, numerous cases of missing Mag/actually being 6050/weird device ID issue on SlimeVR server). I think these should be mentioned in the doc and or replacing "[mpu9250]" with "[mpu6500]". (These are not in the commits)

y x

dingyifei avatar Sep 10 '22 03:09 dingyifei

Thanks. Maybe @dmbutyugin or @bluesforte have comments on this?

-Kevin

KevinOConnor avatar Sep 12 '22 01:09 KevinOConnor

Should I add Device ID of all MPUs, because the refurbished chips with defects appear to have various deviceID but function as a normal MPU-6050. If some people get mpus with these weird ids, they are still likely to have working MPUs.

The workaround for slimeVR is return deviceId == 0x68 || deviceId == 0x70 || deviceId == 0x71 || deviceId == 0x73 || deviceId == 0x75; // Allow any MPUs

I made some changes to the formatting, let me know if the new format is better. I'll fix everything else later in the day.

dingyifei avatar Sep 12 '22 19:09 dingyifei

I'm not sure that it makes sense to add this pull up resistor reference. A user who is not able to setup a proper cable most likely will do more harm than good with this information.

Otherwise, LGTM 👍

Sineos avatar Sep 13 '22 11:09 Sineos

Should I add Device ID of all MPUs, because the refurbished chips with defects appear to have various deviceID but function as a normal MPU-6050. If some people get mpus with these weird ids, they are still likely to have working MPUs.

OK, I'm fine with it. At this rate, maybe you can now define a list of acceptable device IDs rather than having that long list of comparisons?

dmbutyugin avatar Sep 14 '22 20:09 dmbutyugin

Should I add Device ID of all MPUs, because the refurbished chips with defects appear to have various deviceID but function as a normal MPU-6050. If some people get mpus with these weird ids, they are still likely to have working MPUs.

OK, I'm fine with it. At this rate, maybe you can now define a list of acceptable device IDs rather than having that long list of comparisons?

Yes, I this line of code is a SlimeVR Arduino workaround, I'll add a warning message for suspicious IDs and use a list for the known IDs.

For twisted pairs I'll see if I get some time to run some tests with oscilloscope to see how everything is affected. I got a shielded Ethernet cable and a regular Ethernet cable.

dingyifei avatar Sep 14 '22 21:09 dingyifei

I haven't tested the last two changes I've made, but I have included a few more MPU IDs. Note that I changed MPU9250's ID and added MPU9255 with the same ID as the previous MPU-9250 based on the official documents. It is common to get these MPUs just by luck. 0x75 and 0x69 are IDs of MPU 6xxx/9xxx with some internal connection error. Based on SlimeVR community discord, these usually work as usual, but some reported worse measurements/more noise.

I only have MPU-6500s with the correct ID, so I cannot test other MPUs.

dingyifei avatar Sep 17 '22 01:09 dingyifei

Sorry abt the delays..... my testing plan...... is not going well...... still couldn't find enough time to crimp a piece of cable for 50 times

dingyifei avatar Sep 28 '22 18:09 dingyifei

Just saw the notification now for my input on this. Good changes - always a fan of improving the docs and really like the new fritzing diagrams. Good to have the device ids in a list now that there are more than 2.

Only real feedback I have is the one I left above re: mpu devices' production status. My opinion is that it's not worth calling out because a) even if they aren't 'officially' supported, all the mpu devices are still very readily available for purchase at retailers, and b) I think users can do their own research/shopping and figure out which model they want to go with, since all of them are plenty accurate to perform input shaping.

bluesforte avatar Sep 29 '22 09:09 bluesforte

Just saw the notification now for my input on this. Good changes - always a fan of improving the docs and really like the new fritzing diagrams. Good to have the device ids in a list now that there are more than 2.

Only real feedback I have is the one I left above re: mpu devices' production status. My opinion is that it's not worth calling out because a) even if they aren't 'officially' supported, all the mpu devices are still very readily available for purchase at retailers, and b) I think users can do their own research/shopping and figure out which model they want to go with, since all of them are plenty accurate to perform input shaping.

I'll take that line out, it seems to be redundant. In the case they find something wrong with their mpu, existing sources with better explanations are readily available.

dingyifei avatar Sep 29 '22 10:09 dingyifei

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.

github-actions[bot] avatar Oct 13 '22 12:10 github-actions[bot]

Let me formally assign myself as a reviewer (I thought that this already happened). I looked again at the latest changes and I'm fine with them (I just added a tiny comment to move one sentence). Then, if it is tested and confirmed to be working, let's merge it into Klipper.

BTW, I got some MPU chips myself, but I cannot promise I'll be able to test them with this branch soon.

dmbutyugin avatar Oct 15 '22 13:10 dmbutyugin

Thanks. On the top right hand side of this github PR page there is an "assignees" option - if you assign this PR to yourself then it'll turn off the bot.

-Kevin

KevinOConnor avatar Oct 15 '22 20:10 KevinOConnor

Hmm, for some reason, I thought that it is handled via 'Reviewers' list, but seems to be not the case. Tried the 'assignees' now.

dmbutyugin avatar Oct 15 '22 21:10 dmbutyugin

@dingyifei, thank you!

Besides these couple of comments to change the documentation, I tested your branch, and it works all fine:


Found mpu-6500 with id 70

MPU9250 starting 'mpu_test' measurements

accelerometer measurements started

.............

I used the powering I suggested in the comment:

3V3 (pin 36) -> VCC

GND (pin 38) -> GND

and the i2c0e bus (but this should not matter, the suggested I2C connection is fine).

Hi, I'm really busy in the last few weeks, thank you for giving these comments. I'm really sorry for being inactive for so long.

dingyifei avatar Oct 17 '22 15:10 dingyifei

@dingyifei No worries, thank you for your contribution!

dmbutyugin avatar Oct 17 '22 19:10 dmbutyugin

Nice work @dingyifei 👍. Some comments:

If you are already working on it, you could also add the combination for ADXL and Pi Pico. All the details, including a fritzing can be found here: https://klipper.discourse.group/t/raspberry-pi-pico-adxl345-portable-resonance-measurement/1757

There is also a report about numpy compatibility here: https://github.com/Klipper3d/klipper/issues/5831 I cannot reproduce, since on my system automatically v1.16.6 got installed. Acc to the documentation, v1.19 and above is no longer compatible with Python2.

My tests have shown that it is generally preferable to use the Vin port of the ADXLs instead 3V3 (if both are present). An updated fritzing can be found here: https://klipper.discourse.group/t/adxl345-different-between-rpi-pico-and-rpi-3b/1842/30?u=sineos

In the same discourse thread, it was found that pulling GPIO23 high on the Pico, e.g. with

[static_digital_output pico_3V3pwm]
pin: pico:gpio23

is beneficial, due to more stable power supply. It has quite an effect on the ADXL. Did not test with the MPU but surely does not hurt there either.

Sineos avatar Oct 19 '22 06:10 Sineos

I don't mind making additional changes to the PR, but it is also better to have something complete merge than to have never-ending PR that grows bigger and bigger :) So, @dingyifei up to you. I think the suggestion about pulling gpio23 high on Raspberry Pi Pico specifically is actually pretty good (though it is only applicable to Raspberry Pi Pico, for example I've seen standalone boards like FYSETC PortableInputShaper based on Pico that do not require that).

NumPy issue would require further looking. My impression was that pip should figure out the correct compatible version. At least, it does a good job for Python2 installations. Not sure what's going on there.

Also, @Sineos, feel free to submit your own PR with ADXL-related suggested changes ;)

dmbutyugin avatar Oct 19 '22 18:10 dmbutyugin

Also, @Sineos, feel free to submit your own PR with ADXL-related suggested changes ;)

I could and would have done so already but I do not feel comfortable with the real name sign off. My personal policy is to keep my real name off the internet as much as possible. So no facebook, social media, extensive use of VPNs, etc. Maybe I'm just too old, too paranoid, whatever. Not going to change this for a few lines of documentation PRs 😄

@dingyifei take what you deem suitable and thanks for the effort put in!

Sineos avatar Oct 20 '22 05:10 Sineos

I think everything is getting a bit messy and hard to keep track of. I have modified the PR according to most suggestions. I know many people got ADXL 345 working with STM and other chips so I just added a line to mention the possibility.

I think we should merge everything in its current state.

dingyifei avatar Oct 26 '22 08:10 dingyifei

@dingyifei Thanks! I took a look and overall looks good! Just a couple of notices / suggestions:

I know many people got ADXL 345 working with STM and other chips so I just added a line to mention the possibility.

The main issue there is voltage level compatibility. Many MCU boards only have 5V available, and then depending on whether the accelerometer has a logic level shifter and whether the SPI pins are 5V tolerant or not it might or might not be safe to connect. That's why it wasn't suggested by default. But overall, what you wrote is actually a reasonable middle ground. If possible, I would highlight the issue:

**Note: Some MCUs will work with an ADXL345 in SPI mode (e.g. Pi Pico), wiring and
configuration will vary according to your specific board and avaliable pins.
Double-check the acceptable and actual voltage levels on both the MCU board and
the accelerometer for all pins before proceeding.**

Another small comment is that your wiring diagram for Pico states i2c0a bus, but the sample configuration - i2c1a. I would make it consistent and have the same bus in both cases (I don't care which one specifically).

I think we should merge everything in its current state.

I'll give @Sineos a chance to take another look at the text, but modulo these couple of tiny suggestions, I agree.

dmbutyugin avatar Oct 26 '22 10:10 dmbutyugin

LGTM

Sineos avatar Oct 26 '22 10:10 Sineos

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards, ~ Your friendly GitIssueBot

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

github-actions[bot] avatar Nov 16 '22 12:11 github-actions[bot]

Thanks. Sorry for the delay. I've now merged this PR.

-Kevin

KevinOConnor avatar Nov 19 '22 14:11 KevinOConnor