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

usb: unify handling of length parameters #177

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

tobiaskohlbau
Copy link
Contributor

The length parameters in USB packets are strict and the existing implementation does already hardcode the length within the serialize functions. Therefore the user should not provide these lengths during creation time. In order to make this more verbose use defaults values within the structs and reference these defaults values within the serialization functions. It's not possible to remove these fields completly as this fields are used within deserialization from the hardware itself.

The length parameters in USB packets are strict and the existing
implementation does already hardcode the length within the serialize
functions. Therefore the user should not provide these lengths during
creation time. In order to make this more verbose use defaults values
within the structs and reference these defaults values within the
serialization functions. It's not possible to remove these fields
completly as this fields are used within deserialization from the
hardware itself.

Signed-off-by: Tobias Kohlbau <tobias@kohlbau.de>
Copy link
Contributor

@ikskuh ikskuh left a comment

Choose a reason for hiding this comment

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

The two comments are relevant for all changes

core/src/core/usb.zig Outdated Show resolved Hide resolved
@@ -530,7 +530,7 @@ pub const EndpointDescriptor = extern struct {

pub fn serialize(self: *const @This()) [7]u8 {
var out: [7]u8 = undefined;
out[0] = 7; // length
out[0] = self.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should assert self.length to be 7, and hardcode it with either @sizeOf(@This()) or use out.len.

Otherwise, a user could pass in a bad self which has length = 6 or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That a pretty good idea, will implement it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only out.len is possible as the size is reported with padding, which is not correct.

during the serialization and therefore is already hardcoded. The only
other convinience is the calculation of the descriptor size, but this is
already wrong. extern structs are padded and therefore do not have the
right sizes.

Signed-off-by: Tobias Kohlbau <tobias@kohlbau.de>
@tobiaskohlbau
Copy link
Contributor Author

tobiaskohlbau commented Feb 26, 2024

I've updated https://github.com/tobiaskohlbau/microzig/tree/main and both commits are there, looks like GitHub has issues (https://www.githubstatus.com/), most likely therefore there is no information within this PR. If this remains an issue I will force-push my branch in order to trigger the events.

Signed-off-by: Tobias Kohlbau <tobias@kohlbau.de>
@tobiaskohlbau tobiaskohlbau requested a review from ikskuh February 26, 2024 20:00
@tobiaskohlbau
Copy link
Contributor Author

Looks like the CI needs a retry, maybe GitHub still has issues?

Error: Error: server was started but never notified its presence.

https://github.com/ZigEmbeddedGroup/microzig/actions/runs/8054829310/job/22000295495?pr=177

core/src/core/usb.zig Show resolved Hide resolved
@ikskuh ikskuh merged commit 096fda8 into ZigEmbeddedGroup:main Feb 29, 2024
2 checks passed
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