Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding FT232H #1628

Merged
merged 15 commits into from Sep 7, 2021
Merged

Adding FT232H #1628

merged 15 commits into from Sep 7, 2021

Conversation

Ellerbach
Copy link
Member

@Ellerbach Ellerbach commented Aug 9, 2021

  • Adding FT232H
  • Adding documentation
  • Adjusting FT4222 to reuse part of the existing code with minor braking changes
  • Adjusting related samples
  • Creating a shared common project for FTDI common elements getting ready to add more of those devices
Microsoft Reviewers: Open in CodeFlow

@msftbot msftbot bot added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Aug 9, 2021
@dotMorten
Copy link
Contributor

dotMorten commented Aug 9, 2021

SPI works great with my SSD1351 display and Adafruit FT232H adapter:

private static Ssd1351 CreateDisplay()
{
    var devices = FtCommon.GetDevices();
    var device = new Ft232HDevice(devices[0]);
    var driver = device.CreateGpioDriver();
    var ftSpi = new Ft232HSpi(new System.Device.Spi.SpiConnectionSettings(0, 4) { Mode = SpiMode.Mode3, DataBitLength = 8, ClockFrequency = 24_000_000 }, device);
    var controller = new System.Device.Gpio.GpioController(System.Device.Gpio.PinNumberingScheme.Logical, driver);

    Ssd1351 display = new(ftSpi, dataCommandPin: 5, resetPin: 6, gpioController: controller);
    return display;
}

Screenshot_20210809-101417~2

/// </summary>
/// <returns>I2cBus instance</returns>
/// <remarks>You can create either an I2C, either an SPI device.</remarks>
public I2cBus CreateI2cBus() => new Ft232HI2cBus(this);
Copy link
Member

Choose a reason for hiding this comment

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

should this be abstract on the FtDevice? Same question for GPIO, SPI - we could return null or throw if given device doesn't support the protocol

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of this. We can definitely do it for I2CBuc, SpiDevice and GpioDriver

Copy link
Contributor

Choose a reason for hiding this comment

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

we could return null or throw if given device doesn't support the protocol

If it doesn't support it, I'd vote throw

/// <returns>A GPIO Driver</returns>
public GpioDriver CreateGpioDriver() => new Ft232HGpio(this);

internal bool IsI2cMode { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest make this public and abstract, non-settable in FtDevice, perhaps rename to IsI2cAvailable then GetI2cBus could throw if it's not available. Similar for SPI/GPIO

Copy link
Member Author

Choose a reason for hiding this comment

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

The IsI2cAvailable, IsSpiAvailable, IsGpioAvailable is a good idea. I can add. The IsI2cMode is there to say that I2C is activated. So I can rename this one as IsI2cModeEnabled, can do the same for the 2 others.
As for this board, you can't have both I2c and Spi activated, I had to have this way to check.
What do you say about the properties to expose what's available and not?

Write(toSend);
}

private void SetupMpsseMode()
Copy link
Member

Choose a reason for hiding this comment

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

optional: tiny comment what Mpsse means would be useful


private void SetupMpsseMode()
{
// Seems that we have to send a wrong command to get the MPSSE moed working
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Seems that we have to send a wrong command to get the MPSSE moed working
// Seems that we have to send a wrong command to get the MPSSE mode working


internal void SpiWrite(SpiConnectionSettings settings, ReadOnlySpan<byte> buffer)
{
if (buffer.Length > 65535)
Copy link
Member

Choose a reason for hiding this comment

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

do all the pre-checks need to happen on all writes? should settings be part of the initialization instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

settings is the easy way to get the chipselect and the default state of the chip select pin. You can have as many SpiDevice as you want, just limited by the number of GPIO available. So a maximum of 13.
Now for the size of the buffer, I have to say I was mazy. I can do a loop and send the sliced buffer as many times as needed. Will consider to add this. This will make less checks


internal void SpiRead(SpiConnectionSettings settings, Span<byte> buffer)
{
if (buffer.Length > 65535)
Copy link
Member

Choose a reason for hiding this comment

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

same question about checks


internal void SpiWriteRead(SpiConnectionSettings settings, ReadOnlySpan<byte> bufferWrite, Span<byte> bufferRead)
{
if ((bufferRead.Length > 65535) || (bufferWrite.Length > 65535))
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

(disregard comment for buffers - that's correct, only meant settings)


internal void SpiChipSelectEnable(byte chipSelect, bool enable)
{
if (chipSelect < 0)
Copy link
Member

@krwq krwq Aug 10, 2021

Choose a reason for hiding this comment

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

chipSelect is unsigned, unless you meant to use sbyte

Copy link
Member Author

Choose a reason for hiding this comment

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

right. Will adjust. Forgot to do it there. I do support SPI Device without chipselect. Great catch!

Comment on lines 727 to 730
if (!enable)
{
value = value == PinValue.High ? PinValue.Low : PinValue.High;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about this, I'd expect something like this here:

Suggested change
if (!enable)
{
value = value == PinValue.High ? PinValue.Low : PinValue.High;
}
var value = enabled ? conn.ChipSelectLineActiveState : !conn.ChipSelectLineActiveState;

throw new IOException("Can't read device");
}

totalBytesRead += (int)bytesToRead;
Copy link
Member

Choose a reason for hiding this comment

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

I know they're equal but I'd suggest to use numBytesRead here as it's easier to see this is correct without reading all the ifs

bytesToRead = GetAvailableBytes();
if (bytesToRead > 0)
{
ftStatus = FtFunction.FT_Read(_ftHandle, in buffer[totalBytesRead], bytesToRead, ref numBytesRead);
Copy link
Member

Choose a reason for hiding this comment

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

should numBytesRead be out?

Copy link
Member Author

Choose a reason for hiding this comment

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

on the other API it's ref which is used. So I prefer to keep ref so it's homogeneous.

private uint GetAvailableBytes()
{
uint availableBytes = 0;
var ftStatus = FtFunction.FT_GetQueueStatus(_ftHandle, ref availableBytes);
Copy link
Member

Choose a reason for hiding this comment

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

the name suggest the queue could mean either read or write queue but you always use it in combination with reading, I'd suggest changing the name of this method and explain that i.e.: write queue will always be empty because we wait on write so this actually always means read queue

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are the name of the FTDI API. I can adjust them but for reference to the docs, I would prefer to keep them like they are to make it easier later to debug or implement something new.
And you are correct about what it does, it only checks how many bytes are available to read. I'll adjust the docs of the function.

return totalBytesRead;
}

private bool FlushBuffer()
Copy link
Member

Choose a reason for hiding this comment

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

since this means read all available bytes I'd suggest to call it: DiscardInput()

}

if (DeviceInformation.IsI2cMode && (
(pinNumber >= 0) && (pinNumber <= 2)))
Copy link
Member

Choose a reason for hiding this comment

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

tiny comment above why 0,1,2 would be enough here :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Will definitely add there! And I've explained that in the main readme :-) So I can just copy/paste :-)

throw new ArgumentException($"Can't open pin 0, 1 or 2 while SPI mode is on");
}

if (DeviceInformation._connectionSettings.Where(m => m.ChipSelectLine == pinNumber).Any())
Copy link
Member

Choose a reason for hiding this comment

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

_connectionSettings - this variable should be called as ConnectionSettings because it's either internal or public

throw new ArgumentException($"Pin already open or used as Chip Select");
}

DeviceInformation._PinOpen[pinNumber] = true;
Copy link
Member

Choose a reason for hiding this comment

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

similar comment about naming of _PinOpen

/// <inheritdoc/>
protected override bool IsPinModeSupported(int pinNumber, PinMode mode)
{
if ((mode == PinMode.InputPullDown) || (mode == PinMode.InputPullUp))
Copy link
Member

Choose a reason for hiding this comment

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

this is equivalent to return !((mode == PinMode.InputPullDown) || (mode == PinMode.InputPullUp));

I'd suggest to do a positive condition though, i.e.: return mode == PinMode.Input || mode == PinMode.Output;

@@ -411,7 +412,7 @@ Pn5180 Ft4222Pn5180()
Console.WriteLine();
}

var (chip, dll) = FtCommon.GetVersions();
var (chip, dll) = Ft4222Common.GetVersions();
Copy link
Member

Choose a reason for hiding this comment

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

could FtCommon enumerate both Ft4222s and Ft232?

Copy link
Member Author

Choose a reason for hiding this comment

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

because FT4222 have a specific set of API, the GetVersions is only available with FT4222.

@@ -50,7 +51,7 @@
else if (platformChoice.KeyChar == '2')
{
WriteLine("Creating an instance of a CCS811 using FT4222 drivers.");
using (var i2cBus = FtCommon.GetDevices()[0].CreateI2cBus())
using (var i2cBus = new Ft4222Device(FtCommon.GetDevices()[0]).CreateI2cBus())
Copy link
Member

Choose a reason for hiding this comment

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

could FtCommon enumerate both Ft4222 and Ft232? This way we could create a device regardless which device someone is using

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that should be possible. Each device has a unique type, so I can check the type of each device.
So a GetDevices() will return the FT4222Device only. That's doable.

{
if (pinNumber < 8)
{
var val = DeviceInformation.GetGpioValuesLow();
Copy link
Member

Choose a reason for hiding this comment

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

could GetGpioValues() combine both Low and High into a single uint16?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it's then 2 different calls of the API. So you'll write and read the same number of time than like that.

{
if (value == PinValue.High)
{
DeviceInformation._gpioLowData |= (byte)(1 << pinNumber);
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to have a single uint16 for both low and high and make SetGpioValues() write both? I think that should be fairly cheap

Copy link
Member

Choose a reason for hiding this comment

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

treat as optional since this is an implementation detail although I'd prefer we didn't touch fields starting with underscore unless in the same class

Copy link
Member Author

Choose a reason for hiding this comment

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

the reason there are 2 is because the set of API is different for each. the "Low" and the "Hight". Also the "Low" is used as well by I2C and SPI.

}

/// <inheritdoc/>
public override byte ReadByte()
Copy link
Member

Choose a reason for hiding this comment

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

isn't the default implementation gonna do the same? if so, I'd suggest to remove this. The only time you should override this if you can provide faster implementation than default (i.e. specific interop for reading a single byte)

Copy link
Member

Choose a reason for hiding this comment

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

same comment for WriteByte

Copy link
Member Author

Choose a reason for hiding this comment

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

there is no default implementation, the I2cDevice class is abstract


for (int i = 0; i < buffer.Length; i++)
{
ack = DeviceInformation.I2cSendByteAndCheckACK(buffer[i]);
Copy link
Member

Choose a reason for hiding this comment

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

isn't driver providing any way to Write and Read at the same time?

var ack = DeviceInformation.I2cSendDeviceAddrAndCheckACK((byte)deviceAddress, false);
if (!ack)
{
DeviceInformation.I2cStop();
Copy link
Member

Choose a reason for hiding this comment

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

consider wrapping the whole code in try..finally and putting I2cStop in the finally block and then get rid of it in all fail paths

@pgrawehr
Copy link
Contributor

@Ellerbach Derived FtDevice from Board now. Please test whether it works as expected, as I do not have such a device.

One additional finding: You might want to test whether GPIO and I2C/SPI work in parallel, since the Ft4222GpioDriver and the Ft4222I2cDriver both open the device individually.

@mhutch
Copy link
Member

mhutch commented Aug 27, 2021

Excited to see this! I have a 2232H and a 4232H and would be happy to test them. They're basically a double and quadruple version of the 232H.

@Ellerbach
Copy link
Member Author

@mhutch correct and the FT232H code can be adjusted fairly easily to support the 2232 and 4232 versions. I just don't have them at all, so I cannot test. But all up, the hard and core part is done!

Copy link
Member Author

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

One additional finding: You might want to test whether GPIO and I2C/SPI work in parallel, since the Ft4222GpioDriver and the Ft4222I2cDriver both open the device individually.

Thanks. I will check and find some time early this week to go thru all this. I see what I missed when trying to do it. Thanks again.


namespace Iot.Device.Ft4222
namespace Iot.Device.FtCommon
{
/// <summary>
/// FT4222 device information
Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried and it's actually more complicated as it does introduce elements which are not super easy to use later. Also, for some reason, it never let me implement the Board abstract class, always wanted me to use the GenericBoard or the others.
You may want to show me how to properly do that, I may have missed something somewhere.

@joperezr joperezr assigned Ellerbach and unassigned joperezr Sep 2, 2021
@joperezr
Copy link
Member

joperezr commented Sep 2, 2021

[Triage] Pending one last look by @pgrawehr and testing the changes by @Ellerbach

@Ellerbach
Copy link
Member Author

Thanks @pgrawehr all seems ok. During the series of tests, I found an issue on my side when the GpioController is used alone. So was great to catch it and fix it as well.

Now all is ready to merge!

@pgrawehr
Copy link
Contributor

pgrawehr commented Sep 5, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pgrawehr
Copy link
Contributor

pgrawehr commented Sep 6, 2021

@joperezr Could #1654 have broken this build? This one was building before the latest merge.

@joperezr joperezr added the Auto-Merge When added to a PR, it will be automatically merged after all checks pass. label Sep 7, 2021
@msftbot
Copy link
Contributor

msftbot bot commented Sep 7, 2021

Hello @joperezr!

Because this pull request has the Auto-Merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@joperezr joperezr merged commit f3870a8 into dotnet:main Sep 7, 2021
@dotMorten
Copy link
Contributor

Nice! Thank you all! This'll make my blogpost a lot shorter now https://xaml.dev/post/Building-a-side-kick-display-for-your-PC-with-NET

@joperezr
Copy link
Member

joperezr commented Sep 7, 2021

Thanks so much for sharing @dotMorten! It would be nice if we add some good blogs talking about real-world end-to-end examples like yours somewhere in our docs. I believe @microhobby also had some cool projects he worked on and used dotnet/iot that are worth including in our docs as well.

@dotMorten
Copy link
Contributor

@joperezr Thanks! You're more than welcomed to reference it. The project evolved a bit because I wanted a larger display, hence why I also submitted PR #1642

@Ellerbach
Copy link
Member Author

Nice! Thank you all! This'll make my blogpost a lot shorter now https://xaml.dev/post/Building-a-side-kick-display-for-your-PC-with-NET

Very cool and happy being a part of this :-) I should just make the same one :-) I have few of those little screens :-) That gives me ideas.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio Auto-Merge When added to a PR, it will be automatically merged after all checks pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants