Fixing missing GpioController and ShouldDispose by Ellerbach · Pull Request #1016 · dotnet/iot

Ellerbach · 2020-03-19T12:20:59Z

Fixes Add a default constructor on bindings to take GpioController #1013 and adding GpioController and ShouldDispose everywhere it was needed
Adding ShouldDispose as true in all the constructors
Adjusting all Dispose function to take into consideration the new behavior

joperezr · 2020-03-19T17:29:56Z

src/devices/CharacterLcd/LcdInterface.Gpio.cs

                }

                _controller = controller ?? new GpioController(PinNumberingScheme.Logical);
+                _shouldDispose = shouldDispose;


I would recommend to by default set this to false whenever the controller parameter wasn't null. So basically, if somebody passed in a controller, let's default shouldDispose to false.

The current behavior was that everything was disposed if you passed one GpioController. So the shouldDispose is then by default true.
Also this create consistency across all the bindings.

I would prefer false as well, but it would be a breaking change for existing bindings.
@krwq Your oppinion might be useful here.

One thing we should definitely do is to always dispose when controller is null (or throw an exception when user passes null and asks for not disposing).

Considering our current guidelines is to always dispose and to not re-use GpioController IMO we should always dispose but I'm not strongly opinionated. I think it's much easier to explain what this method does if we consistently dispose or not dispose - if we choose to not dispose then we should change guidelines to match.

OK, so let me adjust the code to:

dispose whatever the shouldDispose parameter is if gpioController is null

In case gpioController is not null, then follow the shouldDispose wish

Recommendatin is not to reuse GpioController but in some cases like when using FT4222 or a controller that can only be created once, you don't really have other options.

joperezr · 2020-03-19T17:33:38Z

src/devices/DCMotor/DCMotor.cs

        /// <param name="controller"><see cref="GpioController"/> related with operations on pins</param>
-        protected DCMotor(GpioController controller)
+        /// <param name="shouldDispose">True to dispose the Gpio Controller</param>
+        protected DCMotor(GpioController controller, bool shouldDispose = true)


You don't really need to default shouldDispose here since this is a protected constructor and we are only calling it from our subclasses.

Right, I'll adjust this one.

joperezr · 2020-03-19T18:33:45Z

src/devices/DCMotor/DCMotor.cs

        /// Connecting motor directly to GPIO pin is not recommended and may damage your board.
        /// </remarks>
-        public static DCMotor Create(int speedControlPin, int directionPin, GpioController controller = null)
+        public static DCMotor Create(int speedControlPin, int directionPin, GpioController controller = null, bool shouldDispose = true)


Same comment as the CharacterLCd here, we should probably have a Create Overload that doesn't take a shouldDispose, and set it to false in case controller was passed in. Another way to do this is to default it to false instead, and only set it to true if the controller that was passed in was null.

Same comment then for the previous one, it's more about consistency.

I see, I'm fine with keeping it as is for consistency

joperezr · 2020-03-23T17:37:44Z

src/devices/DCMotor/DCMotor.cs

        /// Connecting motor directly to GPIO pin is not recommended and may damage your board.
        /// </remarks>
-        public static DCMotor Create(int speedControlPin, GpioController controller = null)
+        public static DCMotor Create(int speedControlPin, GpioController controller = null, bool shouldDispose = true)


Wait with your latest changes you are not really disposing anything right? It looks like you are only taking in the extra parameter but not really assigning to anything, and also not checking it before disposing.

joperezr · 2020-03-23T17:39:08Z

src/devices/Dhtxx/DhtBase.cs

-        public DhtBase(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical)
+        /// <param name="gpioController"><see cref="GpioController"/> related with operations on pins</param>
+        /// <param name="shouldDispose">True to dispose the Gpio Controller</param>
+        public DhtBase(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical, GpioController gpioController = null, bool shouldDispose = true)


Same here, it doesn't look like you are actually checking this value before disposing.

joperezr · 2020-03-23T17:43:35Z

src/devices/ExplorerHat/Led.cs

+            if (disposing)
            {
-                if (disposing)
+                if (_shouldDispose)


This is not really disposing, this is only really turning the LED off. I realize you didn't introduce this, but now that you are touching this do you mind fixing that? Just dispose the controller after calling Off. Also, I believe Off should be called regardless in this case.

joperezr · 2020-03-23T17:44:51Z

src/devices/ExplorerHat/Lights.cs

        #region IDisposable Support

-        private bool _disposedValue = false; // Para detectar llamadas redundantes
+        private bool _shouldDispose;


The pourpuse of the previous line was different though, it was meant to make sure that the double call to Dispose() wouldn't crash the device

joperezr · 2020-03-23T17:45:27Z

src/devices/ExplorerHat/Lights.cs

        protected virtual void Dispose(bool disposing)
        {
-            if (!_disposedValue)
+            if (disposing)


I think you shouldn't change this condition from my comment above. This was really just meant to catch the case of double call to Dispose()

joperezr · 2020-03-23T17:45:41Z

src/devices/ExplorerHat/Motors.cs

        protected virtual void Dispose(bool disposing)
        {
-            if (!_disposedValue)
+            if (disposing)


Same comment here.

joperezr · 2020-03-23T17:46:59Z

src/devices/Hcsr04/Hcsr04.cs

        {
-            if (_controller != null)
+            if (_shouldDispose)
            {


NIT: you can also change the bellow code to just be:

_controller?.Dispose(); _controller = null;

in order to remove the if-check

joperezr · 2020-03-23T17:51:45Z

src/devices/Nrf24l01/Nrf24l01.cs

            _ce = ce;
            _irq = irq;
            PacketSize = packetSize;
+            _shouldDispose = shouldDispose;


You are not checking this value on the Dispose() call and are always disposing.

joperezr · 2020-03-23T17:54:00Z

src/devices/Pcx857x/Pcx857x.cs

        /// </param>
-        public Pcx857x(I2cDevice device, int interrupt = -1, GpioController gpioController = null)
+        /// <param name="shouldDispose">True to dispose the Gpio Controller</param>
+        public Pcx857x(I2cDevice device, int interrupt = -1, GpioController gpioController = null, bool shouldDispose = true)


NIT: Try not to default the values on abstract classes so that we make sure that we are flowing this value from all of the subclasses. That's the same feedback that I was suggesting on DCMotor

Yes, that said, I just added on top, the initial class had anyway some default values.

joperezr · 2020-03-23T17:56:53Z

Left a few minor comments where you missed Disposing or other minor things, but looks good in general. Thanks a lot for fixing @Ellerbach

Ellerbach · 2020-03-26T08:26:29Z

Left a few minor comments where you missed Disposing or other minor things, but looks good in general. Thanks a lot for fixing @Ellerbach

OK, so I have adjusted the misses and also made consistency to always dispose when the controller is null. Please check again all is ok! Thanks again

joperezr · 2020-03-27T21:28:35Z

src/devices/Dhtxx/DhtBase.cs

        {
-            _controller?.Dispose();
+            if (_shouldDispose)
+            {


NIT: looks like here we are missing to also reassign _controller to Null, I know it wasn't introduced by your change but can we do that in case double Dispose is called?

It's a readonly value, so I don't think it can be assigned to null?

joperezr · 2020-03-27T21:32:53Z

src/devices/ExplorerHat/Led.cs

                if (disposing)
                {
-                    Off();
+                    if (_shouldDispose)


Looks like my last comment on here hasn't been addressed yet. For convenience, this was it:

This is not really disposing, this is only really turning the LED off. I realize you didn't introduce this, but now that you are touching this do you mind fixing that? Just dispose the controller after calling Off. Also, I believe Off should be called regardless in this case.

joperezr · 2020-03-27T21:34:25Z

src/devices/ExplorerHat/Lights.cs

-                    LedArray[1].Dispose();
-                    LedArray[2].Dispose();
-                    LedArray[3].Dispose();
+                    if (_shouldDispose)


NIT, looks like we forgot to also dispose the controller here.

the controller should not be disposed in this dispose as every individual Led is disposing it. I moved out of the if the individual calls to each Led.

would that be a problem though? it will mean that perhaps the first LED will dispose it, and then the second one will try to call Off() before disposing it again and that will fail. Wouldn't it be better to just have the LEDs dispose and have the Ligths Dispose dispose the actual controller?

Yes, great catch. Adding Off(); before disposing. Then the controller will be disposed at the Lights level, not at every individual level.

joperezr · 2020-03-27T21:34:49Z

src/devices/ExplorerHat/Motors.cs

                {
-                    _motorArray[0].Dispose();
-                    _motorArray[1].Dispose();
+                    if (_shouldDispose)


not disposing the controller here either.

src/devices/Hcsr04/Hcsr04.cs

pgrawehr · 2020-04-17T05:27:06Z

While working on #1044: It might really be worth considering to make the GpioController a mandatory parameter, even if that's breaking. Otherwise, the interface is error-prone, as people leave out the paramter, even if they need to provide one (i.e. for Arduino, etc). This will either cause an exception, because the standard GpioController is likely not available, or worse still, use the wrong controller if there are multiple (and make people wonder why it doesn't work).
Also, as I designed it now, using a GpioController without a Board reference should be deprecated.

Ellerbach · 2020-04-17T06:44:28Z

While working on #1044: It might really be worth considering to make the GpioController a mandatory parameter, even if that's breaking

I did change all the constructors to add the GpioController as a paramter. If null, then the default one will be used. If not null, then the shouldDispose parameter will decide if to be disposed or not at the end.

pgrawehr · 2020-04-17T07:24:12Z

I did change all the constructors to add the GpioController as a paramter. If null, then the default one will be used. If not null, then the shouldDispose parameter will decide if to be disposed or not at the end.

Yes, I know. But I think this might not be optimal. I'm concerned about the fact that the default one will be wrong or just not work at all. I.e. if one is working with an Arduino or an FT4222, attempting to use the default GpioController will just throw an exception, because the default controller for such boards cannot be auto-detected.

So it might be better to require the GpioController to be not null upon constructing the bindings, to avoid this kind of possible error. This would prevent the above misstake as well as later prevent "dangling" controllers that are not associated to a board. (Note: the exception to this rule would of course be bindings where the controller truly is optional, such as the MCP chips, where it's optionally used for interrupt handling only).

Ellerbach · 2020-04-17T08:55:22Z

I'm concerned about the fact that the default one will be wrong or just not work at all. I.e. if one is working with an Arduino or an FT4222, attempting to use the default GpioController will just throw an exception, because the default controller for such boards cannot be auto-detected

Yes and no. I get your point. Now, if you are using an Arduino or a FT4222, you know it and it's on purpose. So you will create the GpioController before and pass it to the constructor. So in short, you'll do the work of passing it. Now, for the very fast majority, the build in driver is just what they need. So they'll just pass it. I guess the important is to make sure we can, if needed pass an Arduino or a FT4222 or any other GpioController we'll build over time. But keep it simple for most people.

pgrawehr · 2020-04-17T10:48:33Z

Now, if you are using an Arduino or a FT4222, you know it and it's on purpose. So you will create the GpioController before and pass it to the constructor. So in short, you'll do the work of passing it. Now, for the very fast majority, the build in driver is just what they need. So they'll just pass it. I guess the important is to make sure we can, if needed pass an Arduino or a FT4222 or any other GpioController we'll build over time. But keep it simple for most people.

I'm not sure that "most people" will be working on (typically) a Raspi. Right now, that's certainly the case, but with Arduino and FT4222 supoort, this library could become feasible also as platform for school projects and rapid prototyping. On the other hand, keeping it simple is certainly a good concept.

Maybe some more complex solutions are necessary, like keeping a static reference somewhere, so that the GpioController can be "auto-detected". Has other drawbacks, I know, but just my 2ct.

joperezr

Changes look good @Ellerbach thanks for the contribution and sorry for the delay. I will hold off on merging this so we don't conflict with #1038 but will merge it once that one goes in.

Fixing missing GpioController and shouldDispose

2f4d54b

joperezr reviewed Mar 19, 2020

View reviewed changes

Adjusting DC Motor

bcf9769

joperezr reviewed Mar 23, 2020

View reviewed changes

Adjusting and fixing dispose elements based on feedbacks

a4972d2

joperezr reviewed Mar 27, 2020

View reviewed changes

src/devices/Hcsr04/Hcsr04.cs Show resolved Hide resolved

Ellerbach added 3 commits March 30, 2020 14:25

Adjusting dispose in Led, Lights and Motors after PR comments

a8d9352

Adding off in dispose for LedArray

e80bd1a

Adjusting dispose in Lights

871318c

pgrawehr mentioned this pull request Apr 15, 2020

[WIP] Board abstraction #1044

Closed

joperezr self-assigned this Apr 24, 2020

joperezr approved these changes Apr 27, 2020

View reviewed changes

joperezr merged commit 8e1f3ef into dotnet:master Apr 27, 2020

github-actions bot locked and limited conversation to collaborators Dec 12, 2023

Conversation

Ellerbach commented Mar 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Mar 23, 2020

Uh oh!

Ellerbach commented Mar 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgrawehr commented Apr 17, 2020

Uh oh!

Ellerbach commented Apr 17, 2020

Uh oh!

pgrawehr commented Apr 17, 2020

Uh oh!

Ellerbach commented Apr 17, 2020

Uh oh!

pgrawehr commented Apr 17, 2020

Uh oh!

joperezr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels