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
|
I always see the IAR build fail, but with no relation to CDCh. Is it OK? |
| static uint16_t const pl2303_vid_pid_list[][2] = {CFG_TUH_CDC_PL2303_VID_PID_LIST}; | ||
| static const struct pl2303_type_data pl2303_type_data[TYPE_COUNT] = {PL2303_TYPE_DATA}; | ||
|
|
||
| CFG_TUH_MEM_SECTION CFG_TUH_MEM_ALIGN |
There was a problem hiding this comment.
IAR built fail is due to this extra statement.
src/class/cdc/cdc_host.c
Outdated
| // TODO not implemented yet | ||
| return false; | ||
| static void ftdi_set_line_coding_stage1_complete(tuh_xfer_t * xfer) { | ||
| uint8_t const itf_num = (uint8_t) tu_le16toh(xfer->setup->wIndex); |
There was a problem hiding this comment.
here is a bug.
will be fixed in a subsequent commit.
|
thank you very much, I wil review this whenever I could. |
|
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. |
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?