Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1923b9
initial commit for possible changes on SPI device and data
DanielSSilva Apr 14, 2020
d9e6795
added default values for SPI settings
DanielSSilva Sep 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions src/Microsoft.PowerShell.IoT/SPI/Get/GetSPIDevice.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
using System;
using System.Management.Automation;
using System.Device.Spi;
using System.Device.Gpio;

[Cmdlet(VerbsCommon.Get, "SPIDevice")]
public class GetSPIDevice : Cmdlet
{

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 0)]
public int BusId { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 1)]
public int ChipSelectLine { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 2)]
public int Frequency { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 3)]
public int DataBitLength { get; set;}

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 4)]
public SpiMode Mode { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 5)]
public DataFlow DataFlow { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 6)]
public PinValue ChipSelectLineActiveState { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true)]
public SwitchParameter Raw { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; Raw.


public GetSPIDevice()
{
this.BusId = 0;
this.ChipSelectLine = 0;
this.Frequency = 500_000; // 500 KHz default speed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

SpiDevice spiDevice = SpiDevice.Create(settings);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//TODO: This can be done this way if we follow the same logic as in I2C where we access the device by doing device.Device
// WriteObject(new SPIDevice(spiDevice, this.BusId, this.ChipSelectLine, this.Frequency,
// this.DataBitLength, this.Mode, this.DataFlow,
// this.ChipSelectLineActiveState));
//Because I'm currently testing this like this : $device = Get-SPIDevice -Frequency 2400000 -Mode Mode0 -DataBitLength 8 -BusId 0 -ChipSelectLine 0
// I want the returned object to be the spiDevice created.
WriteObject(spiDevice);

}
}
35 changes: 26 additions & 9 deletions src/Microsoft.PowerShell.IoT/SPI/SPIData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,33 @@
public class SPIData
// public class SPIData
// {
// public int BusId { get; set; }
// public int ChipSelectLine { get; set; }
// public int Frequency { get; set; }
// public byte[] Data { get; set; }
// public byte[] Response { get; set; }

// public SPIData(int busId, int chipSelectLine, int frequency, byte[] data, byte[] response)
// {
// this.BusId = busId;
// this.ChipSelectLine = chipSelectLine;
// this.Frequency = frequency;
// this.Data = data;
// this.Response = response;
// }
// }

using System.Device.Gpio;
anmenaga marked this conversation as resolved.
Show resolved Hide resolved
using System.Device.Spi;

public class SPIData : SPIDevice
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SPIData should Not inherit from SPIDevice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just have another SPIDevice-type property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{
public int BusId { get; set; }
public int ChipSelectLine { get; set; }
public int Frequency { get; set; }
public byte[] Data { get; set; }
public byte[] Response { get; set; }

public SPIData(int busId, int chipSelectLine, int frequency, byte[] data, byte[] response)
public SPIData(SpiDevice device, int busId, int chipSelectLine, int frequency,
int dataBitLength, SpiMode mode, DataFlow dataFlow,
PinValue chipSelectLineActiveState, byte[] data, byte[] response
): base(device, busId, chipSelectLine, frequency, dataBitLength, mode, dataFlow, chipSelectLineActiveState)
{
this.BusId = busId;
this.ChipSelectLine = chipSelectLine;
this.Frequency = frequency;
this.Data = data;
this.Response = response;
}
Expand Down
34 changes: 34 additions & 0 deletions src/Microsoft.PowerShell.IoT/SPI/SPIDevice.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using System.Device.Gpio;
using System.Device.Spi;

public class SPIDevice
{
internal SpiDevice device = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public SpiDevice device;
public SpiConnectionSettings connectionSettings;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public int BusId { get; set; }

public int ChipSelectLine { get; set; }
Copy link

@iSazonov iSazonov Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these properties be public settable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you consider readonly struct for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Correct.

Do you consider readonly struct for this?

This seems alright.


public int Frequency { get; set; }

public int DataBitLength { get; set;}

public SpiMode Mode { get; set; }

public DataFlow DataFlow { get; set; }

public PinValue ChipSelectLineActiveState { get; set; }

public SPIDevice(SpiDevice device, int busId, int chipSelectLine, int frequency,
int dataBitLength, SpiMode mode, DataFlow dataFlow,
PinValue chipSelectLineActiveState)
{
this.device = device;
this.BusId = busId;
this.ChipSelectLine = chipSelectLine;
this.Frequency = frequency;
this.DataBitLength = dataBitLength;
this.Mode = mode;
this.DataFlow = dataFlow;
this.ChipSelectLineActiveState = chipSelectLineActiveState;
}
}
35 changes: 31 additions & 4 deletions src/Microsoft.PowerShell.IoT/SPI/Send/SendSPIData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Management.Automation;
using System.Device.Spi;
using System.Device.Gpio;

[Cmdlet(VerbsCommunications.Send, "SPIData")]
public class SendSPIData : Cmdlet
Expand All @@ -17,6 +18,18 @@ public class SendSPIData : Cmdlet
[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 3)]
public int Frequency { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 4)]
public int DataBitLength { get; set;}

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 5)]
public SpiMode Mode { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 6)]
public DataFlow DataFlow { get; set; }

[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 7)]
public PinValue ChipSelectLineActiveState { get; set; }

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

Expand All @@ -25,14 +38,25 @@ public SendSPIData()
this.BusId = 0;
this.ChipSelectLine = 0;
this.Frequency = 500_000; // 500 KHz default speed
//use the same defaults as .NET core team is using
this.Mode = SpiMode.Mode0;
this.DataBitLength = 8;
this.DataFlow = DataFlow.MsbFirst;
this.ChipSelectLineActiveState = PinValue.Low;
}

protected override void ProcessRecord()
{
var settings = new SpiConnectionSettings(this.BusId, this.ChipSelectLine);
settings.ClockFrequency = this.Frequency;
var settings = new SpiConnectionSettings(this.BusId, this.ChipSelectLine)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
ClockFrequency = this.Frequency,
DataBitLength = this.DataBitLength,
Mode = this.Mode,
DataFlow = this.DataFlow,
ChipSelectLineActiveState = this.ChipSelectLineActiveState
};
Comment on lines +50 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


using(var spiDevice = SpiDevice.Create(settings))
using (var spiDevice = SpiDevice.Create(settings))
{
var response = new byte[this.Data.Length];

Expand All @@ -44,7 +68,10 @@ protected override void ProcessRecord()
}
else
{
SPIData spiData = new SPIData(this.BusId, this.ChipSelectLine, this.Frequency, this.Data, response);
SPIData spiData = new SPIData(spiDevice, this.BusId, this.ChipSelectLine, this.Frequency,
this.DataBitLength, this.Mode, this.DataFlow,
this.ChipSelectLineActiveState, this.Data, response);
//SPIData spiData = new SPIData(this.BusId, this.ChipSelectLine, this.Frequency, this.Data, response);
WriteObject(spiData);
}
}
Expand Down