Skip to content

Conversation

@jgromes
Copy link
Contributor

@jgromes jgromes commented Nov 17, 2019

Arduino cores offer more than one I2C interface (e.g. STM32). I added the option to use any TwoWire instance as the communication interface - it's set to Wire by default, so existing code shouldn't be affected.

@lewispg228
Copy link
Contributor

Hi @jgromes , Thank you for your pull request! We really appreciate you helping to update this library.

We have some hardware on order and should be able to test this out in the next couple days. We will get back to you asap.

With our initial look through of your changes, we noticed that there are some other things that could also be updated. With each new product, we are always trying to make our libraries stronger, cleaner and more consistent. A couple things we noticed were:

  1. In our more recent libraries, we have been allowing the user to set the device I2C address and the wire port as arguments sent to the begin function. Note, we now always put the address as the first argument and then the wire port as the second argument. A good example of this is shown in our ADS1015 library here:

https://github.com/sparkfun/SparkFun_ADS1015_Arduino_Library/blob/master/src/SparkFun_ADS1015_Arduino_Library.cpp#L45

There was also a recent blog post about setting up two I2C ports. I thought you might like to check that out too:

https://www.sparkfun.com/news/3113

  1. We don't use the Wire.begin() function inside the library. We require that it be called inside the Arduino Sketch examples. We have found that this helps avoid clock speed resetting and some instability issues.

If you are comfortable/willing to add in these other minor tweaks, that would be much appreciated.

Either way, we will get back to you after our hardware testing is complete, and your current PR will be merged as is if all goes well.

Thanks again!

@jgromes
Copy link
Contributor Author

jgromes commented Nov 20, 2019

Hi @lewispg228,

Ad 1. I'm not sure it's a good idea to have it in the begin method - if someone tries to implement similar feature for SPI, he will run into ambiguity issues between the two (or more) begin methods. Constructor would seem like a more appropriate place to put this - I can't think of a reason why would you need to change the interface at runtime.

Ad 2. That will break compatibility with existing code, something I specifically wanted to avoid. To be honest, the whole library would deserver a rewrite using some sort of abstraction layer to handle the two possible interfaces - deciding which interface to use based on a value of internal variable seems dodgy. Unfortunately, I don't have the time to do that now, sorry.

-begin now accepts three arguments (accel/gyro address, mag address, i2c port). Note, this is used for I2C.
-created beginSPI(). Use this now to use SPI. Requires arguments for CS pins.
-removed Wire.begin() from inside library. It is now required that users call this in their Arduino sketches.
-adjusted all examples to use new functions and added Wire.begin().
-removed constructor with arguments for interface_mode and addresses. This is now handled in begin functions.
-FYI, I verified everything (including trying out a different wire port) on an artemis redboard.
@lewispg228
Copy link
Contributor

Hi @jgromes ,
I put some time into this today, and I think I have the solution we want to go with.

As a reference, I looked at our BNO080 library. It has both I2C and SPI. I followed what this library does, which is have separate begin functions: one for I2C (begin), and a second for SPI (beginSPI).

https://github.com/sparkfun/SparkFun_BNO080_Arduino_Library

Also, we do our best to avoid breaking old code, but unfortunately, it is sometime necessary. All of our more recent libraries require that Wire.begin() be called outside the library. And so in order to make this compatible with our larger ecosystem of I2C libraries, it was best to remove Wire.begin() from inside the library.

Thanks again for your pull request, and we appreciate your time put into this.

Please let us know if you have any questions or suggestions.

Thanks!!

@lewispg228 lewispg228 merged commit 97cc721 into sparkfun:master Nov 25, 2019
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