Iot: Output-only SpiDevice?

[HakanL]

Would it make sense to have an Output-only SpiDevice class? For example SpiOutputDevice, which the SpiDevice can inherit from. The use-case is for hardware devices that don't have any read-capabilities, like LED/Pixel/PWM controllers like the TLC59711. It would be nice to create an implementation for a device that could take a SpiOutputDevice in the constructor which could be implemented either in hardware or software (SoftwareSpi).

[krwq]

I don't feel strongly but I'd rather go with parity with Stream so rather go in direction of having CanRead/CanWrite assuming we have a use case where it would mitigate accidental use. If your device is output only (i.e. LED strips are as well and we already use them) just don't call Read...

[HakanL]

It would just allow for a cleaner class structure. Take for example the SoftwareSpi class, it requires chip select, miso input pins, but for some output-only devices those aren't available/used. If we had an OutputSpiDevice then we could just take that as a constructor parameter into a device-implementation and it would be up to the user to choose if they want to use a hardware SPI (SpiDevice), or a software-implementation. We would then have separation of concerns, each device wouldn't have to implement their own bit-banging (when hardware isn't available).
Further, the SpiDevice class is written in a way where you have to implement the TransferFullDuplex, which does both a read and a write. For an output-only device that doesn't make sense. Yes, it could be done without the proposed changes, but it would be a hack, isn't it better to create a nice abstraction in the base classes? SpiDevice would inherit from SpiOutputDevice for example.

[krwq]

on the other hand you can't base your class on more than one abstract class - for SoftwareSpi I guess we could make it work if -1 is used instead of legit pin number. I have mixed feelings on this - too much abstraction usually does more harm long term so better be sure about the abstractions you're adding

[HakanL]

Correct, but the SpiDevice would inherit from SpiOutputDevice, not multiple base-classes (which obviously isn't possible), it would just be optional to create a SoftwareSpiOutput device, or just a SoftwareSpi (which would handle the outputs as well) depending on what your hardware can support.
I'm personally not a fan of hard-coded special numbers (like -1), but yes you could do some if/elses in there, but IMHO it would be cleaner to have the abstraction, especially since the main Write method in SoftwareSpi will do a read (via TransferFullDuplex), so you'd have to add a big chunk of if-cases.
An example of where there already is abstraction in this repo is the Mcp3008 device implementation, I actually think it makes a lot of sense. I like it when the abstraction matches the hardware, and since you can have an output-only SpiDevice, and one without ChipSelect, I think the classes should match that.
Also by adding the SpiOutputDevice we won't break anything existing.

[krwq]

Presumably if you have SpiOutputDevice you should also have SpiInputDevice - not sure if there are any devices like that but presumably it's possible, then SpiDevice would have to inherit from both which would not be possible. Also assuming we will support master devices in the Software implementation then LED strip itself would be an SpiInputDevice

[HakanL]

Actually SPI master devices can't be input-only, you need to write before you read (in a transaction). To be fair, the current SpiDevice should probably be called SpiMasterDevice, but that may be overkill. One could implement a Spi slave, but that's a completely different type. One can argue that a device can't be both master and slave, you have to pick one. There's also the option to create interfaces for these core classes instead, I'm not opposed to that.

[krwq]

@HakanL the Ws2812b is input only master, you need to be very careful with timing though, see https://github.com/dotnet/iot/tree/master/src/devices/Ws28xx

[HakanL]

Yes it is, but this library currently only supports sending data TO the WS2812, so the implementation in this library would be using the SpiOutputDevice. If you wanted to "emulate" a WS2812 pixel then you'd to have to build a Spi slave device, but I don't think that's in scope for this library (please correct me if I'm wrong). All device implementations I've seen in this library are for talking TO the devices (like LEDs, buttons, thermal sensors, pixels, etc), not to "act" (emulate) as the devices.
Sorry if I misunderstood your comment, but the WS2812B is an input-only SPI slave device, whereas the host (master) would be using the Ws28xx implementation linked above.

[krwq]

@HakanL I agree we currently don't have such device but there is couple of things we need to take into considerations when adding new APIs:

• we can't easily remove it - any removing of APIs will cause someone code to be broken
• it needs to be future proof (as much as possible, we can't predict what will actually happen in the future) - I'm currently seeing that we might need SpiInputDevice - even if it might happen in the 5 years and currently there is no way we can have 2 base classes
• is described scenario where API would be useful achievable using existing APIs and what would it take to change that if not - this currently comes down to SoftwareSpi which can be easily adapted to these needs
• consistency/parity with other APIs - to me SpiDevice is similar to Stream, Stream does not implement InputStream and OutputStream and uses CanRead/CanWrite instead
• find what other things could re-use the API - i.e. if we could make abstraction work also with I2cDevice and SerialPort then it would also solve other issues we have with some bindings which work with multiple interfaces and that would be much more convincing to me to add such API - perhaps we should go more in this direction (See https://github.com/dotnet/iot/issues/584 for reference)

Above are mostly experience on working with dotnet/runtime we have already had many APIs we added in the past which were a good idea in the time of adding but caused problems and large maintenance cost in the future - I'd prefer to avoid that so before adding we should do our best to prove the new API will make "the world" easier. I agree that SpiDevice is some sort of combination of input/output device but rather than just adding it I think we should give this more thought.

[HakanL]

Ok, I guess that makes sense. Would you be open to me modifying the SoftwareSpi to have the input (miso) and chip-select be optional? Do you prefer that I use hard-coded values like -1, or make them nullable?

[krwq]

Thanks for offering help, yes if it unblocks a scenario you have you are welcome to make the change 😸 -1, see https://github.com/dotnet/iot/blob/master/Documentation/Devices-conventions.md for reference (search for -1 in that doc)

[joperezr]

Hey @HakanL sorry to join late to the conversation. In hindsight, I agree with you that SpiDevice shouldn't have leaked implementation details like chipselect and MISO on its surface area to allow cleaner implementations of SoftwareSPI for example, but unfortunately we have shipped now with that design and we would really need a very strong argument in order to make a breaking change now. I agree with @krwq point of view and also agree with taking a PR to fix SofwareSpi so that input and chip select are optional as long as that doesn't introduce breaking changes (or if the breaking changes are what we would deem acceptable)

[HakanL]

@joperezr Actually the SpiDevice doesn't have the CS/MISO, that's only in the SoftwareSpi implementation, which I can change.

[joperezr]

Actually the SpiDevice doesn't have the CS/MISO

Not directly, but in order to create one you need SpiConnectionSettings which does have chipselect line and busid, so that is what I meant. It would have been nice to have some SpiConnectionSettings which wouldn't necesarily have implementation details.

[HakanL]

Ah, got it, yeah true. Well, I think I have what I need, I will send in a PR for making those optional in SoftwareSpi, and that will make it work ok for my TLC59711 device. Thanks all for your help! I think we can close this issue, good discussion and insight.

[HakanL]

I found one issue with SoftwareSpi that I'd like your guidance on. It takes a constructor parameter for chipselect pin (cs). But the SpiConnectionSettings class already has this as a property. I can either break the API (👎 ) by removing the cs parameter and only use the one from the settings. Or I can create an overloaded constructor that uses cs and ignoring the one from the settings class. What do you suggest? Breaking API or inconsistent functionality?

[krwq]

Ooops, probably my fault...

I think long-term we should re-design SpiConnectionSettings - perhaps introduce some base type which would only have what it should have.

I think having 3 pins directly as args and one in settings would be a bit confusing but I don't feel strongly either way.

@joperezr what are your thoughts?

Perhaps some in between solution for now though:

• change current cs arg to default to -1 (or introduce another overload)
• if cs arg or settings.CS will be non -1 then that's your chip select pin
• if both are set (and differ?) then you get an exception

[HakanL]

Submitted a PR for the changes to the SoftwareSpi. I will add my TLC device later, just need a little more testing.

[HakanL]

Here's the PR for the work that actually started this ticket (support for TLC-59711): https://github.com/dotnet/iot/pull/1077

[krwq]

@HakanL I assume this is fixed so closing, feel free to re-open if you feel like there's anything left

[HakanL]

Ok to close this ticket, but the #1077 PR is still open.