Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IMU_USE_SPI_ICM45686 #37

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add IMU_USE_SPI_ICM45686 #37

wants to merge 6 commits into from

Conversation

vidma
Copy link
Contributor

@vidma vidma commented Feb 19, 2025

for #35 #36

@qqqlab
Copy link
Owner

qqqlab commented Feb 19, 2025

Hey @vidma, that starts to look like something!

Please submit the MPU9250 fix as a separate PR, then we can get that one in already. Apparently WAI 0x73 is MPU9255 and 0x71 is MPU9250. Did you test with a MPU9255 ?

@vidma vidma changed the title Add IMU_USE_SPI_ICM45686; Fix MPU9250 I2C ids Add IMU_USE_SPI_ICM45686 Feb 19, 2025
@vidma
Copy link
Contributor Author

vidma commented Feb 20, 2025

⚠️ FYI: "This library does NOT support multiple instances of ICM456xx."

@vidma vidma marked this pull request as ready for review February 20, 2025 12:37
@vidma
Copy link
Contributor Author

vidma commented Feb 20, 2025

@qqqlab ready for review.

@qqqlab
Copy link
Owner

qqqlab commented Feb 24, 2025

Hi @vidma

Thanks for your hard work on this, I really appreciate it. It looks fine to me, and I'll merge it. But, as I don't have this sensor I can't test it... So ideally: fly it before we merge it :-)

Currently madflight is single IMU only, so lack of multiple instances is not an issue.

I would still prefer that you copy-paste the fixed TDK sources into this PR:

  • To get rid of a dependency which all users of madflight will have to install, but which only applies for this particular sensor
  • To prevent madflight code from breaking because of future TDK lib changes
  • To move ahead without first waiting for your PR on the TDK lib to get approved
  • To optionally trim down the pedometer, gesture, etc. crud which is not needed here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants