Iot: Reconsider Dispose pattern across devices

[krwq]

Multiple devices take SpiDevice/I2cDevice in the constructor and then dispose it later. We should reconsider if this is good approach and if disposing is always expected. Some alternatives:

• do not dispose the device and let the caller dispose it instead
• take bool flag (i.e. shouldDispose) and let the caller pick what they want

[joperezr]

Instead of taking a flag, I would rather have a bool shouldDispose property that would be defaulted to true, but you could set otherwise if needed.

[Frankenslag]

I like the 'shouldDispose' property... Certainly something is needed as multiple device bindings can use the same GPIO controller for example. I am not sure that it should be defaulted as true as I would generally prefer the 'caller dispose' model but I am undecided if it would be a good idea to change what seems to be happening already.

[krwq]

I'm personally in favor of not disposing at all (move the responsibility to the caller) and where Dispose ends up being empty (presumably most of the devices) I'd just drop interface implementation to make it clearer that we do not Dispose anything

[Gnbrkm41]

+1 to not disposing at all. Personally I think people will dispose of both the original disposable and the type holding the disposable anyway (that's what I do at least 😛 )

using (Stream stream = File.Open())
using (StreamReader reader = new StreamReader(stream))
{
    // Some nice codes come here
}

Like this. usings make them pretty easy to do if one has to dispose of both instances anyway.

I would rather have a bool shouldDispose property that would be defaulted to true

But I think this is a nice idea as well. Giving them some choice is better than no choice, I guess.

[joperezr]

I think people will dispose of both the original disposable and the type holding the disposable anyway

If they do this, it should be fine with the way we are writing the bindings and our devices, because calling dispose twice should just be a no-op the second time. I'd still argue that disposing them being the default would be better, since I believe that most of the time the I2c or Spi device won't be used other than to initialize the device binding, so it should be ok to assume that we call dispose. I do agree however, that we should provide a way for people to change a setting in order to not dispose when the main device binding gets disposed, for those very few cases where people would want this.

[krwq]

I believe that most of the time the I2c or Spi device won't be used other than to initialize the device binding

@joperezr I'd disagree - for simple tests of devices that will be true but any larger/commercial app will likely use multiple SPI devices and will probably try to reuse the same instance.

Most of the time device bindings don't dispose anything else than SPI/I2C device which suggests perhaps user should just dispose SPI/I2C directly and that devices should not implement dispose pattern

[Frankenslag]

I’m a novice here but I think we need to look at why Dispose is there and as I understand it then it is to release unmanaged resources (file handles for example) when the object is no longer required. So an object should dispose

• unmanaged members. Created within the object.
• managed members that are created within the object and they themselves contain managed resources and thus implement IDispose

Those items passed in from the outside are not created inside and thus should be disposed outside of the object in my opinion.

I actually don’t like the way that stream reader does things because it needs you to actually read the documentation to understand the dispose pattern.

I suppose I would be happy to have a parameter to specify the behaviour but I feel that the default should be to not dispose and then if you really want to get rid of the managed resources in a non explicit way then you can.

‘ sorry if this is rambling but it’s late here.....

[krwq]

To sum up this thread after talking to @tarekgh and @joperezr:

• I2cDevice and SpiDevice are bound to specific devices: (for I2C we talk to specific devices on specific addresses and for SPI we got specific CS pin assigned to specific device) which means one instance per one device instance
• GpioController can be used by multiple devices at the same time independently

Given that I think lifetime of SpiDevice/I2cDevice should finish with the device:

• always dispose
• optionally have a way of opting-out of disposing - need justified use-case
• might be a good idea to provide default device

For GpioController:

• devices should not dispose it by default
• there must be an option of opting-out if default is different
• might be a good idea to provide default controller

what do you think?

[joperezr]

That sounds good to me except for one caveat. GpioController's suggestion would be to always try to create and dispose within the code of the binding, since there is really no use/advantage of passing one in in the constructor. This eliminates the whole ownership problem, and it also allows you to still have a GpioController object outside of the binding's code.

[krwq]

@joperezr what about GPIO expanders such as i.e. https://github.com/dotnet/iot/tree/master/src/devices/Pca95x4? I wouldn't expect GpioController to ever get disposed and if you don't allow passing one in the constructor you cannot make i.e. character LCDs to work on expander

[joperezr]

That is a good point. To me what makes most sense with GpioController getting passed in(either a regular controller or a IGpioController) then we shouldn't dispose by default as you suggest. So then we are in agreement that SpiDevice and I2cDevice to do it by default and don't do it for GpioController when passed in.

[krwq]

@shaggygi if you're interested you can pick this one up. Plan is over here: https://github.com/dotnet/iot/issues/419#issuecomment-497916410 if not I can do a sweep here

[shaggygi]

@krwq lemme review over the weekend and get back with you. i'm sure i'll have questions.

[joperezr]

Regarding the plan that @krwq wrote above, specifically to:

For GpioController:

devices should not dispose it by default
there must be an option of opting-out if default is different
might be a good idea to provide default controller
what do you think?

I think that the default should be for devices to dispose but always to have a way to Opt out. Most common case for devices will be to take an optional IGpioController in case the consumer is using gpioexpander, and if not passed in, then the device should just initialize a new controller.

[shaggygi]

@krwq @joperezr I believe we should add the ShouldDispose property we previously discussed to GpioController, PwmChannel, I2cDevice, and SpiDevice. This would make it consistent across all types. Use the #419 (comment) as reference.

Here are a few notes that will help make a decision before I begin a PR.

always dispose (under I2C/SPI)

I was planning to update the Pca9685 to use some of the recent PwmChannel changes. It threw a wrench into initial thinking by deciding not to have a PwmController in core API. This binding could have a PwmChannel implementation and also utilize the ServoMotor binding.

I2cDevice i2cDevice = I2cDevice.Create(...);  // Create I2C device to use with all servos via Pca9685.

// Create servo 1.
PwmChannel pwmChannel1 = Pca9685.CreatePwmChannel(i2cDevice, ...); // or something similar.
var servo1 = new ServoMotor(pwmChannel, new ServoMotorDefinition(540, 2470));

// Create servo 2.
PwmChannel pwmChannel2 = Pca9685.CreatePwmChannel(i2cDevice, ...); // or something similar.
var servo2 = new ServoMotor(pwmChannel, new ServoMotorDefinition(540, 2470));

// do servo stuff...

In some cases, a user might want to only dispose of specific servos and keep the others. Therefore, you couldn't dispose of the I2C device. The default should be to set the ShouldDispose flag to false and allow the caller to dispose I2C device in cleanup after all servos are disposed.

I2cDevice i2cDevice = I2cDevice.Create(...);  // Create I2C device to use with all servos via Pca9685.
i2cDevice.ShouldDispose = false;

// create/do channel/servo stuff like above.

servo1.Dispose(); // Will displose channel, but not I2C device.
servo2.Dispose(); // Will displose channel, but not I2C device.
i2cDevice.Dispose();

Bindings

Since most bindings will use the default devices/controllers, the ShouldDispose property should be initally set to true to limit repeated binding code, make consistent, and help from being forgotten to do so.

Examples

// Device defined and will dispose by default.
I2cDevice i2cDevice = I2cDevice.Create(I2cConnectionSettings settings);

var ssd1306 = Ssd1306(i2cDevice);
// do stuff...
ssd1306.Dispose(); // dispose I2C device.

// Device defined and will not dispose by default.
I2cDevice i2cDevice = I2cDevice.Create(I2cConnectionSettings settings);
i2cDevice.ShouldDispose = false;

var ssd1306 = Ssd1306(i2cDevice);
// do stuff...
ssd1306.Dispose(); // will not dispose I2C device.
// do other stuff...
i2cDevice.Dispose();

// Device not defined, so create default with a default dispose.
var ssd1306 = Ssd1306(I2cConnectionSettings settings);
// do stuff...
ssd1306.Dispose(); // dispose I2C device.

devices should not dispose it by default (under GpioController)

I can see where GpioController is used across multiple bindings and would not want to dispose. However, most apps/samples will only be using one binding. If mulitple devices are used with controller, it would be up to caller to set ShouldDispose to false making it consistent across other types. Therefore, a GpioController would be the same as above.

// GPIO controller not defined, so use default.
var myBinding = new MyBinding();

 myBinding.Dispose(); // dispose controller.

// GPIO controller is defined and set to not dispose.
var gpioController = new GpioController();
GpioController.ShouldDispose = false;

var myBinding1 = new MyBinding(..., gpioController); // with other pin definitions.
var myBinding2 = new MyBinding(..., gpioController); // with other pin definitions.

// do stuff...
myBinding1.Dispose(); // will not dispose controller.
// do other stuff...
myBinding2.Dispose(); // will not dispose controller.

gpioController.Dispose();

Summary

To summarize....

GpioController, PwmChannel, I2cDevice, and SpiDevice would have a ShouldDispose property added with a default value of true.

It would be up to the caller to set the ShouldDispose property false if they do not want the device/controller to be disposed when the binding is disposed.

If a device/controller is not provided to a binding and it creates a default device/controller within, the ShouldDispose property would be initially true and be disposed when the binding is disposed.

[krwq]

I feel like this property is a bit weird: it's not particularly related to any device and it's just implementation detail leaking out. I'd prefer we talked with some API review folks first

[krwq]

After offline conversation with @bartonjs and @joperezr (separately) seems mostly conclusions from https://github.com/dotnet/iot/issues/419#issuecomment-497916410 still stand.

One thing is that opt-out of Dispose for I2C/SPI is currently not well justified since we have no actual use case to re-use SPI/I2C device.

The other thing is we considered for GpioController to not do anything in the Dispose or even remove it at all since there is nothing really useful there to clean up and Closing/Resetting any values is not actually always expected (see i.e. scenario described here https://github.com/dotnet/iot/issues/347#issue-425716555) and we should not dispose it.
Having constructor arg like leaveOpen or shouldDispose is ok but not recommended since it unnecessarily pollutes the code and have very very little use case (basically only scenario when you have exactly one device using GPIO and you actually need a cleanup: most of the time it doesn't seem to be needed at all - even when it is usufeul to dispose it still only doesn't even save too many characters when typing and rather makes it confusing for anyone else).

[joperezr]

(basically only scenario when you have exactly one device using GPIO and you actually need a cleanup: most of the time it doesn't seem to be needed at all

This is not necessarily true. You can have multiple devices, each with their own GpioController instance, and they each cleanup the pins that they use when they are done. I can probably agree that returning the PinValue of a Gpio pin during cleanup to a default might not be ideal so we could change that, but other cleanup like closing the pins that were opened are things that I believe must still be done.

[joperezr]

The part that is tracked by 3.0 of this issue is to have clear documentation on how this should work. Actually moving all device bindings to follow this guidelines is not part of 3.0

[krwq]

One thing to add here, I have previously mentioned that GpioController should not be disposed but I'm no longer convinced about that since @joperezr reminded me that each GpioController can open pins separately. I'm currently unsure what the guidelines should be about it. There are couple of options I can think of from the top of my head:

• always dispose and always pass ownership of new instance of GpioController to the device using it
• allow not to dispose (shouldDispose flag) and allow re-using instances

I feel for clarity we should recommend first option but for very special cases where perf might be an issue (which I currently can't think of any such cases since 1 instance per device doesn't sound particularly scary from memory perspective) we might allow exceptions - I would not include that in the guidelines and rather consider case by case.

Does that sound reasonable?

[joperezr]

Sounds reasonable to me.

[joperezr]

Moving to future since guidelines have been clarified via #673 (thanks @krwq!)