CHCh Upgrade: Improved FTDI and CP210x support, add PL2303 support, bugfixes #2488
Conversation
|
@IngHK thank you, though don't worry too much about the number of commits etc, just make it convenient for you. I don't always have time to review things (pending PRs is already long enough), but small change is indeed easier to review. So just group thing together as long as you think that make sense. The current PR only update comment, do you have anything to push more or you want to merge this as it is. |
… and Helper. no functional changes
Yes, I know, the first commit is pretty small, only to warm up ;-) |
|
The last commit improves the TU_LOGs.
I think these are commits that need to be reviewed first. |
|
@IngHK I appreciated your effor to break it out, but please push everything you think work best. Otherwise this make take several months to complete this whole process. I would like to merge portion of code once it is reviewed, and I may only have time to review this 1 or 2 times a month or so. Last time, I could focus on the PR since I need to get ch34x driver running for another paid works. |
…trol_complete() and moved them into driver's sections. no functional change
… value 0 is also valid
…mplete() to be reused by FTDI & CP210x
|
@hathach OK, no problem, we'll do it the way that suits you best. |
| @@ -422,127 +421,6 @@ bool tuh_cdc_read_clear (uint8_t idx) { | |||
| // Control Endpoint API | |||
| //--------------------------------------------------------------------+ | |||
|
|
|||
| static void process_internal_control_complete(tuh_xfer_t* xfer, uint8_t itf_num) { | |||
There was a problem hiding this comment.
The intention here was to move the driver-specific code parts into the driver sections so that the general CDC section does not contain any driver code (as possible).
In addition, cdch_internal_control_complete() had become quite bloated and confusing.
src/class/cdc/cdc_host.c
Outdated
| bool (*const set_baudrate)(cdch_interface_t* p_cdc, uint32_t baudrate, tuh_xfer_cb_t complete_cb, uintptr_t user_data); | ||
| bool (*const set_data_format)(cdch_interface_t* p_cdc, uint8_t stop_bits, uint8_t parity, uint8_t data_bits, tuh_xfer_cb_t complete_cb, uintptr_t user_data); | ||
| bool (*const set_line_coding)(cdch_interface_t* p_cdc, cdc_line_coding_t const* line_coding, tuh_xfer_cb_t complete_cb, uintptr_t user_data); | ||
| bool (*const set_baudrate)(cdch_interface_t* p_cdc, tuh_xfer_cb_t complete_cb, uintptr_t user_data); |
There was a problem hiding this comment.
The intention here was to simplify the parameter passing of the Control Endpoint API calls to the driver functions.
p_cdc->requested_line_coding was already available for FTDI, CP210X and CH34X anyway.
And it makes the internal control complete functions safe and easy.
| @@ -421,115 +421,101 @@ bool tuh_cdc_read_clear (uint8_t idx) { | |||
| // Control Endpoint API | |||
| //--------------------------------------------------------------------+ | |||
|
|
|||
| bool tuh_cdc_set_control_line_state(uint8_t idx, uint16_t line_state, tuh_xfer_cb_t complete_cb, uintptr_t user_data) { | |||
There was a problem hiding this comment.
Since the central flow code of tuh_cdc_set_control_line_state(), tuh_cdc_set_baudrate(), tuh_cdc_set_data_format() and tuh_cdc_set_line_coding() is identical and very tricky, it was outsourced to the common subfunction set_function_call(), which calls the regarding driver's function.
Here you see, why I moved (among other things) the requested line coding and line state into p_cdc->requested..., because it also simplifies the parameter transfer from Control Endpoint API to Driver API independent of the upper mentoined API functions
| TU_LOG_P_CDC("set config complete"); | ||
| p_cdc->mounted = true; | ||
| if (tuh_cdc_mount_cb) tuh_cdc_mount_cb(idx); | ||
| static void set_config_complete(uint8_t idx, uint8_t itf_offset, bool success) { |
There was a problem hiding this comment.
now the enumeration will continue, even if a process config fails
|
If it's easier for you than one huge PR, then I can split it up into smaller PRs that only contain individual and self-contained feature changes |
|
OK. I finished work for now. |
|
thank you, please give me a bit of time to review. |
|
Flyby comment: This has been sitting for over a year. Would an additional reviewer help? Is there some test suite it needs to pass? Would someone updating it to $head help? What would it take to get unstuck? (I have extensive serial tty and USB experience, albeit not with this code. I may be uninvolved in the project, but I can back up running my mouth as a bystander. :-) |
More reviewers will definitely help especially for large PR like this one. @hathach spends most time for Adafruit, I've some free time after work but I'm much less familiar with host stack than device. Personally I feel TUSB is pretty well architectured, it would be pleasant to get involved ;) |
| #define FTDI_SIO_READ_EEPROM 0x90 // Read EEPROM | ||
|
|
||
| // Channel indices for FT2232, FT2232H and FT4232H devices | ||
| #define CHANNEL_A 1 |
There was a problem hiding this comment.
Minor: B-D are never used and A is used in only one place.
Maybe just make that a CHANNEL_OFFSET? (Maybe it's tradition; I'm reviewing in relative isolation.)
| #define CHANNEL_D 4 | ||
|
|
||
| // Port Identifier Table | ||
| #define PIT_DEFAULT 0 // SIOA |
There was a problem hiding this comment.
None of the PIT_ identifiers are used elsewhere in this PR. (Maybe elsewhere, but since they're being introduced here, seems unlikely.)
| #define FTDI_REQUESTED -2 | ||
|
|
||
| // division and round function overtaken from math.h | ||
| #define DIV_ROUND_CLOSEST(x, divisor)( \ |
There was a problem hiding this comment.
Are you concerned that divisor might be zero or that it might be non-integer?
I think you can do a static_assert using either typeid() or typeof() for the latter. The former is probably more important, even if it's an assert.
| TYPE_TB, | ||
| TYPE_HXD, | ||
| TYPE_HXN, | ||
| TYPE_COUNT |
There was a problem hiding this comment.
TYPE_COUNT is never used. Is it better to have a TYPE_UNKNOWN = 0 at the top for use before the board is successfully probed?
|
FWIW, I picked through it last night. (GitHub kept crashing while reviewing it, including during my review submission, which I've just retriggered.) The actual changes are pretty small and are surely non-contentious. This PR looks large because it takes the sin of changing C comments to C++, reformatting, moving code (the main contributor to the size of this being so large as to blow up github's web review), and actual development all in one PR. After I realized that, I quit fussing about "adding" #defines that weren't used and such. I suspect if this had been maybe four mechanical PRs followed by actual code, it wouldn't have been jammed up for a year. It looks like conflicts can be automatically resolved, so it shouldn't take much effort to get this to pass a presubmit if that's gating a review. (It should certainly block the submit.) Anyway, I've laid eyes on the change. They aren't the eyes of this project, but they're moderately well qualified on the underlying technical matters, and this PR looks OK to me. Approved! |
This PR is the kick-off of the series of single commits, which should result in the following improvements:
I think, it's easier to review the changes commit for commit.
After review of each commit, I'll implement and commit it together with the next improvement.
So this PR should be first merged to master at the end.
OK?