dcd_lpc_ip3511: isochronous support and endpoint accidental write fix #1676
Conversation
tswan-quasi
changed the title
|
It seems that the checks are failing based on some MCUs that I did not modify files for? Did I do something wrong or how should I go about resolving this? |
no worries, it is ci issue, sometime when running multiple jobs too often. It runs out of bandwidth or got into network issue. I have re-run ci. Please give me a bit of time for reviewing and pulling out an lpc55 board for testing. |
Ok no problem, thanks for letting me know |
There was a problem hiding this comment.
Thank you very much for the PR. I have tested both audio on HS and video on FS. Both works great, video HS does not seem to work (though it is probably issue with UVC or something else). The PR is almost ready for merging. Though I still have an question regarding dummy bytes, do we really need it. Have you experience issues without it ?
PS: I made some push to rename the enum name (also add an TODO for myself, since I found some mistake with fs/hs buffer layout when checking with LPC55 UM). Please pull from your fork first, before making any commits.
| @@ -174,6 +178,7 @@ typedef struct | |||
| // For example: LPC55s69 port1 Highspeed must be USB_RAM (0x40100000) | |||
| // Use CFG_TUSB_MEM_SECTION to place it accordingly. | |||
| CFG_TUSB_MEM_SECTION TU_ATTR_ALIGNED(256) static dcd_data_t _dcd; | |||
| CFG_TUSB_MEM_SECTION TU_ATTR_ALIGNED(256) static volatile uint8_t dummy[64]; // TODO temp fix to prevent accidental overwrite due to ep[][].buffer_xx.offset being 0 | |||
There was a problem hiding this comment.
do we really need to have this. Have you experience issue with dcd data is overwritten. USB data shouldn't be accepted when active bit is not set
There was a problem hiding this comment.
I think there is likely a better solution, but it was causing me some issues not assigning the offsets to another location. I am however using a different build setup than the one provided in the repo, I tried to follow the defines as they are in the family makefile, hopefully this does not cause any discrepancies.
In functions such as dcd_init() and others, the USB would write to the offset in _dcd.ep[...][...].buffer_xx.offset and I think because the value of the DATABUFSTART register was 0x2000000, the USB was overwriting some other values I had in SRAM. The reason I noticed it was because the LED blink_interval_ms in main.c was not changing correctly and it happened to be placed at 0x20000000 in my build. First place it happened was after dcd_reg->DEVCMDSTAT |= ... in dcd_init()
A roundabout solution could be to always have the USB operate in USB_SRAM regardless of FS or HS, then in edpt_reset() and edpt_reset_all() have the offset value be set based on the start of USB_SRAM (0x40100000) since setting to zero could potentially cause issues with the APB bus which start at 0x40000000 which is what the value of DATABUFSTART would be. I could see this potentially having other issues though.
There was a problem hiding this comment.
it is not clear enough that USB overwrite the data, even if that is the case, I think we should properly fix it e.g explicitly set SKIP/DISABLE bit for endpoint first before zero it out. Otherwise we could run into issue that host may think we already received data (overwritten to zero), while it is ended up in dummy. Maybe we should do it in a separated PR.
PS: Do you have an easy/reliable way to reproduce this issue, I will try my luck to troubleshoot it
There was a problem hiding this comment.
I think I noticed it on the video example when I was testing because blink_interval_ms was getting overwritten at address 0x20000000. I will take a look again as well as seeing if I can find proper solution.
There was a problem hiding this comment.
A repeatable setup for me:
- example: video_capture
- modifications: revert all lines using
dummyto before - OS: Windows
- when USB is asleep it blinks as intended. open the camera app, once the device is awake, the value of
blink_interval_msis 0 (address 0x20000000) when it should be 1000
There was a problem hiding this comment.
I think I have narrowed it down to dcd_edpt_xfer() when buffer is NULL, occurring for EP0 OUT ZLPs. The code below makes it modify dummy, and the other previous uses of dummy can be removed. From what I have seen, the USB writes 4 bytes of zeros to its offset location in this scenario. I am not sure if there is a proper solution to this aside from a dummy buffer?
bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t total_bytes)
{
uint8_t const ep_id = ep_addr2id(ep_addr);
tu_memclr(&_dcd.dma[ep_id], sizeof(xfer_dma_t));
_dcd.dma[ep_id].total_bytes = total_bytes;
if (!buffer && !ep_id)
{
buffer = (uint8_t*)(uint32_t)dummy;
}
prepare_ep_xfer(rhport, ep_id, get_buf_offset(buffer), total_bytes);
return true;
}
I think the UVC was only setup with FS support unless something has changed from #1118 original description |
agreed, it is probably the issue with UVC than the driver. |
There was a problem hiding this comment.
Thank you very much for the PR. I am able to verified the issue with ZLP. It probably caused by USB/DMA controller, even though we schedule zero byte. The controller still clear/reset buffer probably due to its bug. I made it a bit generic to use dummy whenever we use ZLP for all endpoint (not only EP0) and for both direction. Will merge when ci passed.
PS: on my makefile setup, the overwritten memory is SystemCoreClock variable, and has no visual impact once setup and running.
Describe the PR
Added support for isochronous endpoints as well as stopped the USB from accidentally writing the incorrect address upon endpoint resets for dcd_lpc_ip3511
Additional context
Tested on an LPC55S69JBD100 using the LPC55S69-EVK. Only the FS port was tested, not HS.
Isochronous Examples Tested: headset, video
Other Examples Tested: cdc_msc, hid