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 TCA9548A device binding #1908

Merged
merged 20 commits into from
Nov 3, 2022
Merged

Conversation

asheesh1996
Copy link
Contributor

@asheesh1996 asheesh1996 commented Aug 4, 2022

Added support for TCA9548A multiplexer

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Aug 4, 2022
@dnfadmin
Copy link

dnfadmin commented Aug 4, 2022

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@pgrawehr pgrawehr left a comment

Choose a reason for hiding this comment

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

Thanks for the addition!

I have some minor comments and a suggestion.

src/devices/Tca9548a/Tca9548A.cs Outdated Show resolved Hide resolved
src/devices/Tca9548a/Tca9548A.cs Outdated Show resolved Hide resolved
src/devices/Tca9548a/Tca9548A.cs Outdated Show resolved Hide resolved
src/devices/Tca9548a/Tca9548A.cs Outdated Show resolved Hide resolved
src/devices/Tca9548a/README.md Outdated Show resolved Hide resolved
src/devices/Tca9548a/README.md Outdated Show resolved Hide resolved
src/devices/Tca9548a/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pgrawehr pgrawehr left a comment

Choose a reason for hiding this comment

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

That was a good idea, but unfortunately, it fails in some use cases we have to consider.

Please ping me if you want me to make a suggestion to fix it. Of course, you can try yourself first.

src/devices/Tca9548a/README.md Outdated Show resolved Hide resolved
src/devices/Board/I2cBusExtensions.cs Outdated Show resolved Hide resolved
@ghost ghost added Needs: Author Feedback We are waiting for author to react to feedback (action required) and removed Needs: Author Feedback We are waiting for author to react to feedback (action required) labels Aug 6, 2022
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

Thanks, nice addition, few comments

src/devices/Tca9548a/Tca9548A.cs Outdated Show resolved Hide resolved
src/devices/Tca9548a/Tca9548A.cs Outdated Show resolved Hide resolved
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

Really nice device. I think we can further improve usability though. See my comment above

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

We're heading in the good direction - this is pretty close now.

  • I2cDevice implementation looks good, I'd recommend to make this class internal though (unless it offers anything extra on top of I2cDevice)
  • Tca9548A - CreateI2cDevice implementation looks good (arguably int might be easier to use rather than Channel)
  • Tca9548A - I'll recomend to also create CreateI2cBus method - I2cBus would only call to Tca9548A internal methods but nothing special other than that
  • Tca9548A - you might want to make everything but CreateI2cBus and CreateI2cDevice and basic functionality internal
  • you might also want to add slight improvement to not switch channel if it's already on the right one to avoid unnecesarilly writing to parent I2cDevice

Note that once this implementation provides I2cBus/I2cDevice we will be able to combine muxes together - that's the biggest benefit of doing this work.

@pgrawehr
Copy link
Contributor

pgrawehr commented Sep 1, 2022

One question: Are there similar devices available that would use the same binding (Eg is there a TCA9548b)?

Because if so, the namespace should be more generic. (I'm not asking to implement those devices now, it's just about considering future extensions)

@pgrawehr
Copy link
Contributor

If I read https://www.ti.com/interface/i2c/switches-and-multiplexers/products.html?keyMatch=TCA9548 correctly, there are other, very similar devices with 2 or 4 channels. The naming is [P|T]CA954x. So at least the last digit of the namespace should be changed to x. Not sure about what to do with the lead character, as it can be P or T.

@krwq
Copy link
Member

krwq commented Oct 24, 2022

@pgrawehr pick one, we should put all possible names in the README.md in the title so it's easier to find

@pgrawehr
Copy link
Contributor

I have updated the project name and the namespace to TCA954x, since there are several similar chips available. No functional changes otherwise, therefore to actually support e.g. TCA9544A, additional changes (and probably an additional derived type) will be necessary.

Removed some duplicate API methods
@pgrawehr
Copy link
Contributor

@asheesh1996 I cleaned up the documentation and the API slightly, but hopefully I have not changed the behavior (I have no way of testing). There were several methods that exposed the same functionality twice (e.g. the SwitchTCA9548AState method was removed, because the same can be achieved by calling SelectChannel with an empty set). Unless you have some objections, I think this is ready to merge now.

/// <param name="i2cDevice">The I2C Device of the Mux itself</param>
/// <param name="shouldDispose">true to dispose the I2C device at dispose</param>
/// <exception cref="ArgumentNullException">Exception thrown if I2C device is null</exception>
public Tca9548A(I2cDevice i2cDevice, bool shouldDispose = true)
Copy link
Member

Choose a reason for hiding this comment

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

Outside of this PR: @pgrawehr and @krwq we should discuss the I2cDevice disposal in bindings. Originally, we said we won't dispose them. because they have not being created in the binding. Now, there are quite some binding disposing them.
We have something similar for GpioController with a shouldDispose pattern. So maybe to be clear as well, like here but with a _shouldDispoeI2c, we may bring a bit of clarity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, good point. I think, most of the time this is just confusing for the user.

Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

Few remarks, otherwise, all good for me.

Tca9548AChannelBus must not call I2cDevice.Create(), because
that is a static method that always returns a default bus.
@asheesh1996
Copy link
Contributor Author

No issues, thanks for refactoring the code base :)

@pgrawehr pgrawehr merged commit 893ed04 into dotnet:main Nov 3, 2022
@pgrawehr
Copy link
Contributor

pgrawehr commented Nov 3, 2022

@asheesh1996 Thanks again for this contribution!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants