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

Optionally support other Wire objects. #45

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

KurtE
Copy link
Contributor

@KurtE KurtE commented Aug 1, 2020

Many processors, now days have more than one hardware I2C on them. Sometimes it is convenient to use a different I2C object like Wire1 or Wire2 instead of the default Wire object.

This change allows you to pass in a pointer to the desired I2C(Wire) object on the constructor. which defaults to Wire.

This change requires the main header file to include the wire.h header file.

Many processors, now days have more than one hardware I2C on them.  Sometimes it is convienent to use a different I2C object like Wire1 or Wire2 instead of the default Wire object.

This change allows you to pass in a pointer to the desired I2C(Wire) object on the constructor. which defaults to Wire.

This change requires the main header file to include the wire.h header file.
@KurtE
Copy link
Contributor Author

KurtE commented Aug 4, 2020

Looks like this is more or less the same as #13

Other than I did with the constructor, and @mjs513 did it on the begin. I was 6 of one and half of a dozen the other way so either would be good.

But as seeing that the earlier PR was several years ago. I am guessing it will be even harder to try to add additional functionality.

@ryantm
Copy link
Contributor

ryantm commented Aug 6, 2020

Hi, @KurtE.

We would prefer the Wire pointer be set via a separate function (instead of as a constructor argument), because:

  • we feel that functions are more readable
  • constructor arguments don't scale well: they can get confusing and unwieldy when you have more than two
  • it allows the Wire pointer to be changed dynamically

Sincerely,
Ryan Mulligan

@KurtE
Copy link
Contributor Author

KurtE commented Aug 10, 2020

Hi @ryantm - I can easily add a new method for this. probably a set and a get of the current one.
Do you have a suggestion on names? Something like ?

void setWire(TwoWire *theWire = &Wire);
TwoWire *getWire(void);

Although I don't have all of your sensors, At some point these should be added to your other similar libraries. like the VL53L1X and probably the VL6...

The question I have for myself and others, is there are several simple features that if I were to use this library I would like. I started implementing some of them, but I also see that they overlap with the other current PR #42

So I am trying to decide what the best approach would be to take.

@kevin-pololu kevin-pololu merged commit 9bc65ea into pololu:master Sep 24, 2020
@kevin-pololu
Copy link
Member

Hi, KurtE.

I went ahead and merged your changes along with some further revisions of our own (including making separate setBus() and getBus() functions instead of doing it through the constructor, like you and Ryan were discussing). Some of the changes were mostly stylistic, but we wanted this to be a good template for adding this feature to our other libraries, so we thought it was worth some extra effort to make the code better match our internal coding style and preferences. Thanks for your contribution!

Kevin

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