Added I2C Create method

[shaggygi]

Resolves #118

This adds new Create method to return I2cDevice based on OS app is executing.

[joperezr]

Thanks for putting this up @shaggygi. I left some comments on the proposal so lets talk about those first and once we are in agreement I'll go ahead and review this. From just a very quick look, some feedback would be that you don't really need the Interop file you added to check the current OS, since we currently build two System.Device.Gpio.dll implementations, one for Linux and one for Windows. When we are building for Linux, we will compile all files that don't end like *.Windows.cs and similar for windows, we will build everything but the *.Linux.cs. This means that you can add all the shared logic for the controller in a file called I2cController.cs and then simply add the Open methods that will create the actual I2cDevices on separate files like I2cController.Linux.cs. You can see an example of this in our GpioController : https://github.com/dotnet/iot/blob/master/src/System.Device.Gpio/System/Device/Gpio/GpioController.Linux.cs

[shaggygi]

@joperezr as you probably noticed, this is breaking now as I'm using some of the new C# 8 nullable stuff. Would it be possible to add the Nullable = enable to the main project?

Not sure what that would cause, but we might need to begin preparing the APIs based on that to get ahead of the game?

[shaggygi]

Added some notes.

[joperezr]

as you probably noticed, this is breaking now as I'm using some of the new C# 8 nullable stuff. Would it be possible to add the Nullable = enable to the main project?

Right, enabling this project wide will generate some warnings as today we are setting some values to null without annotating, so I wouldn't set this yet project-wide. For the mean time, what you can do is to add #nullable enable on the top of the .cs files where you are adding annotations so that the compiler would not complain and annotations will still be taken into account. Usually the pattern followed for annotating nulls, one would go file-by-file enabling nullable with #nullable enable and once all files are anotated, then you can go and remove all of those and enable this project wide by setting the following property on the csproj: <NullableContextOptions>enable</NullableContextOptions>

[shaggygi]

@joperezr @krwq my brain hurts 🤕 😄

I made a few more changes and removed WIP from PR title. Please give it a look. Be gentle, but I know there are things missed and improvements to be made.

[krwq]

@shaggygi I like the fact that you had a time to tackle on this issue but it feels to me like this API could be collapsed to just:

namespace System.Device.I2c
{
    public static class I2cController
    {
        public static I2cDevice Create(I2cConnectionSettings settings);
    }
}

Or just directly:

public class I2cDevice
{
  public static I2cDevice Create(I2cConnectionSettings settings);
}

I don't find much value in having the controller representation. Do you have any use cases where this would be useful?

[shaggygi]

@krwq

I don't find much value in having the controller representation. Do you have any use cases where this would be useful?

Maybe you're right and it is being over-engineered. I thought about it some more today while reviewing the TCA9548 binding:

The TCA9548 doesn't work how first thought and would be a little more complex to code or would require unnecessary writes for reading/writing selected I2C devices. So let's step back and try a scenerio.

// Create an I2C device used with Tca9548.
I2cDevice tca9548I2cDevice = I2cDevice.Create(new I2cConnectionSettings(1, 0x40));   // Will dispose when disposing Tca9548.
Tca9548 tca9548 = new Tca9548(tca9548I2cDevice, ...);

// Create another I2C device used with Mcp23008.
I2cDevice mcp23008I2cDevice = I2cDevice.Create(new I2cConnectionSettings(1, 0x20));  // Will dispose when disposing Mcp23008.
Mcp23008 mcp23008 = new Mcp23008(mcp23008I2cDevice, ...);

// Select the Mcp23008 that is hard-wired on channel 3 of Tca9548.
tca9548.Select(Channel.Three);

// At this point, the Mcp23008 should look like it is connected on the same bus as the Tca9548.
// So doing the GPIO commands are still using the default I2C work without having to interact with an I2C controller.
mcp23008.WritePin(1, PinValue.High);

I like the ShouldDispose property so you have option to use the same I2C device for multiple devices on the buses connected to the Tca9548. For example, you could have a Mcp23008 connected to channels 1-3 with the same settings. This would allow one of the Mcp23008 to be disposed, but allow the shared I2C device to continue working on the other 2 Mcp23008 devices.

I2cDevice mcp23008I2cDevice = I2cDevice.Create(new I2cConnectionSettings(1, 0x20),  false);  // Don't dispose when disposing of any Mcp23008.
Mcp23008 mcp23008_1 = new Mcp23008(mcp23008I2cDevice, ...);
Mcp23008 mcp23008_2 = new Mcp23008(mcp23008I2cDevice, ...);
Mcp23008 mcp23008_3 = new Mcp23008(mcp23008I2cDevice, ...);

tca9548.Select(Channel.One);
// Do work with mcp23008_1.
mcp23008_1.Dispose();  // Clean up other resources like GPIO pins if used.

tca9548.Select(Channel.Two);
// Do work with mcp23008_2.

mcp23008_2.Dispose();  // Clean up other resources like GPIO pins if used.
mcp23008_3.Dispose();  // Clean up other resources like GPIO pins if used.

mcp23008I2cDevice.Dispose();  // Finally releases I2C device.

If we end up not adding an I2cController, we should at least consider the following.

1. Add ShouldDipose property to I2cDevice with default value of true.

• Update: I previously stated to add to I2cConnectionSettings, but probably need to add to I2cDevice as if there was a future PwmDevice and GpioPin, they might not have a type of Settings. So keep in I2cDevice?

2. Bindings should continue to implement IDisposable.
3. The binding guidance should be to provide two constructors where one allows for I2cConnectionSettings (for default) and another allows for I2cDevice (for non-default types).

// For default.  Creates based on OS app executing.
Tca9548 tca9548 = new Tca9548(new I2cConnectionSettings(1, 0x40), ...);

// For non-default.
GpioI2cDevice gpioI2cDevice = new GpioI2cDevice(new I2cConnectionSettings(1, 0x40), ...);
Tca9548 tca9548 = new Tca9548(gpioI2cDevice, ...);

[krwq]

@shaggygi, IMO we should remove the shouldDispose overall and that would remove weird abstraction (and also it is still possible to add it in the future without breaking anything).

I feel like I2cSettings should not have any board specific things so specifically it should only have address and perhaps some other I2c generic stuff. I feel like everything else should have either specific implementation which takes settings and extra info: for SoftI2c that would be SDA/SCL pin numbers and GpioController, for hardware implementation it would be busId and settings

[shaggygi]

@krwq

public class I2cDevice
{
public static I2cDevice Create(I2cConnectionSettings settings);
}

I couldn't figure a way to use since I2cDevice is abstract class (without doing the which OS logic. So in local work I have it in I2cController.[OS].cs files.

And should Create be Open to follow your previous comment?

[krwq]

@joperezr I think the current version looks reasonable, we only got APIs we are sure we want to have which should be sufficient for 3.0. Do we need any process to push this through somehow?

[joperezr]

This looks good. It is true that we were overengineering it a bit, since while it makes a lot of sense to have more complex controllers for gpio and PWM, it probably doesn't make that much sense to have it for I2c and Spi. My only comment here is: should we have the Create static method in the I2cController? or simply just add it to I2cDevice itself?

[shaggygi]

or simply just add it to I2cDevice itself?

While I can't explain/justify some of the work we didn't add, I think having in controller gives us room to grow. I still believe there will be a need for II2cController/ISpiController one day and the default controllers could add future support.

Other than that, we can put the Create wherever. I guess we just need to make sure we want Create or Open as @krwq mentioned.

[joperezr]

I still believe there will be a need for II2cController/ISpiController one day and the default controllers could add future support.

I agree that we need an interface for I2c and SPI functionalities, but we could also just have II2cDevice and ISpidevice right?

[shaggygi]

I agree that we need an interface for I2c and SPI functionalities, but we could also just have II2cDevice and ISpidevice right?

I suppose it wouldn't be a problem.

[krwq]

My vote is to put Create under SpiDevice/I2cDevice. Anything else we can add later

[shaggygi]

Referencing: #505 (comment)

I know I'm overlooking something, but not sure how to proceed while moving under I2cDevice class.

[krwq]

@shaggygi does it work if you make it so that you have 3 copies of I2cDevice similarly as we have for other classes (make sure to mark class as partial):

• I2cDevice.cs - non-os specific version
• I2cDevice.Windows.cs - windows specific implementation
• I2cDevice.Unix.cs - Unix specific implementation

you will need to modify csproj and move the <Compile under correct <ItemGroup with appropriate conditionals (follow whatever we do for other os specific files)

[shaggygi]

@krwq comparing to other files we use *.[OS].cs convention on, they are sealed partial where I2cDevice is abstract.

[krwq]

just do partial without sealed (basically repeat all decorators just add partial everywhere)

[shaggygi]

Gotcha. It's the way VS and red squiggles trip me up and how this project is structure.

[krwq]

Thanks @shaggygi!

[joperezr]

Changes look good to me. There are still some questions around the design of I2C and SPI: Should I2cDevice and SpiDevice be abstract? should we remove the constructors from UnixI2cDevice and WindowsI2cDevice now in favor of the Create method? We can discuss that in an issue and come back and fix if there is anything that needs fixing.

[krwq]

@shaggygi planning to also do matching PR on SPI?

[shaggygi]

@krwq @joperezr i’ll be honest i didn’t necessarily like the outcome of this, but understand it was probably best to make it simpler than originally thought. Working with a team, especially remotely, is sometimes difficult to express and discuss goals effectively. But hey, that’s why we need gate keepers to keep us all inline. Thank you for this.

One thing I’ve definitely learned about OSS projects is you don’t always get what you want. But that is not what it’s supposed to be about. Even though sometimes frustrating, for me it’s about learning from others and trying to understand their point of view to improve my coding skills. I usually learn something new each PR and this one was no different. Again, thanks for taking the time while we worked on this.

Until next time... i already have the next PR setup for SPI Create like we did for I2C. I’ll push that soon. Once those are checked in, i can scan/update the bindings to make them check device type in samples.

Thx again 😄 👍

[shaggygi]

@krwq

planning to also do matching PR on SPI?

Added #527 just now.

[joperezr]

i’ll be honest i didn’t necessarily like the outcome of this, but understand it was probably best to make it simpler than originally thought.

Hey @shaggygi, just a thought, if you ever feel like some path we took is wrong you should feel free and empowered to say it and push for a fix. This project is as much yours as it is ours, hence we decided to make it open source 😄. I think I understand your sentiment, where you wanted all 4 protocols to feel like part of the same ecosystem, so having controllers for all 4 was a great way to do that. I think that the main question that @krwq and I raised here is that having a controller for Spi and I2c might be a bit of an overkill as those controllers wouldn't have much functionallity around them, and would really only serve as "Collections" of devices. There were also not many scenarios where having a collection of devices would be useful, as mostly all consumers will be working with device bindings and those would only take one device. That is kind of why we thought that perhaps it wasn't worth to create a Controller just for one method (Create), so putting it as a static method in the device made more sense. That said, I'm just explaining our reasoning, but it is totally fair for you to not agree with this, and if so, let's start a discussion to find the best design :)