multi-language STRING Desc support #122

eZioPan · 2023-07-21T12:03:17Z

add a ~~extra_lang_ids: Option<&'a [u16]>~~ extra_lang_ids: Option<&'a [LangID]> to struct Config to store LANGIDs.

extend descriptor_type::STRING branch of fn get_descriptor(), so that it will send all support LANGID in first STRING Desc, and try responding every STRING Request with matched LANGID.

add a ~~.extra_lang_ids()~~ .set_extra_lang_ids() method to UsbDeviceBuilder, which will take a ~~&[u16]~~ &[LangID] to add extra support LANGID.

Breaking

UsbDeviceBuilder::manufacturer() UsbDeviceBuilder::product() UsbDeviceBuilder::serial_number() will ask a &[&str] as input, first &str in &[&str] will match English lang, the rest will match what have been put into ~~.extra_lang_ids()~~ .set_extra_lang_ids().

ryan-summers

Looking really cool. Glad to see we're adding support for non-English language :)

Some minor feedback requested to make this more robust

src/device.rs

src/device_builder.rs

Cross check the list length between `extra_lang_ids`, `manufacturer`, `product` and `serial_number` on constructing `UsbDeviceBuilder`. Build `lang_id_bytes` with rust iterators in `descriptor_type::STRING` of `fn get_descriptor()`. A quick impl `LangID` enum.

eZioPan · 2023-07-25T14:55:18Z

hi, I have do some modifications.
And LangID implementation is really ugly and buggy, would you give me some hints?

src/descriptor/lang_id.rs

src/device.rs

Replace `u16` to `LangID` impl with `num_enum::TryFromPrimitive`. Add `defmt` formatter for `LangID`. Reject any requests that asking lang_ids which is not supported by device.

ryan-summers

Looking really good - one last request

src/device_builder.rs

Co-authored-by: Ryan Summers <[email protected]>

eZioPan · 2023-07-26T12:40:05Z

hi, I still got a few concerns.

~~Currently the max count of lang_id is limit to 16, which isn't forced by USB Spec. I pick this number "randomly". I'm not sure if it's a good idea.~~
* If this limitation is ok, then maybe we need to specify this number as a const value, such that we can easily modify it when some one meets the limit. This number has been used in device_buider.rs and device.rs(to specify lang_id_bytes length), so maybe write a const in lib.rs file? What would you sugguest?
thanks for @ithinuel , now it's not a problem!
The variants of LangID enum is dumped from MS-LCID. I did some modification, I'm not very sure about my modification is ok. And if these are ok, then shall we doc something on LangID enum?
1. I manually remove every LANGID mark as Reserved, Neither defined nor reserved, temporarily assigned, etc.
  I keep the LANGID both marked with a language and Reserved.
2. I blindly replace hyphen with underscore, and make every character upper case(since 0x004D named as, happens to be a keyword of rust).
3. ff-NG and ff-Latn-NG occupy the same LANGID 0x0467, I concat them to FF_NG__FF_LATN_NG
Different Host USB stack may have different LANGID implemtations, and since LANGID has been marked as "deprecated" both by USB.org and MS. Should we doc something on LangID enum?
shall we export LangID enum in prelude mod?

ithinuel

I wonder if it wouldn't be simpler/less costly to check that EN_US is part of the custom lang_ids list at the time it is set, rather than having constructs like [LangID::EN_US].iter().chain(extra_lang_ids.iter()).

See: https://github.com/eZioPan/usb-device/compare/multi-language-support...ithinuel:usb-device:PR122

eZioPan · 2023-07-26T19:47:15Z

Hi @ithinuel , thanks for the work. I think it's a good solution. Let see what @ryan-summers will say about this.

And I will sugguest to ensure that LangID::EN_US be the first lang_id in the list, rather only containing it.

ithinuel · 2023-07-27T06:16:46Z

A modified my proposal to check LangID::EN_US is first.
I also added a commit that should lift the arbitrary limit to the number of languages.

eZioPan · 2023-07-27T08:22:05Z

@ithinuel it looks really cool. I have test on my device, and find that there could be a minor bug.
In src/device.rs Line 560, currently it is if len < buf.len(), I think it should be if buf.len() < len ?

ryan-summers

I'd prefer if we just got the groundwork in here, and maybe @ithinuel wants to follow-up with a new PR with their improvements? :)

Thanks to both of you for the great work on this!

src/device_builder.rs

eZioPan · 2023-07-27T09:04:07Z

I have reverse the change, keep 16 lang_ids limitation.

ryan-summers · 2023-07-27T09:12:20Z

Different Host USB stack may have different LANGID implemtations, and since LANGID has been marked as "deprecated" both by USB.org and MS. Should we doc something on LangID enum?

Please do - if this is the case, I'm less concerned about the overall "correctness" of the various lang IDs - if people hit issues, they can submit PRs to fix it, I don't see a huge risk in a slightly incorrect lang ID outside of potentially failing to enumerate properly on some PC dialects, but given that lang ID is deprecated, I imagine this will go away in the future anyways.

shall we export LangID enum in prelude mod?

No, as long as LangID is publicly accessible from the crate. prelude is generally useful for traits that people aren't explicitly instantiating in their code.

ryan-summers · 2023-07-27T09:16:44Z

If you want to add documentation to LangID, feel free. If not, I'll merge as-is in a little while.

In general, I think we could use a rewrite of the builder to be something like .extra_language([LangID::DE, [manufacturer_in_german, product_in_german, serial_in_german]]) - I think this would solve the weirdness of this current API, but it's also a niche usecase

eZioPan · 2023-07-27T09:24:07Z

Thanks, I would ask a merge. I'm afraid that I'm not very good at English, so maybe I just leave it.
What I can provide is that currently this LANGID is dumped from MS-LCID Protocol Revision 15.0, so if anyone find a bug, their can fetch some information from the MS side.

ryan-summers · 2023-07-27T09:26:59Z

I understand :) Thanks a lot for working on this! It's been greatly appreciated

eZioPan added 3 commits July 21, 2023 19:50

multi-language STRING Desc support

a3b846a

Update CHANGELOG.md for multi-language STRING desc support

9124768

Update CHANGELOG.md

10daddc

eZioPan marked this pull request as ready for review July 21, 2023 12:27

typo fix

87deb19

ryan-summers requested changes Jul 25, 2023

View reviewed changes

ryan-summers reviewed Jul 25, 2023

View reviewed changes

src/device_builder.rs Outdated Show resolved Hide resolved

modification after review

f4f3f3c

Cross check the list length between `extra_lang_ids`, `manufacturer`, `product` and `serial_number` on constructing `UsbDeviceBuilder`. Build `lang_id_bytes` with rust iterators in `descriptor_type::STRING` of `fn get_descriptor()`. A quick impl `LangID` enum.

eZioPan requested a review from ryan-summers July 25, 2023 14:53

replace From with TryFrom for u16 to LangID converstion

283b1f3

ryan-summers reviewed Jul 25, 2023

View reviewed changes

src/descriptor/lang_id.rs Outdated Show resolved Hide resolved

ryan-summers reviewed Jul 25, 2023

View reviewed changes

src/device.rs Outdated Show resolved Hide resolved

modify after another review

f0c2cfe

Replace `u16` to `LangID` impl with `num_enum::TryFromPrimitive`. Add `defmt` formatter for `LangID`. Reject any requests that asking lang_ids which is not supported by device.

eZioPan requested a review from ryan-summers July 26, 2023 10:30

ryan-summers requested changes Jul 26, 2023

View reviewed changes

src/device_builder.rs Outdated Show resolved Hide resolved

src/device_builder.rs Show resolved Hide resolved

src/device_builder.rs Show resolved Hide resolved

always check list length

e7bc3f0

Co-authored-by: Ryan Summers <[email protected]>

always check lang id list length

4dd48d7

eZioPan requested a review from ryan-summers July 26, 2023 15:22

ithinuel reviewed Jul 26, 2023

View reviewed changes

ryan-summers reviewed Jul 27, 2023

View reviewed changes

src/device_builder.rs Outdated Show resolved Hide resolved

src/device_builder.rs Outdated Show resolved Hide resolved

src/device_builder.rs Outdated Show resolved Hide resolved

ryan-summers reviewed Jul 27, 2023

View reviewed changes

src/device_builder.rs Show resolved Hide resolved

list length check only if user already specify "extra_lang_ids"

cf693a0

eZioPan requested a review from ryan-summers July 27, 2023 09:05

ryan-summers approved these changes Jul 27, 2023

View reviewed changes

ryan-summers merged commit 7ebb10f into rust-embedded-community:master Jul 27, 2023

eZioPan deleted the multi-language-support branch July 27, 2023 09:35

ryan-summers mentioned this pull request Nov 2, 2023

Refactoring language management for simplicity #130

Merged

jose-acevedoflores mentioned this pull request Jan 31, 2024

Make LangID optional on get_string() #140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi-language STRING Desc support #122

multi-language STRING Desc support #122

eZioPan commented Jul 21, 2023 •

edited

Loading

ryan-summers left a comment

eZioPan commented Jul 25, 2023 •

edited

Loading

ryan-summers left a comment

eZioPan commented Jul 26, 2023 •

edited

Loading

ithinuel left a comment •

edited

Loading

eZioPan commented Jul 26, 2023 •

edited

Loading

ithinuel commented Jul 27, 2023

eZioPan commented Jul 27, 2023

ryan-summers left a comment

eZioPan commented Jul 27, 2023

ryan-summers commented Jul 27, 2023 •

edited

Loading

ryan-summers commented Jul 27, 2023 •

edited

Loading

eZioPan commented Jul 27, 2023

ryan-summers commented Jul 27, 2023

multi-language STRING Desc support #122

multi-language STRING Desc support #122

Conversation

eZioPan commented Jul 21, 2023 • edited Loading

ryan-summers left a comment

Choose a reason for hiding this comment

eZioPan commented Jul 25, 2023 • edited Loading

ryan-summers left a comment

Choose a reason for hiding this comment

eZioPan commented Jul 26, 2023 • edited Loading

ithinuel left a comment • edited Loading

Choose a reason for hiding this comment

eZioPan commented Jul 26, 2023 • edited Loading

ithinuel commented Jul 27, 2023

eZioPan commented Jul 27, 2023

ryan-summers left a comment

Choose a reason for hiding this comment

eZioPan commented Jul 27, 2023

ryan-summers commented Jul 27, 2023 • edited Loading

ryan-summers commented Jul 27, 2023 • edited Loading

eZioPan commented Jul 27, 2023

ryan-summers commented Jul 27, 2023

eZioPan commented Jul 21, 2023 •

edited

Loading

eZioPan commented Jul 25, 2023 •

edited

Loading

eZioPan commented Jul 26, 2023 •

edited

Loading

ithinuel left a comment •

edited

Loading

eZioPan commented Jul 26, 2023 •

edited

Loading

ryan-summers commented Jul 27, 2023 •

edited

Loading

ryan-summers commented Jul 27, 2023 •

edited

Loading