[Proposal] I2cController and II2cController

[dotnet]

[Proposal] I2cController and II2cController

[shaggygi]

@joperezr @krwq Proposal was updated above. Please review when you get a few minutes. Thx

[shaggygi]

@joperezr I think we're on the same page. I'm thinking (and beginning to document in proposal above in section 3) we could have a ShouldDispose for both controller and devices. This would allow the various scenarios of when/what to dispose. It would be more of a default value of true as that would be what is expected in most cases. I like having as just a default value instead of passing OpenDevice method, but seems like it would work either way.

public bool ShouldDispose { get; set; }

// For controller.
// With property default to true.
var controller = new I2cController();
// Or include optional parameter for constructor.
var controller = new I2cController(bool shouldDispose = true);

// For device
// With property default to true.
controller.OpenDevice(settings);
// Or include optional parameter for open method.
controller.OpenDevice(settings, bool shouldDispose = true);

Still pondering.

[shaggygi]

Referencing #126

[joperezr]

Agreed that we need this. The next steps are to have this issue be a formal Api proposal so that the board can review it, and approve it so that we can submit a PR.

[joperezr]

We should also include an SPIController on this proposal as well, as they are related and will be very similar.

[shaggygi]

@joperezr If the plan for I2C/SPI controllers is to mainly determine best device to use for platform, I still think this older PR is still valid. Thoughts?

[shaggygi]

@joperezr Since this is labeled as 'up-for-grabs', please assign to me. I came up with some good thoughts this week and currently working on proposal notes to add here soon.

I originally opened this issue specifically for I2C and another SPI (#119), but we later thought it could be combined. My proposals will be big enough to justify separate reviews and implementations, so I renamed issues back for individual specifics. Thx

[shaggygi]

After getting into development and the weeds for this proposal, I needed to make a slight change to include the bus ID as a way to manage devices. This makes it one extra parameter, but believe it makes more sense while comparing to PwmController (chip/channel) and how the SpiController (bus ID/chip select line) might work. In addition, this change makes it easier to work with a device that contains multiple buses (e.g. RPi3, TCA9548A, etc.). The proposal and WIP PR has been updated to reflect change.

[krwq]

@joperezr I think we should address this as part of 3.0, especially if @shaggygi is willing to help fix this

[joperezr]

In general the proposal looks great thanks a lot for working on this @shaggygi

Here are some thoughts on the above proposal:

1. Regarding the II2cController interface members:

    void AddDevice(int busId, int address);
    void AddDevice(I2cDevice device);
    void RemoveDevice(int busId, int address);
    void ClearDevices();

I believe that in order to echo a bit more of what we are doing with the other controllers (Gpio and PWM) we should instead use Open and Close instead of Add and Remove, just so that it feels that the API is familiar with the rest of the library. Also, for the Open/Close methods I would only have two overloads, one that takes the I2cDevice, and one that takes the I2cConnectionSettings (meaning we should remove the one that takes busId and address).

2. Regarding the constructors on II2cController:

// Constructor overloaded
public I2cController(int busId, int address)
public I2cController(I2cConnectionSettings settings)
public I2cController(I2cDevice device)

I don't like these very much, as it might be a bit confusing to have I2cControllers take similar parameters to what an I2cDevice constructor would. Think of this similar to what the Collections classes in the BCL usually look like, usually you would first initialize a List for example, and then call the Add methods later, instead of passing a single member of the list to the list constructor. For this, I would just keep a parameter-less constructor, and simply call the Open/Close in order to add/remove I2cDevices. What do you think?

3. Regarding static method:

public static I2cDevice CreateDevice(I2cConnectionSettings settings)

I'd say that there is no big need for this one, given that the Open method that will take ConnectionSettings will effectively do this(this meaning picking the right device for the OS), so there is no big need for another static method that does the same thing.

4. Open question. I'm wondering how should we handle the case where somebody creates an I2cDevice, and then calls the OpenDevice method on a controller passing in connection settings instead of the I2cDevice instance. In this case, there would now effectively be 2 different I2c device instances, and you might find the case where one disposes the other. Speaking of which, did you give some thought into whether or not the I2cController would dispose the I2cDevices in it when calling dispose()? If so, perhaps we will want to provide an API to override this behavior if necessary like:

public class I2cController
{
  //...
  public bool ShouldDisposeDevices { get; set; }
  //...
}

What do you think?

[shaggygi]

@joperezr Thanks for the feedback. Here are some notes.

1. I sort of like Add/Remove better, but it is better to keep similar to the other controllers. I updated to Open/Close.
2. I was on the fence on adding parameters to constructors, but was thinking it'd make it easy to quickly add a device while creating the controller. We don't add pins for the GPIO/PWM controllers, so I removed to keep consistent.
3. Thanks for explaining and pointing out the partial classes for each OS (wasn't thinking about that approach). This makes more sense and better than the interop code. I removed.
4. Disposing is a good point and not sure. I know there are chats going on in other areas, so maybe this fits with those talks. I'll try to ponder a little.

Until next time... I'm cleaning up the changes to reflect the feedback. Should have an update on PR in a few. 👍

[shaggygi]

Note to self related to disposing...

What if we used ShouldDisposeDevices property as @joperezr mentioned above where if true, it would dispose all opened devices when disposing the controller.

We could add a 2nd ShouldDisposeDevice parameter for CloseDevice method. This could be an optional parameter and defulat to true. If set to false, the might want to continue use elsewhere?

Most of the time the intent is to dispose of the device(s) when disposing the controller as the app/process is probably shutting down.

If a binding is not provided a controller and creates the default along with opening its own devices, the dispose flag should be set to dispose controller and devices opened within.

[joperezr]

We could add a 2nd ShouldDisposeDevice parameter for CloseDevice method. This could be an optional parameter and defulat to true. If set to false, the might want to continue use elsewhere?

I like the idea, but the problem is that you might have the case where people will dispose the controller without closing the device. In this case, should the device be disposed? One alternative is to have that optional parameter of shouldDispose passed in when calling OpenDevice, that way you could even pick which devices in the controller would get automaticly disposed, and which ones will be preserved. What do you think?