Skip to content

Conversation

nseidle
Copy link
Member

@nseidle nseidle commented Dec 16, 2019

I believe the primary issue with with getting the mag to work in original library was two fold: the bank bits were not being set correctly, and bank3 was not set in ICM_20948_i2c_master_configure_slave().

Data from mag is now harvested at the same time at gyro and accel from EXT_SLV_SENS_DATA_xx registers.

  • Add i2cMasterReset() - Needed to reset the I2C master during startup in the case that it has hung due to an incomplete previous transaction with Mag.
  • Remove all I2C based mag functions. Move to global ICM class.
  • Remove _has_magnetometer flag. No longer needed. If mag doesn't come online, data returned in 0 in DMP array.

Tested on Artemis via SPI/I2C and Uno SPI/I2C.

Nathan Seidle added 4 commits December 13, 2019 10:00
I believe the primary issue with with getting the mag to work in original library was two fold: the bank bits were not being set correctly, and bank3 was not set in ICM_20948_i2c_master_configure_slave().

Data from mag is now harvested at the same time at gyro and accel from EXT_SLV_SENS_DATA_xx registers.

* Add i2cMasterReset() - Needed to reset the I2C master during startup in the case that it has hung due to an incomplete previous transaction with Mag.
* Remove all I2C based mag functions. Move to global ICM class.
* Remove _has_magnetometer flag. No longer needed. If mag doesn't come online, data returned in 0 in DMP array.
@ruffner
Copy link
Contributor

ruffner commented Dec 16, 2019

Maybe squash these commits and fix-up messages?

I will test this on an ICM-20948 board I've made and report back

@nseidle
Copy link
Member Author

nseidle commented Dec 16, 2019

Thanks Matt! I didn't expect anyone but @oclyke to have a look but I'd love the review and feedback.

@oclyke oclyke self-requested a review December 16, 2019 22:43
@oclyke oclyke self-assigned this Dec 16, 2019
Copy link
Contributor

@oclyke oclyke left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good. Only caught off guard by the use of a pointer to send a single byte and fighting an internal struggle with VSC's whitespace preferences

@@ -1,251 +1,347 @@
#include "ICM_20948_C.h"
#include "ICM_20948_REGISTERS.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is me picking nits - these whitespace changes make it hard to see if any significant code changes happened in the C implementation.

It's a personal thing but I'm not a big fan of VSC's style when it comes to automatic formatting.

I wonder how much effort it will take to revert the whitespace changes w/o undoing any important code changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for the C.h file but I'll keep the comment here

Stick with pass-by-value in i2cMasterSingleW
@ruffner
Copy link
Contributor

ruffner commented Dec 17, 2019

just realized this was spi only - i am using i2c.

@oclyke
Copy link
Contributor

oclyke commented Dec 17, 2019

@ruffner It should apply to both interfaces. Previously the magnetometer could only be accessed when connected to the ICM over I2C because it relied on the I2C passthrough feature. This PR enables the I2C master feature that allows the ICM to handle I2C transactions with other sensors - including the magnetometer chip. This PR also makes that the default mode of accessing the magnetometer so that it is used whether you are talking to the ICM over SPI or I2C

@ruffner
Copy link
Contributor

ruffner commented Dec 17, 2019

@oclyke got it - the 'Advanced' example seems to work well once I add myICM.startupMagnetometer(); to the sketch, at the end of setup().

@oclyke oclyke merged commit 9a36921 into master Dec 19, 2019
@oclyke oclyke deleted the addSPIMag branch December 19, 2019 18:41
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.

3 participants