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

dcd_da146xx: Implement dcd_edpt_close_all() #1096

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

kasjer
Copy link
Collaborator

@kasjer kasjer commented Sep 20, 2021

Describe the PR
Unconditionally disables all endpoints except EP0.
Related to #1059

Additional context
Not tested yet with USBCV

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

we should also reset the xfer_ctl_t xfer_status[EP_MAX][2]; (except EP0) as well. Since the new configure may have less endpoint than the one we are closing.

Note: for multiple configuration , SET_CONFIG can be set without an bus_reset.

@kasjer
Copy link
Collaborator Author

kasjer commented Sep 23, 2021

@hathach thanks for input, I will update PR.
But before this I will make another PR that will remake power state handling. Bus reset and sleep related code was only testes on live systems. Now when I've used USB3CV, I see that more changes are needed in that area.

But since you are on this, let me ask about something that prevents USB3CV tests to pass on this board.
EP0 size is 8 for this chip family, when code below is NOT removed tests fail.

      // Only send up to EP0 Packet Size if not addressed
      // This only happens with the very first get device descriptor and EP0 size = 8 or 16.
      if ((CFG_TUD_ENDPOINT0_SIZE < sizeof(tusb_desc_device_t)) && !_usbd_dev.addressed)
      {
        len = CFG_TUD_ENDPOINT0_SIZE;

        // Hack here: we modify the request length to prevent usbd_control response with zlp
        ((tusb_control_request_t*) p_request)->wLength = CFG_TUD_ENDPOINT0_SIZE;
      }

It looks like USB3CV during test queries about device descriptors with request size 8 at the beginning,
then when address is set is asks with request size 18 as expected, but then it resets and sends again
device descriptor requests with size 18 (with no address set up first) which TinyUSB cuts to 8 bytes only,
then USB3CV fails waiting for some more data that never comes.

Have you tried running different board (like STM32F4, that are now configured for EP0 size 64) to have EP0 size 8 to
see if this will also happen?

I'm not familiar with USB spec so I'm not sure if USB3CV is wrong or TinyUSB.
I only used USB3CV not USB2CV which did not see USB controller on PC I used for testing.

@hathach
Copy link
Owner

hathach commented Sep 23, 2021

@kasjer this is really an edge case. This particular code is added since it is needed when testing with actual host driver such linux/windows with 8-byte control endpoint e.g msp430. The host driver won't bother getting the 2nd packet EP0 and will send ZLP as status right after the 1st packet. Maybe the USBCV has different expectation, I will try to test it later on.

You can try to comment it out to run the USBCV, but you may have issue with normal enumeration.

@kasjer
Copy link
Collaborator Author

kasjer commented Sep 24, 2021

@hathach I decided to make dcd_edpt_close_all() look very simple.
This PR will be followed by next change that will take into account what should be done when endpoint is closed so basically I will update dcd_edpt_close() as well.

@kasjer kasjer marked this pull request as draft September 24, 2021 08:38
Unconditionally disables all endpoints except EP0.
@kasjer kasjer marked this pull request as ready for review September 24, 2021 08:47
@kasjer
Copy link
Collaborator Author

kasjer commented Sep 24, 2021

@hathach about USB3CV and Windows/Linux enumeration difference.
For me behavior of USB3CV is more reasonable, but then I also understand what was done in Windows/Linux.
It could be fixed in TinyUSB if we had EP0 OUT endpoint ready not just after expected length of DATA STAGE, but (for example) after first packet or from the beginning. Then STATUS STAGE could just stop control request.

But there is simpler solution that can make USB3CV and Windows happy.
We could slightly modify condition for hacked code:

      if ((CFG_TUD_ENDPOINT0_SIZE < sizeof(tusb_desc_device_t)) && !_usbd_dev.addressed &&
         ((tusb_control_request_t*) p_request)->wLength > sizeof(tusb_desc_device_t)) // <---- New condition
      {
        len = CFG_TUD_ENDPOINT0_SIZE;

        // Hack here: we modify the request length to prevent usbd_control response with zlp
        ((tusb_control_request_t*) p_request)->wLength = CFG_TUD_ENDPOINT0_SIZE;
      }

USB3VC when requests device descriptor in non-addressed state asks for exactly 18 bytes (descriptor size).
Windows/Linux asks for 64 bytes which is far more then needed for valid descriptor size and they are happy
with whatever they get in single packet and send STATUS STAGE packet.

You may consider this new condition that will only affect boards with small EP0 size. I will not insist, but it was quite
annoying to comment this code for tests and restore for normal usage.

I did not checked Mac behavior though.

@hathach
Copy link
Owner

hathach commented Sep 24, 2021

@hathach I decided to make dcd_edpt_close_all() look very simple.
This PR will be followed by next change that will take into account what should be done when endpoint is closed so basically I will update dcd_edpt_close() as well.

The previous edpt_close_all() is actually simpler since it is used when expecting a full reconfiguration. The edpt_close() is currently meant for ISO only, in the future, I plan to retire/remove it to prefer and better API e.g edpt_resize() due to the fact that close/reopen does not work well with some MCUs. But I am ok if you still prefer to keep it this way though.

@kasjer
Copy link
Collaborator Author

kasjer commented Sep 24, 2021

I looked simpler but it was missing stuff that you mentioned in previous comment.
dcd_edpt_close() is still missing that part but I already have code ready for PR that does xfer cleanup there.

@hathach
Copy link
Owner

hathach commented Sep 24, 2021

@kasjer the new condition look great. IMHO, It is really OS host driver fault, getting the 1st 8bytes (wLength = 8) to get EP0 size is common approach similar to getting first 9 bytes of configuration descriptor. I did this as well in the host stack https://github.com/hathach/tinyusb/blob/master/src/host/usbh.c#L712

Back to your suggestion, I think it is great, could you made an PR for it along with the comment to explain the different between the 2 cases

  • Host only want to receive exactly 1 packet with wLength = 64 for EP0 size
  • Deliberately request 18 bytes (full descriptor) length, which it will try to get multiple packets if needed

I will try to check with msp430 and macOS on my side. That is the port that requires this condition to work with. #184

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

I looked simpler but it was missing stuff that you mentioned in previous comment.
dcd_edpt_close() is still missing that part but I already have code ready for PR that does xfer cleanup there.

Look good, thank you for your PRs.

PS: esp32s2 failed due to unrelated issue, I will check it out.

@hathach hathach merged commit cd865f8 into hathach:master Sep 24, 2021
@kasjer kasjer deleted the kasjer/da146xx-close-all branch September 24, 2021 14:08
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