Adding FT232H #1628

Ellerbach · 2021-08-09T17:02:38Z

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

dotMorten · 2021-08-09T17:15:58Z

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;
}

krwq · 2021-08-10T08:20:15Z

src/devices/Ft232H/Ft232HDevice.cs

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


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

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

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

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

krwq · 2021-08-10T08:21:31Z

src/devices/Ft232H/Ft232HDevice.cs

+        /// <returns>A GPIO Driver</returns>
+        public GpioDriver CreateGpioDriver() => new Ft232HGpio(this);
+
+        internal bool IsI2cMode { get; set; }


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

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?

krwq · 2021-08-10T08:22:50Z

src/devices/Ft232H/Ft232HDevice.cs

+            Write(toSend);
+        }
+
+        private void SetupMpsseMode()


optional: tiny comment what Mpsse means would be useful

krwq · 2021-08-10T08:23:05Z

src/devices/Ft232H/Ft232HDevice.cs

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


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

krwq · 2021-08-10T08:24:21Z

src/devices/Ft232H/Ft232HDevice.cs

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


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

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

krwq · 2021-08-10T08:24:34Z

src/devices/Ft232H/Ft232HDevice.cs

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


same question about checks

krwq · 2021-08-10T08:24:43Z

src/devices/Ft232H/Ft232HDevice.cs

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


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

krwq · 2021-08-10T08:25:52Z

src/devices/Ft232H/Ft232HDevice.cs

+
+        internal void SpiChipSelectEnable(byte chipSelect, bool enable)
+        {
+            if (chipSelect < 0)


chipSelect is unsigned, unless you meant to use sbyte

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

krwq · 2021-08-10T08:28:14Z

src/devices/Ft232H/Ft232HDevice.cs

+            if (!enable)
+            {
+                value = value == PinValue.High ? PinValue.Low : PinValue.High;
+            }


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;

krwq · 2021-08-10T08:30:08Z

src/devices/Ft232H/Ft232HDevice.cs

+                        throw new IOException("Can't read device");
+                    }
+
+                    totalBytesRead += (int)bytesToRead;


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

krwq · 2021-08-10T08:30:25Z

src/devices/Ft232H/Ft232HDevice.cs

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


should numBytesRead be out?

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

krwq · 2021-08-10T08:32:17Z

src/devices/Ft232H/Ft232HDevice.cs

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


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

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.

krwq · 2021-08-10T08:33:22Z

src/devices/Ft232H/Ft232HDevice.cs

+            return totalBytesRead;
+        }
+
+        private bool FlushBuffer()


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

krwq · 2021-08-10T08:34:47Z

src/devices/Ft232H/Ft232HGpio.cs

+            }
+
+            if (DeviceInformation.IsI2cMode && (
+                (pinNumber >= 0) && (pinNumber <= 2)))


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

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

krwq · 2021-08-10T08:35:47Z

src/devices/Ft232H/Ft232HGpio.cs

+                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())


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

krwq · 2021-08-10T08:36:12Z

src/devices/Ft232H/Ft232HGpio.cs

+                throw new ArgumentException($"Pin already open or used as Chip Select");
+            }
+
+            DeviceInformation._PinOpen[pinNumber] = true;


similar comment about naming of _PinOpen

krwq · 2021-08-10T08:52:18Z

src/devices/Ft232H/Ft232HGpio.cs

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


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;

krwq · 2021-08-10T14:29:09Z

src/devices/Card/Ndef/samples/Program.cs

    }

-    var (chip, dll) = FtCommon.GetVersions();
+    var (chip, dll) = Ft4222Common.GetVersions();


could FtCommon enumerate both Ft4222s and Ft232?

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

krwq · 2021-08-10T14:29:45Z

src/devices/Ccs811/samples/Program.cs

 {
    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())


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

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.

krwq · 2021-08-10T14:31:11Z

src/devices/Ft232H/Ft232HGpio.cs

+        {
+            if (pinNumber < 8)
+            {
+                var val = DeviceInformation.GetGpioValuesLow();


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

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

krwq · 2021-08-10T14:32:05Z

src/devices/Ft232H/Ft232HGpio.cs

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


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

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

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.

krwq · 2021-08-10T14:35:54Z

src/devices/Ft232H/Ft232HI2c.cs

+        }
+
+        /// <inheritdoc/>
+        public override byte ReadByte()


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)

same comment for WriteByte

there is no default implementation, the I2cDevice class is abstract

krwq · 2021-08-10T14:37:17Z

src/devices/Ft232H/Ft232HI2cBus.cs

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


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

krwq · 2021-08-10T14:38:49Z

src/devices/Ft232H/Ft232HI2cBus.cs

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


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

Functionally, there should be no change.

pgrawehr · 2021-08-27T20:06:59Z

@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 · 2021-08-27T22:10:33Z

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 · 2021-08-30T09:13:14Z

@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!

Ellerbach

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.

Ellerbach · 2021-08-11T15:59:04Z

src/devices/FtCommon/FtDevice.cs

+namespace Iot.Device.FtCommon
 {
    /// <summary>
    /// FT4222 device information


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 · 2021-09-02T15:58:26Z

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

Ellerbach · 2021-09-03T08:33:15Z

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 · 2021-09-05T18:19:01Z

/azp run

azure-pipelines · 2021-09-05T18:19:11Z

Azure Pipelines successfully started running 1 pipeline(s).

pgrawehr · 2021-09-06T15:13:59Z

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

ghost · 2021-09-07T17:05:23Z

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.

dotMorten · 2021-09-07T18:21:32Z

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 · 2021-09-07T18:29:23Z

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 · 2021-09-07T19:12:37Z

@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 · 2021-09-08T06:18:01Z

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.

Adding FT232H, adjusting FT4222 and related samples

01bf671

ghost 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

adding category file

828a4d2

Ellerbach added 2 commits August 9, 2021 19:21

adjusting nit

266a2a6

fixing broken name in sample

ac7fd2a

krwq reviewed Aug 10, 2021

View reviewed changes

joperezr assigned pgrawehr and joperezr Aug 26, 2021

pgrawehr added 3 commits August 26, 2021 22:01

Derive FtDevice from Board

80cfc33

Functionally, there should be no change.

Update FT4222 to derive from Board

e6d5c48

Fix usages in examples

2869c95

Add exception in case of invalid bus Id

cc43e6c

Ellerbach commented Aug 30, 2021

View reviewed changes

joperezr assigned Ellerbach and unassigned joperezr Sep 2, 2021

Ellerbach added 2 commits September 3, 2021 08:52

Merge branch 'main' of https://github.com/dotnet/iot into add-ft232h

16ef72b

Adjusting GPIO initialization when used alone

34cfe79

pgrawehr approved these changes Sep 3, 2021

View reviewed changes

Merge branch 'main' into add-ft232h

9641d7a

Fixing project configurations caused by incorrect merge

675c717

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

joperezr merged commit f3870a8 into dotnet:main Sep 7, 2021

github-actions bot locked and limited conversation to collaborators Dec 9, 2023

	// 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

Adding FT232H #1628

Adding FT232H #1628

Uh oh!

Conversation

Ellerbach commented Aug 9, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

dotMorten commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ellerbach commented Aug 9, 2021 •

edited by ghost

Loading

dotMorten commented Aug 9, 2021 •

edited

Loading

krwq Aug 10, 2021 •

edited

Loading