-
Notifications
You must be signed in to change notification settings - Fork 571
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
Reconsider Dispose pattern across devices #419
Comments
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. |
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. |
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 |
+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.
But I think this is a nice idea as well. Giving them some choice is better than no choice, I guess. |
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. |
@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 |
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
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..... |
To sum up this thread after talking to @tarekgh and @joperezr:
Given that I think lifetime of SpiDevice/I2cDevice should finish with the device:
For GpioController:
what do you think? |
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. |
@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 |
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. |
@shaggygi if you're interested you can pick this one up. Plan is over here: #419 (comment) if not I can do a sweep here |
@krwq lemme review over the weekend and get back with you. i'm sure i'll have questions. |
Regarding the plan that @krwq wrote above, specifically to:
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. |
@krwq @joperezr I believe we should add the Here are a few notes that will help make a decision before I begin a PR.
I was planning to update the Pca9685 to use some of the recent 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 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(); BindingsSince most bindings will use the default devices/controllers, the 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.
I can see where // 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(); SummaryTo summarize....
It would be up to the caller to set the If a device/controller is not provided to a binding and it creates a default device/controller within, the |
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 |
After offline conversation with @bartonjs and @joperezr (separately) seems mostly conclusions from #419 (comment) 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 #347 (comment)) and we should not dispose it. |
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. |
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 |
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:
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? |
Sounds reasonable to me. |
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:
The text was updated successfully, but these errors were encountered: