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

Make LangID optional on get_string() #140

Conversation

jose-acevedoflores
Copy link
Contributor

@jose-acevedoflores jose-acevedoflores commented Jan 31, 2024

I've been playing around with the Microsoft OS String Descriptors (specifically to get the winusb driver installed on my device) and everything was working fine on 0.2.9 but the update to 0.3.0 (#122) introduced a behavior difference.

The change in #122 requires that req.index be a valid lang id, if not, it exits early and doesn't call the get_string method.

I also thought lang id could be set to a default in order to not get a breaking change but I think it obscures that LangId might not be set for all requests?

Any thoughts appreciated. I'm just dabbling on embedded stuff so I could be missing something.

src/device.rs Outdated Show resolved Hide resolved
src/device.rs Outdated
@@ -599,7 +602,7 @@ impl<B: UsbBus> UsbDevice<'_, B> {
let index = StringIndex::new(index);
classes
.iter()
.find_map(|cls| cls.get_string(index, lang_id))
.find_map(|cls| cls.get_string(index, lang_id.ok()))
Copy link
Member

Choose a reason for hiding this comment

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

I think defaulting to EN_US for the lang ID is probably a good idea. I don't think Lang ID is even defined in the USB spec anymore.

src/class.rs Outdated Show resolved Hide resolved
@jose-acevedoflores
Copy link
Contributor Author

Made the change to just use EN_US as fallback if LangID is not valid. Let me know if you want me to collapse the two commits into one too.

@ryan-summers ryan-summers merged commit ac36070 into rust-embedded-community:master Feb 26, 2024
3 checks passed
@ryan-summers
Copy link
Member

Wonderful, thanks for the PR! :)

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.

2 participants