RP2040:Enable full size isochronous buffers#657
Conversation
hathach
left a comment
hathach
left a comment
There was a problem hiding this comment.
Look good to me, though I prefer to merge if() else{} with simple min function against ep max size. Let wait for more feedback from RPI team.
| } | ||
| else | ||
| { | ||
| ep->transfer_size = total_len > 64 ? 64 : total_len; |
There was a problem hiding this comment.
I think this else{} could be merged to the above if() as well if 64 stands for ep size of non-iso here. Unless there is something I don't know of. @liamfraser what do you think, could we just use the max ep size for 64 here on non-ISO endpoints
There was a problem hiding this comment.
64 is the max Non ISO EP size. We could potentially use a define for this.
There was a problem hiding this comment.
The ep size can actually be smaller for example interrupt ep can have max packet size of 8 or 16. Will we use the actual mps (8) here or 64 still is correct ? If 8 is needed then we should change it to wMaxPacketSize (like iso) instead of fixed 64.
There was a problem hiding this comment.
I think we should use wMaxPacketSize everywhere. I think that's what my "// FIXME: What if low speed" was meant for but I never got round to fixing it. I would like there to be a check that wMaxPacketSize is in a valid range for RP2040 somewhere though.
There was a problem hiding this comment.
Yeah, that is what I think as well. Will update pr accordingly. usbd already perform the endpoint size check already before passing it to dcd open, so I guess it is rather OK afterwards.
Line 1111 in 1bba2c0
There was a problem hiding this comment.
Ah. I think I need to swap dcd_event_bus_signal(0, DCD_EVENT_BUS_RESET) with dcd_event_bus_reset(0, TUSB_SPEED_FULL). We don't actually report our speed anywhere. I think the API might have changed since I did the original port.
There was a problem hiding this comment.
Yeah, it changes recently to dynamically support FS/HS. It is mostly for HS capable device to work properly with attached to let's say PICO FS host. Fullspeed device as default should have no issue, don't worry about low speed mode. It is virtually non-existent, no body is interested in it 😄 .
This was a github edit, not tested
…b.com/ndinsmore/tinyusb into ndinsmore-RP2040_enable_isochronous_buffer_size
hathach
left a comment
hathach
left a comment
There was a problem hiding this comment.
update the PR to min check against remaining and ep size. I did accidentally merge from latest master and have to do the trick to change-rechange base for github to update the file difference. All good now, PR will be merged when ci complete. Thank you very much for the fix.
3 participants
Describe the PR
The RP2040 spec allows full-sized isochronous buffers, this enables it.
Additional context
The rp2040 spec limits none ISO buffers to 64 bits.