Add more settings to SPI, and possibly add a Get-SPIDevice #54

DanielSSilva · 2020-04-14T23:05:03Z

This is an initial commit so that we can discuss if such changes can make sense.
This comes in sequence of issue #53. In my case, I have the neopixels which I can use to test the SPI, basing my implementation on dotnet IoT binding sample for the WS2812b.

Why a Get-SPIDevice - For this binding, they are passing the SPI device as a parameter (which was one of the reasons why I've decided to try an implementation of a Get-SPIDevice). It also seems, as I've pointed on the issue, that "this seems that the SPI device could be created once and used many times", which would make more sense than creating a device for each time we send some data.
Why more settings - Again, basing my implementation on their sample, there were some settings that seemed to be mandatory for it to work properly. Because it seems they are not mandatory, they can be created with default values.

Here's an example:

$device = Get-SPIDevice -Frequency 2400000 -Mode Mode0 -DataBitLength 8 -BusId 0 -ChipSelectLine 0
	$neo = [Iot.Device.Ws28xx.Ws2812b]::new($device,1)                              
	$img = $neo.Image
	$img.SetPixel(0,0, [System.Drawing.Color]::DarkRed)
	$neo.Update()

TylerLeonhardt · 2020-04-15T01:29:06Z

src/Microsoft.PowerShell.IoT/SPI/Send/SendSPIData.cs

+        var settings = new SpiConnectionSettings(this.BusId, this.ChipSelectLine)
+        {
+            ClockFrequency = this.Frequency,
+            DataBitLength = this.DataBitLength,
+            Mode = this.Mode,
+            DataFlow = this.DataFlow,
+            ChipSelectLineActiveState = this.ChipSelectLineActiveState
+        };


Should we be concerned about overwriting defaults here if they aren't specified via the cmdlet?

I don't think we should be concerned because if those fields are not specified, they will have their defaults (0 for int, etc), which already happens with our current version. DataBitLength wasn't specified, hence it was always 0, etc

We should Not rely on types' default values.
For defaults we should use values common for SPI communication, so use/copy-paste the defaults from .NET code.
For example, in normal SPI communications default DataBitLength=8.

Totally agree. I was relying on the defaults because from what I saw they seemed to be the .NET defaults. Checking again, I was wrong. I was probably checking the wrong file .
Leaving the SpiSettings here for further reference

iSazonov · 2020-04-15T03:09:32Z

src/Microsoft.PowerShell.IoT/SPI/SPIDevice.cs

+    internal SpiDevice device = null;
+    public int BusId { get; set; }
+
+    public int ChipSelectLine { get; set; }


Should these properties be public settable?

I've followed the same implementation as we have for I2C (here).
From my understanding, it will depend on what we want to do:

What should happen if the user changed their hardware from busId 0 to 1, or the ChipSelectLine? Should he create a new device? Or should we allow to change the busId and ChipSelectLine?

If we want to bind a device to a busId and ChipSelectLine, this should not be settable.

High level thinking is that these properties here are for information only / aka readlonly. If people want to use another set of settings, they need to construct another SPIDevice using Get-SPIDevice.

Just to clarify, this means that all properties should be readonly, correct?

Do you consider readonly struct for this?

Just to clarify, this means that all properties should be readonly, correct?

Correct.

Do you consider readonly struct for this?

This seems alright.

anmenaga · 2020-04-22T21:57:17Z

src/Microsoft.PowerShell.IoT/SPI/Get/GetSPIDevice.cs

+    {
+        this.BusId = 0;
+        this.ChipSelectLine = 0;
+        this.Frequency = 500_000; // 500 KHz default speed


defaults for other properties also have to be set here. They can be taken from .NET's SpiDevice code.

anmenaga · 2020-04-22T22:00:29Z

src/Microsoft.PowerShell.IoT/SPI/Get/GetSPIDevice.cs

+    public PinValue ChipSelectLineActiveState { get; set; }
+
+    [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true)]
+    public SwitchParameter Raw { get; set; }


looks like this is not used anywhere so can be removed.

Can't understand from GH interface what property you are referring to. The Raw?

Yes; Raw.

anmenaga · 2020-04-22T22:49:39Z

src/Microsoft.PowerShell.IoT/SPI/Get/GetSPIDevice.cs

+            ChipSelectLineActiveState = this.ChipSelectLineActiveState
+        };
+
+        SpiDevice spiDevice = SpiDevice.Create(settings);


I think this should return our SPIDevice wrapper that has public property of .NET's SpiDevice. This way .NET's SpiDevice object is accessible and can be used for scenarios like neopixels in this PR.

anmenaga · 2020-04-22T22:50:18Z

src/Microsoft.PowerShell.IoT/SPI/SPIData.cs

+using System.Device.Gpio;
+using System.Device.Spi;
+
+public class SPIData : SPIDevice


SPIData should Not inherit from SPIDevice.

Just have another SPIDevice-type property

I'm not familiar with best practices for such cases. Why shouldn't it inherit from SPIDevice? I've leveraged from inheritance because SPIData has all the properties that SPIDevice has, plus the Data and Response. Also, because an SPIData requires an SPIDevice

Yes, it is very tempting to inherit SPIData from SPIDevice, but semantically the data object is not a subclass of a device object. Inheriting one from another usually brings problems down the road.
For same reason I2CDeviceRegisterData does not inherit from I2CDevice but rather has a property of this type that exposes device configuration data (as well as device object).

anmenaga · 2020-04-22T22:54:09Z

src/Microsoft.PowerShell.IoT/SPI/SPIDevice.cs

+    internal SpiDevice device = null;
+    public int BusId { get; set; }
+
+    public int ChipSelectLine { get; set; }


High level thinking is that these properties here are for information only / aka readlonly. If people want to use another set of settings, they need to construct another SPIDevice using Get-SPIDevice.

anmenaga · 2020-04-22T22:55:57Z

src/Microsoft.PowerShell.IoT/SPI/SPIDevice.cs

+
+public class SPIDevice
+{
+    internal SpiDevice device = null;


I don't see a problem if this class just holds 2 public properties:

public SpiDevice device; public SpiConnectionSettings connectionSettings;

I agree with this, but it would make sense to "expand" the connectionSettings when they are being displayed, like overriding the ToString

This can probably be done in a type format file for class SPIDevice.
I also don't have a bug problem with overriding ToString.

src/Microsoft.PowerShell.IoT/SPI/SPIData.cs

anmenaga · 2020-04-22T23:04:44Z

src/Microsoft.PowerShell.IoT/SPI/Send/SendSPIData.cs

+        var settings = new SpiConnectionSettings(this.BusId, this.ChipSelectLine)
+        {
+            ClockFrequency = this.Frequency,
+            DataBitLength = this.DataBitLength,
+            Mode = this.Mode,
+            DataFlow = this.DataFlow,
+            ChipSelectLineActiveState = this.ChipSelectLineActiveState
+        };


We should Not rely on types' default values.
For defaults we should use values common for SPI communication, so use/copy-paste the defaults from .NET code.
For example, in normal SPI communications default DataBitLength=8.

anmenaga · 2020-04-22T23:06:51Z

src/Microsoft.PowerShell.IoT/SPI/Send/SendSPIData.cs

-        settings.ClockFrequency = this.Frequency;
-
-        using(var spiDevice = SpiDevice.Create(settings))
+        var settings = new SpiConnectionSettings(this.BusId, this.ChipSelectLine)


Constructing SpiConnectionSettings and SPIDevice can be moved from this cmdlet to Get-SPIDevice

This cmdlet can just take SPIDevice object instead of all settings parameters.

anmenaga · 2020-04-22T23:16:10Z

Also, a general comment: using SPI for neopixels is a hack that exploits the high-frequency of SPI bus to emulate (relatively)low-frequency of neopixel protocol.
This hack is a valid scenario, but it can't be used as verification that our SPI cmdlets actually work correctly for real SPI communications between devices.

DanielSSilva

general comments regarding feedback. Leaving it here to check later

DanielSSilva · 2020-04-23T22:25:12Z

src/Microsoft.PowerShell.IoT/SPI/Get/GetSPIDevice.cs

+    public PinValue ChipSelectLineActiveState { get; set; }
+
+    [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true)]
+    public SwitchParameter Raw { get; set; }


Can't understand from GH interface what property you are referring to. The Raw?

src/Microsoft.PowerShell.IoT/SPI/SPIData.cs

DanielSSilva · 2020-04-23T22:32:12Z

src/Microsoft.PowerShell.IoT/SPI/SPIData.cs

+using System.Device.Gpio;
+using System.Device.Spi;
+
+public class SPIData : SPIDevice


I'm not familiar with best practices for such cases. Why shouldn't it inherit from SPIDevice? I've leveraged from inheritance because SPIData has all the properties that SPIDevice has, plus the Data and Response. Also, because an SPIData requires an SPIDevice

DanielSSilva · 2020-04-23T22:33:55Z

src/Microsoft.PowerShell.IoT/SPI/SPIDevice.cs

+
+public class SPIDevice
+{
+    internal SpiDevice device = null;


I agree with this, but it would make sense to "expand" the connectionSettings when they are being displayed, like overriding the ToString

initial commit for possible changes on SPI device and data

f1923b9

DanielSSilva requested review from TylerLeonhardt and anmenaga April 14, 2020 23:05

TylerLeonhardt reviewed Apr 15, 2020

View reviewed changes

iSazonov reviewed Apr 15, 2020

View reviewed changes

anmenaga suggested changes Apr 22, 2020

View reviewed changes

DanielSSilva commented Apr 23, 2020

View reviewed changes

added default values for SPI settings

d9e6795

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more settings to SPI, and possibly add a Get-SPIDevice #54

Add more settings to SPI, and possibly add a Get-SPIDevice #54

DanielSSilva commented Apr 14, 2020

TylerLeonhardt Apr 15, 2020

DanielSSilva Apr 15, 2020

anmenaga Apr 22, 2020

DanielSSilva Apr 23, 2020

iSazonov Apr 15, 2020 •

edited

Loading

DanielSSilva Apr 15, 2020

anmenaga Apr 22, 2020

DanielSSilva Apr 23, 2020

iSazonov Apr 24, 2020

anmenaga Apr 30, 2020

anmenaga Apr 22, 2020

anmenaga Apr 22, 2020

DanielSSilva Apr 23, 2020

anmenaga Apr 30, 2020

anmenaga Apr 22, 2020

anmenaga Apr 22, 2020

anmenaga Apr 22, 2020

DanielSSilva Apr 23, 2020

anmenaga Apr 30, 2020

anmenaga Apr 22, 2020

anmenaga Apr 22, 2020

DanielSSilva Apr 23, 2020

anmenaga Apr 30, 2020

anmenaga Apr 22, 2020

anmenaga Apr 22, 2020

anmenaga Apr 22, 2020

anmenaga commented Apr 22, 2020

DanielSSilva left a comment

DanielSSilva Apr 23, 2020

DanielSSilva Apr 23, 2020

DanielSSilva Apr 23, 2020

Add more settings to SPI, and possibly add a Get-SPIDevice #54

Are you sure you want to change the base?

Add more settings to SPI, and possibly add a Get-SPIDevice #54

Conversation

DanielSSilva commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmenaga commented Apr 22, 2020

DanielSSilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Apr 15, 2020 •

edited

Loading