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

initial support of CH34x CDC device #2391

Merged
merged 24 commits into from
Jan 22, 2024
Merged

Conversation

IngHK
Copy link

@IngHK IngHK commented Dec 26, 2023

this is the initial version to support the CH34x CDC device.
the CH34x is as popular as FTDI or CH34x.
this version supports only RX and TX and the line_coding settings.
modem control handling will follow...

@IngHK
Copy link
Author

IngHK commented Jan 15, 2024

what is the preferred code style?
image
what tool do you use tool to format the code?

@hathach
Copy link
Owner

hathach commented Jan 15, 2024

what is the preferred code style? image what tool do you use tool to format the code?

In short it is K&R 2 spaces. Currently it is quite a bit of a mess, I used to like BSD/allman, but make a switch recently. So you wil mixed style here and there. It will take a while to settle. I tend to only change style within file that I work with.

I don't use any auto format tool, but does occasioally auto-format selection line or file using clion with my own custom of c style. Don't worry much about it, I still prefer manual for readig than auto formmat everywhere.

@IngHK
Copy link
Author

IngHK commented Jan 15, 2024

Thanks!
Please wait with this PR!
I'm finishing the CH34x support and I'll get back to you soon.
Due to compatibility issues I had to change large parts and so current commit is no more ready for review.

@IngHK
Copy link
Author

IngHK commented Jan 15, 2024

It would be nice, if you first review my other PRs.
After they are OK and merged, I will adopt them into the CH34x section.

# Conflicts:
#	src/class/cdc/cdc_host.c
@hathach
Copy link
Owner

hathach commented Jan 16, 2024

Thanks! Please wait with this PR! I'm finishing the CH34x support and I'll get back to you soon. Due to compatibility issues I had to change large parts and so current commit is no more ready for review.

Which compatibility issue you are encoutering with, I actually want to wrap up this ch340 support since I need it in another project . Initial test giving great result, I have fixed some gcc strict warning regarding siged/unsigned cast and do more with code reviews and testing. If you plan to push more, please pull first, I will probably push a few more commits before merging.

@IngHK
Copy link
Author

IngHK commented Jan 16, 2024

There is no CH340/CH341 datasheet or programming guide available from vendor WCH.
I found 3 sources of linux driver I used as base for my implementation:

  1. from vendor WCH (with poor quality)
  2. from Linux Kernel
  3. FreeBSD

All 3 differ in details and are NOT working with all derivatives on my desk, so I need to build an extract and to do many "try and errors".

@IngHK
Copy link
Author

IngHK commented Jan 16, 2024

I just pushed the current but unfinished version.
I'm currently working on ch34x_set_line_coding().
It's not ready to merge to master, but you can work with and test.
It includes all other open PR from me.

@hathach
Copy link
Owner

hathach commented Jan 16, 2024

there is some .bak in the latest push since I am in need to get ch340 support, I will do the clean up right away. Please hold until my push, thank you.

PS: when merging, please also retain my changes, it seems like your latest push discard most of my changes. But that is OK, there is an conflict going on since both of us are updating the driver.

@hathach
Copy link
Owner

hathach commented Jan 16, 2024

I just pushed the current but unfinished version.

I'm currently working on ch34x_set_line_coding().

It's not ready to merge to master, but you can work with and test.

It includes all other open PR from me.

Thank you, I am cherry picking changes and re-added changes that got lost in the latest push. Please wait a bit before making more push. Hopefully we can got it merged asap. I only tested with an usb2uart dongle ch340g, what else are you testing with ?

@IngHK
Copy link
Author

IngHK commented Jan 16, 2024

there is some .bak in the latest push since I am in need to get ch340 support, I will do the clean up right away. Please hold until my push, thank you.

PS: when merging, please also retain my changes, it seems like your latest push discard most of my changes. But that is OK, there is an conflict going on since both of us are updating the driver.

yes, it was a mistake

@IngHK
Copy link
Author

IngHK commented Jan 16, 2024

there is some .bak in the latest push since I am in need to get ch340 support, I will do the clean up right away. Please hold until my push, thank you.

PS: when merging, please also retain my changes, it seems like your latest push discard most of my changes. But that is OK, there is an conflict going on since both of us are updating the driver.

sorry

@IngHK
Copy link
Author

IngHK commented Jan 16, 2024

I just pushed the current but unfinished version.
I'm currently working on ch34x_set_line_coding().
It's not ready to merge to master, but you can work with and test.
It includes all other open PR from me.

Thank you, I am cherry picking changes and re-added changes that got lost in the latest push. Please wait a bit before making more push. Hopefully we can got it merged asap. I only tested with an usb2uart dongle ch340g, what else are you testing with ?

OK.
I tested it with several CH340 and CH341 devices: USB/TTL, USB/RS232 and measurement instrument with integrated CH340.
I tested it with CH340C, CH341A and some other devices, where I'm not able to see the exact letter behind CH340.
But all of them with version 0x30 or newer.

@hathach
Copy link
Owner

hathach commented Jan 16, 2024

there is some .bak in the latest push since I am in need to get ch340 support, I will do the clean up right away. Please hold until my push, thank you.
PS: when merging, please also retain my changes, it seems like your latest push discard most of my changes. But that is OK, there is an conflict going on since both of us are updating the driver.

yes, it was a mistake

no problem at all, I did a cherry picking and push the update to the PR. Please pull first before making more update. We got everything update now.

OK. I tested it with several CH340 and CH341 devices: USB/TTL, USB/RS232 and measurement instrument with integrated CH340. I tested it with CH340C, CH341A and some other devices, where I'm not able to see the exact letter behind CH340. But all of them with version 0x30 or newer.

Thank you for your information. There is no worry to get it all working in the first PR. We can always fix it later on.

@IngHK
Copy link
Author

IngHK commented Jan 17, 2024

Take care when testing!
You need to unplug (power down) and replug the CH34x!
It has no reset to default via USB control requests...

@IngHK
Copy link
Author

IngHK commented Jan 17, 2024

ch34x_set_line_coding() is finished

@hathach
Copy link
Owner

hathach commented Jan 19, 2024

I have pushed the update that implement 2 stage set line coding, also added single set_data_format() as new API. Please test it out to see if that works for you. I will probably do a bit more clean up before merging this.

I'm now implementing ch34x_set_line_coding() and then and then I'm done with CH34x. My goal is to add complete support for all popular CDC devices. And so support for the Prolific PL2303 is missing. And the line coding settings for FTDI and CP210x are still missing. What are your plans regarding finishing FTDI and CP210x support?

And are there plans to implement functions to get the line states CTS, DSR, DCD and RI? => Abstract Control Model Notifications of CDC PSTN Subclass

missinng features will be implemented when needed and/or having free time.

Copy link
Author

@IngHK IngHK left a comment

Choose a reason for hiding this comment

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

I advise not to change that.
There are explicit instructions that default line coding is necessary to start up the CH34x.
For this reason I created CFG_TUH_CDC_LINE_CODING_ON_ENUM_CH34X.
It may happen, that the following steps 'CONFIG_CH34X_SPECIAL_REG_WRITE' etc. will not work properly.
see

@IngHK
Copy link
Author

IngHK commented Jan 21, 2024

I advise not to change that. There are explicit instructions that default line coding is necessary to start up the CH34x. For this reason I created CFG_TUH_CDC_LINE_CODING_ON_ENUM_CH34X. It may happen, that the following steps 'CONFIG_CH34X_SPECIAL_REG_WRITE' etc. will not work properly. see

* https://github.com/torvalds/linux/blob/c25b24fa72c734f8cd6c31a13548013263b26286/drivers/usb/serial/ch341.c#L386

* inital 9600 Bd write https://github.com/WCHSoftGroup/ch341ser_linux/blob/8966d44d8c8f7e5178a53c34984f2e74d655ccb3/driver/ch341.c#L218

When I tried overtaking all three driver sources (WCH, Linux & FreeBSD) one after the other, I found that all 3 did NOT work with all CH34x devices on my desk.
Only the WCH Windows driver V3.5.2019.1 works properly.
I then recorded the USB data traffic of the Windows driver.
The recorded sequences have little in common with those of the 3 Linux sources.
Here are the control sequences:
image

@hathach
Copy link
Owner

hathach commented Jan 22, 2024

I advise not to change that. There are explicit instructions that default line coding is necessary to start up the CH34x. For this reason I created CFG_TUH_CDC_LINE_CODING_ON_ENUM_CH34X. It may happen, that the following steps 'CONFIG_CH34X_SPECIAL_REG_WRITE' etc. will not work properly. see

* https://github.com/torvalds/linux/blob/c25b24fa72c734f8cd6c31a13548013263b26286/drivers/usb/serial/ch341.c#L386

* inital 9600 Bd write https://github.com/WCHSoftGroup/ch341ser_linux/blob/8966d44d8c8f7e5178a53c34984f2e74d655ccb3/driver/ch341.c#L218

When I tried overtaking all three driver sources (WCH, Linux & FreeBSD) one after the other, I found that all 3 did NOT work with all CH34x devices on my desk. Only the WCH Windows driver V3.5.2019.1 works properly. I then recorded the USB data traffic of the Windows driver. The recorded sequences have little in common with those of the 3 Linux sources. Here are the control sequences: image

Thank you for your reviews and fixinng up them, everything makes sense except for the line coding on enum. Have you tried testing with ch34x board without set line coding while enum ? I have tested and it worked just fine with application manually set it in mounted callback

https://github.com/IngHK/tinyusb/blob/cdc_ch34x_support/examples/host/cdc_msc_hid/src/cdc_app.c#L98

For dynamic application such as Arduino, it can have different baudrate for each example (or device it connects to) and a fixed coding does not satify that. For Arduino, it almost always try to set line codinng in the callback, forcinng setting line coding on enum will actualy set line coding twice (enum, then app), which is not neccessary.

Line coding must be set before data transferring, if CFG_TUH_CDC_LINE_CODING_ON_ENUM is not defined, then it is application/user's resposibility.

@IngHK
Copy link
Author

IngHK commented Jan 22, 2024

I advise not to change that. There are explicit instructions that default line coding is necessary to start up the CH34x. For this reason I created CFG_TUH_CDC_LINE_CODING_ON_ENUM_CH34X. It may happen, that the following steps 'CONFIG_CH34X_SPECIAL_REG_WRITE' etc. will not work properly. see

* https://github.com/torvalds/linux/blob/c25b24fa72c734f8cd6c31a13548013263b26286/drivers/usb/serial/ch341.c#L386

* inital 9600 Bd write https://github.com/WCHSoftGroup/ch341ser_linux/blob/8966d44d8c8f7e5178a53c34984f2e74d655ccb3/driver/ch341.c#L218

When I tried overtaking all three driver sources (WCH, Linux & FreeBSD) one after the other, I found that all 3 did NOT work with all CH34x devices on my desk. Only the WCH Windows driver V3.5.2019.1 works properly. I then recorded the USB data traffic of the Windows driver. The recorded sequences have little in common with those of the 3 Linux sources. Here are the control sequences: image

Thank you for your reviews and fixinng up them, everything makes sense except for the line coding on enum. Have you tried testing with ch34x board without set line coding while enum ? I have tested and it worked just fine with application manually set it in mounted callback

https://github.com/IngHK/tinyusb/blob/cdc_ch34x_support/examples/host/cdc_msc_hid/src/cdc_app.c#L98

For dynamic application such as Arduino, it can have different baudrate for each example (or device it connects to) and a fixed coding does not satify that. For Arduino, it almost always try to set line codinng in the callback, forcinng setting line coding on enum will actualy set line coding twice (enum, then app), which is not neccessary.

Line coding must be set before data transferring, if CFG_TUH_CDC_LINE_CODING_ON_ENUM is not defined, then it is application/user's resposibility.

OK, I understand your reasons.
I also don't think it's a good thing that the CH34x is already made ready for reception during the enumeration with 9600 8N1.
However, we don't know whether all CH34x will start correctly if no 9600 8N1 is set during the start-up sequence.
Then, if CFG_TUH_CDC_LINE_CODING_ON_ENUM is not defined, the startup sequence should not be carried out during enumeration, but once at the first call of ch34x_set_line_coding(), just like the Windows driver does.

@hathach
Copy link
Owner

hathach commented Jan 22, 2024

I advise not to change that. There are explicit instructions that default line coding is necessary to start up the CH34x. For this reason I created CFG_TUH_CDC_LINE_CODING_ON_ENUM_CH34X. It may happen, that the following steps 'CONFIG_CH34X_SPECIAL_REG_WRITE' etc. will not work properly. see

* https://github.com/torvalds/linux/blob/c25b24fa72c734f8cd6c31a13548013263b26286/drivers/usb/serial/ch341.c#L386

* inital 9600 Bd write https://github.com/WCHSoftGroup/ch341ser_linux/blob/8966d44d8c8f7e5178a53c34984f2e74d655ccb3/driver/ch341.c#L218

When I tried overtaking all three driver sources (WCH, Linux & FreeBSD) one after the other, I found that all 3 did NOT work with all CH34x devices on my desk. Only the WCH Windows driver V3.5.2019.1 works properly. I then recorded the USB data traffic of the Windows driver. The recorded sequences have little in common with those of the 3 Linux sources. Here are the control sequences: image

Thank you for your reviews and fixinng up them, everything makes sense except for the line coding on enum. Have you tried testing with ch34x board without set line coding while enum ? I have tested and it worked just fine with application manually set it in mounted callback
https://github.com/IngHK/tinyusb/blob/cdc_ch34x_support/examples/host/cdc_msc_hid/src/cdc_app.c#L98
For dynamic application such as Arduino, it can have different baudrate for each example (or device it connects to) and a fixed coding does not satify that. For Arduino, it almost always try to set line codinng in the callback, forcinng setting line coding on enum will actualy set line coding twice (enum, then app), which is not neccessary.
Line coding must be set before data transferring, if CFG_TUH_CDC_LINE_CODING_ON_ENUM is not defined, then it is application/user's resposibility.

OK, I understand your reasons. I also don't think it's a good thing that the CH34x is already made ready for reception during the enumeration with 9600 8N1. However, we don't know whether all CH34x will start correctly if no 9600 8N1 is set during the start-up sequence. Then, if CFG_TUH_CDC_LINE_CODING_ON_ENUM is not defined, the startup sequence should not be carried out during enumeration, but once at the first call of ch34x_set_line_coding(), just like the Windows driver does.

Ok, I agree, setting default for ch34x when CFG_TUH_CDC_LINE_CODING_ON_ENUM not defined is totally OK since it does not take any extra request since it is part of SERIAL_INIT request. I kind of mixed up with linux driver. All is good.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

perfect, the PR is well tested and is optimized for ch34x support. Thank you very much for your patient and following up to my request as well as my modification. I am glad that we are finally getting this merged.

@hathach hathach merged commit 57bbf3a into hathach:master Jan 22, 2024
49 checks passed
@IngHK
Copy link
Author

IngHK commented Jan 22, 2024

perfect, the PR is well tested and is optimized for ch34x support. Thank you very much for your patient and following up to my request as well as my modification. I am glad that we are finally getting this merged.

Thank you, I enjoyed working with you and learned a lot.
I will be doing compatibility tests with the master version in the next few days.

@hathach
Copy link
Owner

hathach commented Jan 23, 2024

thank you, I enjoyed co-working with you on this PR as well.

@IngHK IngHK deleted the cdc_ch34x_support branch February 18, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants