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

Added support for MPU6500 basic functionality #581

Merged
merged 9 commits into from
May 17, 2022

Conversation

Bananamannn
Copy link
Contributor

Will need to be double checked using a MPU6050 as I don't have one on hand. However MPU6500 seems to work just fine

pio/src/iSpindel.cpp Outdated Show resolved Hide resolved
@mp-se
Copy link

mp-se commented May 6, 2022

You need to patch the MPU6050.cpp file as well or it will fail on this function; accelgyro.testConnection(). This code will work;

bool MPU6050_Base::testConnection() { return getDeviceID() == 0x34 || getDeviceID() == 0x38; // Allow both MPU6050 and MPU6000 }

@Bananamannn
Copy link
Contributor Author

You need to patch the MPU6050.cpp file as well or it will fail on this function; accelgyro.testConnection(). This code will work;

bool MPU6050_Base::testConnection() { return getDeviceID() == 0x34 || getDeviceID() == 0x38; // Allow both MPU6050 and MPU6000 }

I didn't want to make any changes to the standard MPU6050 repository so I tried to remove the "accelgyro.testConnection()" line in the main ISpindel.cpp file and replace it so that function was never used. Did I miss it somewhere else other than the repo?

@mp-se
Copy link

mp-se commented May 7, 2022

You need to patch the MPU6050.cpp file as well or it will fail on this function; accelgyro.testConnection(). This code will work;
bool MPU6050_Base::testConnection() { return getDeviceID() == 0x34 || getDeviceID() == 0x38; // Allow both MPU6050 and MPU6000 }

I didn't want to make any changes to the standard MPU6050 repository so I tried to remove the "accelgyro.testConnection()" line in the main ISpindel.cpp file and replace it so that function was never used. Did I miss it somewhere else other than the repo?

Sorry, my misstake. didnt look at the changes. just noticed that there was a call to that method in the code.

Copy link
Owner

@universam1 universam1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter : code formatter fails

@thegreatgunbantoad
Copy link
Contributor

thegreatgunbantoad commented May 8, 2022

Is it worth altering that if statement a bit by adding brackets so as to not rely on operation order i.e.:
if ( (id == 0x34) || (id == 0x38) )

@universam1
Copy link
Owner

Is it worth altering that if statement a bit by adding brackets so as to not rely on operation order i.e.: if ( (id == 0x34) || (id == 0x38) )

https://docs.microsoft.com/en-us/cpp/c-language/precedence-and-order-of-evaluation?view=msvc-170 parentheses are not necessary here

@thegreatgunbantoad
Copy link
Contributor

I know they aren't strictly needed, it's more of a good practise / style kind of thing, also for readability.

@Bananamannn
Copy link
Contributor Author

Linter : code formatter fails

I honestly have no idea what this is. I haven't changed this from your master branch. Any ideas why it's failing on here but not when I build it on my PC?

Copy link
Contributor Author

@Bananamannn Bananamannn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes made

@Bananamannn
Copy link
Contributor Author

Could I please get some advice on how to deal with the Linter fail, any pointer would be greatly appreciated. I honestly have no idea why it keeps failing.

https://github.com/universam1/iSpindel/runs/6349107436?check_suite_focus=true

@thegreatgunbantoad
Copy link
Contributor

Is the dark green stuff in the files changed bit the stuff the linter doesn't like? I can't find the results of the linter.
So possibly a space after the 2nd comma in main.yml line 3
and one less space before the if in ispindel.cpp line 974

@Bananamannn
Copy link
Contributor Author

It's in the link below. I added the linter to my fork for testing.
The red and green are the changes that i've made.

https://github.com/Bananamannn/iSpindel/runs/6418152846?check_suite_focus=true

image

@pppedrillo
Copy link
Contributor

pppedrillo commented May 15, 2022

@Bananamannn All you had to to to get rid of format errors is just execute "Format document" command in VS Code (assuming you have clang-format installed by default)

Capture

@Bananamannn
Copy link
Contributor Author

Bananamannn commented May 15, 2022

@Bananamannn All you had to to to get rid of format errors is just execute "Format document" command in VS Code (assuming you have clang-format installed by default)

Oh right, thanks for the info. I'll defiantly do it that way next time. I did it all through the Github editor this round.

Copy link
Contributor Author

@Bananamannn Bananamannn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed formatting. Linter now passes

Copy link
Owner

@universam1 universam1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Excellent @Bananamannn thanks for contribution

@universam1 universam1 merged commit 6772058 into universam1:master May 17, 2022
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.

5 participants