MCP2210 Device Binding - Githubissues

shaggygi commented 4 years ago

@joperezr

I'm looking at working on a new MCP2210 (USB-to-SPI Protocol Converter with GPIO) binding.

This is controlled via a USB interface. Before starting the development, I wanted to know if I would be allowed to reference the HidSharp NuGet package for the communications? It appears it is under Apache license.

The plan is to not expose anything from HidSharp in the device's public API. I haven't reviewed all details from the datasheet, but the device provides 9 GPIO and a SPI bus. I'm assuming it would look something like the following (but not confirmed yet).


var mcp2210 = new Mcp2210("[serialNumber]")  // serialNumber would allow to communicate with multiples.

// Allow for direct control of GPIO.
mcp2210.GpioController.SetPinMode(3, PinMode.Output);
mcp2210.GpioController.Write(3, PinValue.High);

// Allow for direct SPI communications.
mcp2210.SpiDevice.WriteByte(0x12);

// Or control another device binding.
var mcp23s08 = new Mcp23S08(mcp2210.SpiDevice, 0x20, 1, 2, mcp2210.GpioController);
mcp23s08.SetPinMode(7, PinMode.Output);
mcp23s08.Write(7, PinValue.High);

krwq commented 4 years ago

From my perspective: Yes - I'm not a fan of libraries not putting their code on github but it seems to me good enough reason to use it - in the future might be worth to put this library in separate package so not everyone has to pull the dependency. Also seems that this library might also in the future support Bluetooth Low Energy (They say Added experimental Bluetooth Low Energy support on Windows. This part of the library may change in the future. Let me know how it works for you. in the release notes - not sure about Linux). @joperezr, @Ellerbach thoughts?

From legal perspective: I think this is fine as long as we add relevant info in 3rd party notices file (will double check with our PM after @joperezr is fine with the plan).

Other aspects (security):

we should skim through the source code and csproj/msbuild files to make sure there is no obvious/intentional security holes - we don't have to spent super much time but enough to gain confidence - also note the files which are unusually large
make sure nupkg doesn't contain any binaries other than .NET dlls and check with ILSpy that it more or less matches what the source is

krwq commented 4 years ago

From API perspective I think Mcp2210 should directly implement SpiDevice unless there is good reason not to.

Can you also explain deeper what serial number is and the comment you wrote?

krwq commented 4 years ago

And welcome back @shaggygi! Hope you're doing well!

shaggygi commented 4 years ago

From API perspective I think Mcp2210 should directly implement SpiDevice unless there is good reason not to.

I was thinking this, but it is also similar to a GpioController. That is where the older interfaces would have been nice to implement both. I'll probably have more questions for you when I dive deeper.

shaggygi commented 4 years ago

And welcome back @shaggygi! Hope you're doing well!

Thanks buddy! Been super busy with work life, but trust me... I've been watching the repo's progress when I can. Actually, this new binding is for a work-related project.

joperezr commented 4 years ago

Hope you are doing well @shaggygi! The device sounds very interesting so we would love to have a binding for it. In terms of the NuGet package question, as @krwq suggests we definitely need to check with legal as this is not something that either him or myself can sign off on. I'll start an internal thread right away and will ping you once we know more and I'll make sure to drive the discussion so that we get a resolution as fast as possible as similar things have taken longer time than I would have wanted in the past.

For the public sruface area I agree with @krwq where we would want this to be an implementation of SPIDevice, as in my mind I'm seeing it as an equivalent to the GPIO Expander bindings we have today that just implement GpioDriver.

Ellerbach commented 4 years ago

I was thinking this, but it is also similar to a GpioController. That is where the older interfaces would have been nice to implement both. I'll probably have more questions for you when I dive deeper.

Welcome back :-) You can implement both Gpiocontroller and SpiDevice. See for example FT4222: https://github.com/dotnet/iot/tree/master/src/devices/Ft4222

The FT4222 has 3 interfaces: Gpio, I2C and SPI, so I built the driver for all 3. What is left on top if just find if a device is present. All the rest is behind the drivers.

pgrawehr commented 4 years ago

... or wait for the Concept review of #1044. It's designed just for such hardware.

krwq commented 4 years ago

@shaggygi feel free to add dependency on the library with following conditions:

https://github.com/dotnet/iot/blob/master/THIRD-PARTY-NOTICES.TXT needs to be updated, use https://github.com/dotnet/runtime/blob/master/THIRD-PARTY-NOTICES.TXT as an example - I believe you will need to copy license there see what dotnet/runtime does)
quickly inspect the library as mentioned above to gain confidence in security - I'll also skim it
when adding PackageReference put comment above saying that issues/patches for that library should be reported here: https://forum.zer7.com/ and source code is here: https://www.zer7.com/files/oss/hidsharp/HIDSharp21.zip

Ellerbach commented 3 years ago

[Triage] Please look at the Arduino board class on how the API should look like: https://github.com/dotnet/iot/tree/main/src/devices/Arduino

krwq commented 1 year ago

[Triage] We're not tracking device binding requests. If you have anything to discuss around the binding please reopen

dotnet / iot

MCP2210 Device Binding #1059