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

Improve FT232H, add FT2232H and FT4232H #2019

Merged
merged 7 commits into from Feb 10, 2023
Merged

Conversation

Ellerbach
Copy link
Member

@Ellerbach Ellerbach commented Jan 20, 2023

This PR improves the initial work on FT232H.
It adds as well the full support for FT2232H and FT4232H.
Improvement of the documentation.
Addition for new sample.

Both 3 devices has been tested successfully with I2C using a TSL2561 sensor. GPIO pins tested including for the specific Bit Band mode on FT4232H.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Jan 20, 2023
@krwq krwq requested a review from pgrawehr January 26, 2023 16:34
Copy link
Contributor

@pgrawehr pgrawehr left a comment

Choose a reason for hiding this comment

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

Looks good to me, some minor findings.

Maybe a namespace adjustment would be appropriate? It's hard to find the FT4232 in a folder called FT232 (there's also a FT4222...)

src/devices/Ft232H/Ftx232HDevice.cs Outdated Show resolved Hide resolved
src/devices/Ft232H/Ftx232HDevice.cs Show resolved Hide resolved
src/devices/Ft232H/Ft2232HDevice.cs Show resolved Hide resolved
@Ellerbach
Copy link
Member Author

Maybe a namespace adjustment would be appropriate? It's hard to find the FT4232 in a folder called FT232 (there's also a FT4222...)

This is what I've tried to do:
Ftx232HDevice => FtCommon
Ft2232HDevice => Ft2232H
Ft232HDevice => Ft232H (same as before)
Ft4232HDevice => Ft4232H
Now, I've kept all the Ft232HGpio, Spi, I2c, I2cBus in the Ft232H namespace for compatibility (same as before)
All the above *HDevice uses those ones for the specific Gpio, Spi, I2c and I2cBus operations.

I know the names are not that easy to read without breaking what was present, so if you have a better suggestion, happy to apply it.

@pgrawehr
Copy link
Contributor

pgrawehr commented Feb 2, 2023

Maybe a namespace adjustment would be appropriate? It's hard to find the FT4232 in a folder called FT232 (there's also a FT4222...)

This is what I've tried to do: Ftx232HDevice => FtCommon Ft2232HDevice => Ft2232H Ft232HDevice => Ft232H (same as before) Ft4232HDevice => Ft4232H Now, I've kept all the Ft232HGpio, Spi, I2c, I2cBus in the Ft232H namespace for compatibility (same as before) All the above *HDevice uses those ones for the specific Gpio, Spi, I2c and I2cBus operations.

I know the names are not that easy to read without breaking what was present, so if you have a better suggestion, happy to apply it.

The names of the classes are fine. I was just thinking about changing the namespace. I am aware that this would be a breaking change. We've done it in other places as well, but I leave it to you to decide.

@Ellerbach
Copy link
Member Author

@krwq do you want to have a quick look at it or as Patrick's already reviewed it, I can go and merge it?

@pgrawehr pgrawehr merged commit c0ae4e1 into dotnet:main Feb 10, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants