-
Notifications
You must be signed in to change notification settings - Fork 983
feat: allow bidirectional usb endpoint numbers #5014
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
base: dev
Are you sure you want to change the base?
feat: allow bidirectional usb endpoint numbers #5014
Conversation
3288f5e
to
0f3d0c3
Compare
Ok, all ready for review now. I don't have any atsamd21 boards, but I've tested USB CDC on an atsamd51, nrf52840, and a pico-w as well as USB mass storage on the pico-w and everything looks good to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion, didn't review the code in depth but I see nothing weird at a quick glance.
Did a quick test on a Circuit Playground Express (atsamd21) and it seems to be working fine! |
@mikesmitty I just switched the branch for this to Anyone else have any comments on this PR? |
Sure, I'll squash them all |
ba24793
to
0066bb3
Compare
Before merge, just want to confirm with @sago35 that this does not affect any TinyGo Keeb code. ⌨️ |
I’ll make sure to review it early next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some parts of my TinyGo Keeb code didn’t work, but that turned out to be an issue on my end.
So overall, everything looks really good.
There are just a few parts that may need slight changes, and a couple of things I'd like to understand the reasoning behind.
var ( | ||
EndpointMIDIIN = &EndpointEP4IN | ||
EndpointMIDIOUT = &EndpointEP3OUT | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to move it to src/machine/usb/descriptor/endpoint.go
.
} | ||
if (epFlags & sam.USB_DEVICE_EPINTFLAG_TRCPT1) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What effect does this change have?
Depending on the nature of the change, it might be better to split it into a separate PR.
} | ||
if (epFlags & sam.USB_DEVICE_ENDPOINT_EPINTFLAG_TRCPT1) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What effect does this change have?
Depending on the nature of the change, it might be better to split it into a separate PR.
} | ||
if outDataDone { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What effect does this change have?
Depending on the nature of the change, it might be better to split it into a separate PR.
This one is still a bit fresh, will update once it's had a bit more time to cook