usb: host: ch9: Add string descriptor request helper function#102941
usb: host: ch9: Add string descriptor request helper function#102941D-Veda wants to merge 2 commits intozephyrproject-rtos:mainfrom
Conversation
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
This was an utility missing so thank you.
Some question about what should be done in this function, and what should be done by the user.
D-Veda
force-pushed
the
usbh_add_str_desc_helper_func
branch
from
97be2a3 to
6c146e2
Compare
D-Veda
force-pushed
the
usbh_add_str_desc_helper_func
branch
from
6c146e2 to
8cda99d
Compare
D-Veda
force-pushed
the
usbh_add_str_desc_helper_func
branch
from
8cda99d to
96c0c49
Compare
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Some more suggestion for polishing things.
I think closer to completion now.
Thank you!
D-Veda
force-pushed
the
usbh_add_str_desc_helper_func
branch
from
96c0c49 to
e7c8c46
Compare
|
Letting some threads open but issues are addressed, thank you for the changes. |
D-Veda
force-pushed
the
usbh_add_str_desc_helper_func
branch
from
e7c8c46 to
efd5bd7
Compare
|
|
@jfischer-no , can you revisit this PR. |
|
|
||
| bool usbh_desc_is_valid_string(const void *const desc) | ||
| { | ||
| return usbh_desc_is_valid(desc, sizeof(struct usb_string_descriptor), USB_DESC_STRING); |
There was a problem hiding this comment.
Considering how net_buf are typically used, I have doubts how useful this helper is.
| uint8_t langs; | ||
| int ret; | ||
|
|
||
| if (lang_ids == NULL) { |
There was a problem hiding this comment.
Why would it be used this way? This seems too complicated just to get a list of supported LANGIDs.
| if (desc_buf->frags != NULL) { | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Why would there by any?
| has_non_ascii = false; | ||
| utf16le_byte = (const uint8_t *)&str_desc->bString; | ||
| for (copy_idx = 0; copy_idx < MIN(utf16le_str_len, len - 1); copy_idx++) { | ||
| utf16le_code = sys_get_le16(&utf16le_byte[copy_idx * 2]); |
There was a problem hiding this comment.
That is not how we use net_buf. This PR needs complete rework. We have tests for the device string descriptor, so that is roughly what we need. I will open a pull request against your branch (nxp-upstream#21).
D-Veda
force-pushed
the
usbh_add_str_desc_helper_func
branch
from
efd5bd7 to
b86fe4b
Compare
zephyrbot
added
the
area: Tests
| return -EINVAL; | ||
| } | ||
|
|
||
| net_buf_simple_clone(&buf->b, &tmp_buf); |
There was a problem hiding this comment.
What is the reason to clone the buffer?
There was a problem hiding this comment.
The use of net_buf_simple_clone() here to perform a shallow copy is solely to prevent modification of the original data. If net_buf_pull_u8() were used directly to retrieve the data, the header of the original string descriptor would be modified, posing a risk if the caller intends to reuse that descriptor.
Add 4 functions to handle USB string descriptor operations: - usbh_req_desc_str() to retrieve USB string descriptors from device - usbh_desc_is_valid_string() to validate string descriptor type - usbh_desc_get_supported_langs() to get supported languages list - usbh_desc_str_utfle16_to_ascii() to convert UTF-16LE string to ASCII Co-authored-by: Johann Fischer <johann.fischer@nordicsemi.no> Signed-off-by: Dv Alan <zhangyang.shen@nxp.com>
Use the recently introduced descriptor helper in the device stack tests. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
D-Veda
force-pushed
the
usbh_add_str_desc_helper_func
branch
from
b86fe4b to
3c50964
Compare
|
6 participants
Add 4 functions to handle USB string descriptor operations: