Made Pins more structured rather than just using strings

[NathanJPhillips]

This would be a breaking change and perhaps should be done using a new property name for that reason, but it's so much more Bifrost-style that it might be right to include this as a breaking change in the next version.

[jeremylindsayni]

Thank you Nathan! Your pull request is definitely an improvement to the code.

Before I merge the change, I want to test if I can do something to avoid a breaking change. I'm not sure if I can have a couple of overloaded methods (and mark one as 'Obsolete' or something), but I'll give it a go.

Thank you again for your input!
Jeremy

[barkermn01]

I have just pulled this into my fork and your change is just bad practice.

Your connections to all pins at the moment the Pins getter is called effectively as application startup and not as needed, so you have objects created in memory with file handles open for no reason.

This also causes issues if people build applications with config files allowing pins to be specified E.G so multiple applications could be run side by side that only use 3 or 4 pins each.

This is for systems with limited free memory you should not be wasting it by the preloading object you don't need.

It would be better if you changed your dictionary to give a delegate that can then return the opened pin.

E.G

GpioPin gpiop0 = (GpioPin) GpioController.Instance.Pins[0]();

[barkermn01]

@NathanJPhillips I have sent you a pull request that will implement what I have spoken about here, this does not fix the fact it's a breaking change.

[NathanJPhillips]

@barkermn01 - I think you might be a bit confused about the implementation of GpioPin - it doesn't hold file handles. See here: https://github.com/jeremylindsayni/Bifrost/blob/master/Bifrost.Devices.Gpio/GpioPin.cs

[barkermn01]

Ok it's still creating unneeded objects

[NathanJPhillips]

I haven't seen your PR but I wonder if the functions you are returning are actually closures. If so they may be slightly bigger than the objects that my PR creates as they will contain all the data in the object as well as the overhead of the closure itself (I'm not sure exactly how C# lays out closures but they will at least need an extra function pointer over the plain object). It's fine if you made a mistake here, but perhaps the take-home message should be around the tone of your original comment.
I always think that in order to encourage contributors to a project it is better to be nice to people and display humility, especially in public forums. This can be done by asking questions to help people see their mistakes themselves rather than telling them that their code is bad.

[barkermn01]

i think that would depend on how C# is authored, i would assume at ASM level it should be similar to

This would only becalled when the delegate is called,

jump {address of closure}
copy {address of pin} {address of pin for closure}
jump {create object structure ops}
...
Jump {address of open}
...

please note I'm aware there would be other ASM commands from the compiler and OS libraries but the required operations for the application to function is what I'm trying to cover. so I'm pretty your way would be quicker on the CPU instructions if someone was to use every pin in an app. and I'm unsure due to not knowing how Delegates work in the ASM core of C# as to at what point mine would be quicker, I because of fewer CPU instructions.

Because I'm adding more instructions to go through to get the pin, I'm also only assigning heap memory as it's needed not all at once but that means more instructions to set up the heap at call time, but I'm removing those instructions from app startup. so it might be that I'm adding more processing but reducing memory wastage so long as most of the pins are not used.

Think it might be time to build a tester app for this... could be interesting to see how much overhead delegates/closures add.

[NathanJPhillips]

Post a link to your PR and I'll see if I can give you an idea of the size of the function object required to hold the closure.

[barkermn01]

NathanJPhillips#1 it's a PR on your code base for you to review before updating this PR 👍

[NathanJPhillips]

Your PR creates function objects that contain:
1 x pointer (to the code of the function)
1 x pointer (to tuple being passed in the closure to the function - needs a pointer as it's a class, not a struct)
1 x pointer (to DirectoryInfo)
1 x DirectoryInfo (haven't looked at the implementation of this, but it will be at least as big as the string for the path)
1 x bool (inside Nullable)
1 x int

Conversely the GpioPin size is:
1 x int
1 x string (for the path)

Therefore it will use less memory to create the GpioPins at startup rather than delaying the creation of them through the use of closures.

[barkermn01]

Ok, so that list of things in my code that is also in yours but you have counted against me...
structured-pins/Bifrost.Devices.Gpio/GpioController.cs this is your original branch

1 x DirectoryInfo (haven't looked at the implementation of this, but it will be at least as big as the string for the path)

I just changed it so that instead of (IGpioPin)new GpioPin(diAndPinNo.Item2.Value, diAndPinNo.Item1.FullName) it has GetGpioPin(diAndPinNo) so yours is returning a pointer to an object mine is returning a pointer to the method. my method then creates and returns the pointer to the object of the GPIO when called.

1 x pointer (to DirectoryInfo)

I'm not sure where you are getting this from that is not in your code? both instances exist is both codebases the one on line 33 & 70.

Other points:

1 x bool (inside Nullable)

All objects are nullable in C# so I don't understand the point about that, but also that should be in your's that's actually another feature added at the same time that allows and application while running to close a port should it need to wich is a feature that should always be there you should not open something and not close it safely and not really on the memory cleaner to close it when it gets around to disposing of the object.

Other points
Yes, I use an extra int and yes I use an extra pointer to a memory location.

However, no part of your answer has actually covered what we were talking about what is the tipping point of All Objects created at the start vs Creating the objects on-demand I.E at how many open pins does my overhead become a bigger memory footprint that all the pins being opened from the start I know if all pins are open mine is bigger than yours but if for example, I'm using a device that uses only 3 pins does the overhead for pin on use loading make the application bigger than yours opening all 3 if not at what point does it? because if the overhead is less than 1 GpioPin Object it's worth it. I would say if it's less than 3 pins it's probably worth it.

[tombiddulph]

Off topic here but it would be prudent for both of you to stop nitpicking at each other and focus on what is the best solution for this problem rather than trying to score points off of each other. /rant

[barkermn01]

That's my point I'm not trying to score points I have not nitpicked his work at any point I offered an alternative and called out what is normally classed as bad code in implementing unneeded/unused objects at the start of this, have tried only to offer an explanation of my work, I have called out his point scoring system where it is wrong because it's an attack on my work without suitable reasoning.

I ended with ended my comment with exactly what needs to be done to test both implementations I'm going to see if I can get hold of a rPi and test both methods and get the actual running memory usage information of the same application using each method, so this a method can be implemented and this can be put to bed.

Thinking more on this being a breaking change it might be that this get's branched into its own version as it is breaking but then new applications should use the new version and old application use the old version.

But that can be dealt with once the methodology is figured out.

[NathanJPhillips]

I don't think we're nitpicking - I'm just trying to explain that the function objects the suggested change creates are not just function pointers but closures that are actually larger than the objects originally created and so the change does not add any value. That makes a big difference.
@barkermn01 - you are right that creating objects up front rather than lazily is bad practice. You just need to realise that your closures are objects.

[barkermn01]

@NathanJPhillips so then we should not be using Lambda at all because it goes,
.Where(Closure)
.Select(Closure)
.Where(Closure)
.ToDictionary(Closure, Closure)

Because => "arrow functions" are closures. it is For example
di => di.Name.StartsWith("gpio") is short hand for (DirItem) => {return DirItem.Name.StartsWith("gpio"); } created for every object in the GetDirectories return array