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 getters for multiple variables at the same time #56

Merged
merged 3 commits into from
Sep 23, 2020

Conversation

guihomework
Copy link
Contributor

As discussed here #55 , this PR adds getters for multiple variables at the same time, also avoiding extra data copy.

Example was tested on SPI1 on a teensy 4.0 but can be modified to test on a standard i2c bus that I cannot test right now immediately

Copy link
Collaborator

@PaulZC PaulZC left a comment

Choose a reason for hiding this comment

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

This is a nice PR - thank you. But it needs a few changes...
Please see the review comments.
Best wishes,
Paul

@@ -0,0 +1,163 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @guihomework ,
Thank you for submitting this PR.
Some changes are required...
We already have an Example16. Please change the title of this file to Example18-AccessMultiple.ino.
We have separate examples for I2C and SPI. Please change this example so it uses I2C. You can include a separate SPI example in the SPI\examples folder.
Please comment out the Teensy-specific setMOSI commands etc.. Please see the other SPI examples and use the same format - using generic SPI. It is OK to leave the Teensy code in - so long as it is commented.
I think you missed an '&'? (See comment below.
Thank you!
Paul

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nice PR. Great work!
Please make the suggested changes and I will be happy to merge it.
Best wishes,
Paul

}

// set up the SPI pins utilized on Teensy 4.0
SPI1.setMOSI(imuMOSIPin);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please comment these lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, please use the new SPI terminology:
We don't use the terms Master and Slave any more. We now use Controller and Peripheral. We would like all new code to use the new terms: CIPO and COPI.
https://www.oshwa.org/2020/06/29/a-resolution-to-redefine-spi-pin-names/

Copy link
Collaborator

Choose a reason for hiding this comment

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

(You cannot do anything about setMOSI but you can change imuMOSIPin to imuCOPIPin etc.)

SPI1.begin();

//Setup BNO080 to use SPI1 interface with max BNO080 clk speed of 3MHz
imu_initialized = myIMU.beginSPI(imuCSPin, imuWAKPin, imuINTPin, imuRSTPin, 3000000, SPI1); //changed from slaveCPin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please comment this line and add a line without the 3000000, SPI1 to make the example generic to SPI.

Serial.println(F("Gyro enabled, Output in form x, y, z, in radians per second"));
Serial.println(F("Rotation vector, Output in form i, j, k, real, accuracy"));
//Serial.println(F("Magnetometer enabled, Output in form x, y, z, in uTesla"));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be best to add a warning here that the IMU did not initialize (instead of doing it in the main loop?)?

@@ -485,6 +485,18 @@ float BNO080::getYaw()
return (yaw);
}

//Gets the full quaternion
//i,j,k,real output floats
void BNO080::getQuat(float &i, float &j, float &k, float &real, float radaccuracy, uint8_t &accuracy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot the & and capital A for &radAccuracy?

@@ -169,32 +169,38 @@ class BNO080
uint16_t parseInputReport(void); //Parse sensor readings out of report
uint16_t parseCommandReport(void); //Parse command responses out of report

void getQuat(float &i, float &j, float &k, float &real, float radAccuracy, uint8_t &accuracy);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment: I think you forgot the & for radAccuracy?

@guihomework
Copy link
Contributor Author

Hi @PaulZC thanks for reviewing and sorry for the bad PR, not looking carefully about the existing examples, not noticing there were SPI separate examples and not being aware that a renaming campaign is underway in the world of programming/electronics.

I hope I tackled all your remarks, but cannot test i2c or spi code in generic fashion. I re-tested with my teensy4.0 while uncommenting the required lines.

@PaulZC
Copy link
Collaborator

PaulZC commented Sep 18, 2020

Hi @guihomework ,
Don't worry! It is a very nice PR. It just needs a little "fine tuning".
I will merge and test this as soon as I can - but it might not be until Monday.
Best wishes - and thank you again,
Paul

@PaulZC
Copy link
Collaborator

PaulZC commented Sep 23, 2020

Thanks again for this PR - and for making the changes!
I am merging this into the release-candidate branch so I can test it.
I will merge into the master branch when we release v1.1.9.

@PaulZC PaulZC merged commit f416278 into sparkfun:release-candidate Sep 23, 2020
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