-
Notifications
You must be signed in to change notification settings - Fork 594
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
Add Tca955x device family #2374
base: main
Are you sure you want to change the base?
Conversation
@dotnet-policy-service agree |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Couple of comments after a first quick review. Thanks!
src/devices/Tca955x/README.md
Outdated
The `Tca955x` has one interrupt pin. The corresponding pins need to be connected to a master GPIO controller for this feature to work. You can use a GPIO controller around the MCP device to handle everything for you: | ||
```csharp | ||
// Gpio controller from parent device (eg. Raspberry Pi) | ||
_gpioController = new GpioController(PinNumberingScheme.Logical); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a detail: we want to get rid of the PinNumberingSchemebecause no one use it and it's hard to maintain. So, better take it out of the parameters as it's anyway the default.
src/devices/Tca955x/Tca9554.cs
Outdated
/// The device address for the connection on the I2C bus. | ||
/// Start with 0x20 and ends with 0x27 | ||
/// </param> | ||
public Tca9554(I2cDevice bus, int interrupt = -1, GpioController? gpioController = null, bool shouldDispose = true, byte adress = 0x20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than a default 0x20 like this, can you add a public constant with DefaultI2cAddress and reuse that one? (see other I2C binding for the pattern)
src/devices/Tca955x/Tca955x.cs
Outdated
|
||
private bool _shouldDispose; | ||
|
||
private byte _adress = 0x20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned before a public const would be better
src/devices/Tca955x/Tca955x.cs
Outdated
if (adress < 0x20 || | ||
adress > 0x27) | ||
{ | ||
new ArgumentOutOfRangeException(nameof(adress), "Adress should be in Range 0x20 to 0x28"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess 0x27 regarding the code
src/devices/Tca955x/Tca955x.cs
Outdated
_interrupt = interrupt; | ||
_adress = adress; | ||
|
||
if (adress < 0x20 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can then use the const and a +7 if that's the range. A few less magic numbers then.
src/devices/Tca955x/Tca955x.csproj
Outdated
|
||
<ItemGroup> | ||
<Compile Include="*.cs" /> | ||
<Content Include="category.txt" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can put it as none as well I guess
@@ -0,0 +1,3 @@ | |||
# TODO: This needs to be determined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just make it simple :-) typically it's the usage you can add like for the main README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankd, looks really good now!
Add support for the device Tca9554 and Tca9555.
This PR introduces support for the TCA9554 and TCA9555 devices, which are simple I2C IO extensions (8-bit and 16-bit, respectively). The implementation in
Tca955x.cs
is based on the existingMcp23xxx.cs
class.Datasheets:
Microsoft Reviewers: Open in CodeFlow