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

da146xx bus reset and sleep reworked #1101

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

kasjer
Copy link
Collaborator

@kasjer kasjer commented Sep 23, 2021

Describe the PR
So far bus reset was handled (with some holes in it).
Sleep and remote wake-up were not really tested and did not work.

With this change:

  • bus reset is reworked
  • remote wakeup works
  • bus signal sleep is handled
  • in some cases register modification (read/update/write) was change to just writes.
  • FRAME interrupt is used for bus reset end condition and for delay needed in remote wakeup condition.

Additional context
While it work with Windows and Linux some change in usbd.c is needed
for USB3CV tests to pass due to EP0 size 8.
Following lines prevent tests but are good for normal usage:

      // 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;
      }

This was tested with USB3CV tool Chapter 9 Tests [USB 2 devices] and HID Tests

Not much of an improvement but handle only interrupts
that were enabled and are expected.
So far bus reset was handled (with some holes in it).
Sleep and remote wakeup were not really tested and did not work.

With this change:
- bus reset is reworked
- remote wakeup works
- bus signal sleep is handled
@hathach
Copy link
Owner

hathach commented Sep 24, 2021

I am in the middle of esp32s2 work. Give me a bit of time to pull out the da146xx usb to test and review this.

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.

Sorry for late response, have been busy with other works. To be honest, I still haven't pull out my DA146xx USB board to test with, since I had an issue with flashing its bin previously. Anyway, the changes look good, it mostly to fine tune the bus event handling with precise reset, suspend & resume detection. Therefore I don't think I could do anything better than yours.

PS: If you tested with USCV and has its result, please let me know so that we could update DA146xx status in #1059

@hathach hathach merged commit 9da234c into hathach:master Oct 13, 2021
@kasjer
Copy link
Collaborator Author

kasjer commented Oct 13, 2021

I will re-run test. I assume USCV has some log that can be send to you (or to #1059) or it's just screen shot?
If it is clear what needs to be sent in #1059 (which I have not read to carefully) just ignore this comment I will follow what others did.

@hathach
Copy link
Owner

hathach commented Oct 13, 2021

@kasjer it output an html result in its working directory (click on view result will bring you there), you could upload it here for reference

@kasjer
Copy link
Collaborator Author

kasjer commented Oct 13, 2021

Here are test results from running USCV, Chapter 9, HID and MSC. Let me know it MSC failure is due to driver not working as expected or cdc_msc example limitation.
USB3CV.zip

@hathach
Copy link
Owner

hathach commented Oct 13, 2021

@kasjer I just re-run the MSC test on nrf52840, it seems to be fine. From the log, device seems to stop response to the Test Unit Ready after BOT MSC Reset. Sometimes a random failure can occur, could you

  1. Re-run the suite to see if still continue to fail
  2. Switch USBCV "Select Test Mode" to Debug and run the only failed test to see if it still fail when running alone.

I am not entirely sure what is wrong, sometime it can be caused by Data toggle mismatched when unstall. Also make sure when endpoint is stalled, the current queued transfer should be aborted as well. If you don't have time, don't worry, we will just note this, and I could help to troubleshoot this later on.

@kasjer
Copy link
Collaborator Author

kasjer commented Oct 13, 2021

Thanks for confirmation that application is fine and should work without problems.
I tried to run it on LPC55S69 (that I'm not familiar at all) and pyocd filed so I gave up.
I do have access to nrf52840 so I will compare behavior if needed and find the problem soon.

@hathach
Copy link
Owner

hathach commented Oct 13, 2021

@kasjer no problem at all, I am glad that I could help. Btw, for chapter9 test suite, it is best to test with hid_composite example, since that has remote wakeup attribute set, and the example will attempt to wake system up when suspended while cdc_msc example does not https://github.com/hathach/tinyusb/blob/master/examples/device/hid_composite/src/main.c#L215 . There are 2 tests for this: one host enabled the remote wakeup, another it disables to test if device respect that.

@kasjer
Copy link
Collaborator Author

kasjer commented Oct 13, 2021

Ups, I could have added Chapter 9 test on HID instead of MSC, that did ask for wake up button activation. It work on pro-dk and usb-dk.
I will attach this file once I have MSC tests fully working.

@hathach
Copy link
Owner

hathach commented Oct 13, 2021

@kasjer no need to attach it for proof, your words is enough. Just want to mention since it can be easily missed :) . I have updated your DA146xx result to #1059 as reminder. Later on if you (or me) manage to figure it out, feel free to post it here or there (whichever is more convenient to you).

@kasjer kasjer deleted the kasjer/da146xx-bus-reset-sleep branch October 15, 2021 12:01
@kasjer kasjer mentioned this pull request Oct 15, 2021
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