Unify platform specific implementations of I2cDevice/SpiDevice into one class

[krwq]

• Make WindowsI2cDevice/LinuxI2cDevice single non-abstract class I2cDevice
• I2cDevice.Create => new I2cDevice()

open questions:

• should we have II2cDevice interface or rather have I2cDevice be unsealed
  • personally leaning toward no interface because we won't be able to add any APIs to I2cDevice after we do that and ship - on the other hand lack of clear abstraction is weird - perhaps we should have I2cDevice as is and have GenericI2cDevice
• should we do similar changes for drivers (libgpiod/sysfs/...)

cc: @joperezr @shaggygi @ZhangGaoxing

[shaggygi]

How would we implement a GpioI2cDevice? Is this where GenericI2cDevice comes into play?

[krwq]

GpioI2cDevice/SoftI2cDevice assuming we don't pick the interface would derive from I2cDevice.

The only thing is that without GenericI2cDevice the abstraction for I2cDevice will be kinda weird (software implementation would not use anything from that class - it would just reuse the public surface).

On the other hand GenericI2cDevice is just awkward from naming and discoverability point of view (which feels we should have I2cDevice.Create which creates internal GenericI2cDevice).

[shaggygi]

I might be overlooking this new direction, but I'm thinking we keep the I2cDevice and update it based on your recent comments where it can determine whether its for Linux or Windows. This would implement II2cDevice.

Then the GpioI2cDevice/SoftI2cDevice could implement II2cDevice, as well.

We could pass II2cDevice to binding constructors similar to how we do IGpioController. We could do in 2 ways.

1. Pass in only the I2cConnectionSettings and the default could be used from I2cDevice using the provided settings.
2. Pass in II2cDevice where it could be non-nullable and caller would have to define or nullable and if null the default could be created.

Again, not completely sure.

[shaggygi]

Thinking a little more on this... why not consolidate to the following files...
NOTE: I changed all this locally and compiles, but have not tested.

I2c  
  Devices (can possibly get rid of this directory and place I2cDevice files back at I2c folder?)
    I2cDevice.Linux.cs   // Implements II2cDevice. Move all code from UnixI2cDevice to this file.
    I2cDevice.Windows.cs  // Implements II2cDevice. Move all code from Windows10I2cDevice to this file.
    II2cDevice.cs // Includes same method signatures as original I2cDevice.cs
  I2cConnectionSettings.cs (not changed; same location)

Delete the following files.
  UnixI2cDevice.Linux.cs
  UnixI2cDevice.Windows.cs
  Windows10I2cDevice.Linux.cs
  Windows10I2cDevice.Windows.cs

public class I2cDevice : II2cDevice

// Both flavors of I2cDevice.*.cs has static Create method, so it should still return II2cDevice for OS.
public static II2cDevice Create(I2cConnectionSettings settings) => new I2cDevice(settings);
var i2cDevice = I2cDevice.Create(settings);
// Can pass along to binding.

won't be able to add any APIs to I2cDevice after we do that and ship

I see your point on this. Not sure if the C# 8 default interface feature helps in this case.

[krwq]

I've talked with @bartonjs offline about the design and I believe we should go with more or less following direction (we are currently very close to that):

public abstract class I2cDevice
{
  public static I2cDevice Create(I2cSettings settings);
  // ideally we should move all non-generic settings like BusId outside and take as args
  // but considering this particular function should cover 99% use case it is acceptable if we leave it as is
  // we can re-iterate on that separately

  // software implementation can be added in the future i.e. like following:
  // public static I2cDevice Create(IGpioController controller, int sda, int scl, I2cSettings settings);
  // note that implementation does not have to be public
}

• interface is no-go for now - since we can't add anything to it in the future this will likely be a problem sooner than later
• Linux/WindowsI2cDevice should not be public unless we have a reason to make them public (same for other APIs) - it is easy to make API public but we won't be able to get rid of it in the future
  • as an implementation detail we can unify implementation into something like BoardSpecificI2cDevice which is internal (so name doesn't matter too much)

Assuming we went with proposed design *.I2c namespace would effectively have 2 types which does not justify making another sub-namespace for it (or in other words we should get rid of I2c.Devices namespace or at least it should not have anything public)

as an implementation detail layout can be similar to as proposed above (perhaps we can move I2cDevice outside of Devices but I don't have strong opinion either way).

[shaggygi]

This is how I interpret the direction...

• UnixI2cDevice and WindowsI2cDevice can be changed to internal instead of public. Since it will be called privately within API, we can remove UnixI2cDevice.Windows.cs and Windows10I2cDevice.Linux.cs files.

• We can create a future software implementation, but also keep it internal. It can be created similar to the following.

  // In I2cDevice.
  public static I2cDevice Create(IGpioController controller, int sda, int scl, I2cSettings settings)
  {
      return new SoftI2cDevice(controller, sda, scl, settings);
  }
  
  // In app.
  var i2cDevice = I2cCreate.Create(controller, sda, scl, settings);

• Above are the only 2 scenarios I can see users creating I2C connections. If one needs a new type, it could be an external API based on the I2cDevice allowing it to still be passed to bindings.

  public class MyI2cDevice : I2cDevice { ... }
  
  var i2cDevice = new MyI2cDevice(...);
  using (var binding = new Binding(i2cDevice)) { ... }

• I like the management of I2C device files within the Devices folder. However, I agree with @krwq extra *.Devices in namespace with this proposal. My vote is to place all I2cDevice related files under Devices folder but not include the *.Devices namespace. Meaning... namespace System.Device.I2c for all.

So basically the final structure would be...

I2c  
  Devices
    I2cDevice.cs                   // All classes have namespace of 'namespace System.Device.I2c'
    I2cDevice.Linux.cs
    I2cDevice.Windows.cs
    SoftI2cDevice.cs               // Future internal
    UnixI2cDevice.Linux.cs         // Internal
    Windows10I2cDevice.Windows.cs  // Internal
  I2cConnectionSettings.cs

[shaggygi]

One area this breaks is the DevicApiTester utility. I was able to update and compiles locally.

iot/tools/DevicesApiTester/Commands/I2c/I2cDriverType.cs

Line 10 in 833728c

public enum I2cDriverType

This shouldn't be a biggie though as the commands could be simplified by removing Device parameter and update CreateI2Device method to new I2cDevice.Create.

// Example command...
./DeviceApiTester i2c-detect -d Unix -b 1

// Would become...
./DeviceApiTester i2c-detect -b 1

[shaggygi]

Side question... since we are changing file names, would it be appropriate to go ahead and rename the following since it is based on WinRT? Just curious.

// From...
Windows10I2cDevice.Windows.cs

// To...
WinRtI2cDevice.Windows.cs

[krwq]

@shaggygi corefx convention would be (GenericI2cDevice is internal):
GenericI2cDevice.WinRT.cs

[kmate95]

Wouldn't making GenericI2cDevice internal and not providing an interface cripple future extensibility?
Like virtual devices in #78.

[shaggygi]

@kmate95 No, the plan is to continue having I2cDevice so you'll still be able to implement your own and pass to bindings. This internal approach would be more for creating and providing the defaults for 1) Linux/Windows and 2) software types using specified GPIO pins and controller.

var i2cDevice = I2cDevice.Create(settings);
var i2cDevice = I2cCreate.Create(controller, sda, scl, settings); // or similar

[kmate95]

@shaggygi Thanks for clarifying.

[krwq]

@shaggygi planning to pick it up or shall I go ahead with this?

[shaggygi]

@krwq I have a PR about ready to go. Gimme a couple of hours.

[shaggygi]

@krwq @joperezr PR started. Give it a look and let me know if this fits your thinking. If so, I should be about an hour away on updating the same for SPI files.

[krwq]

@shaggygi thanks! PR makes stuff simpler and is inline with what I'm thinking