Skip to content

USBHost::Task blocks for 5 seconds #2

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

Open
bbx10 opened this issue Jan 1, 2016 · 21 comments · May be fixed by #6
Open

USBHost::Task blocks for 5 seconds #2

bbx10 opened this issue Jan 1, 2016 · 21 comments · May be fixed by #6
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@bbx10
Copy link

bbx10 commented Jan 1, 2016

Calling usb.Task() results in a 5 second blocking delay. This makes it hard to use USB host with
other devices that need regular polling. For example, if data is
streaming in to Serial1 at 115200 bits/s, the incoming data will not
be processed for up to 5 seconds.

This can be seen by adding timing output to the KeyboardController and MouseController examples. For example, the following prints "delta millis 5000" when there is no keyboard or
mouse input:

void loop() {
  uint32_t delta, startMillis = millis();

  // Process USB tasks
  usb.Task();

  delta = millis() - startMillis;
  if (delta > 10) {
    Serial.print("delta millis ");
    Serial.println(delta);
  }
}

The problem is the input endpoint property defaults to wait until timeout which is 5 seconds. The proposed solution is to set the EpInfo.bmNakPower to USB_NAK_NOWAIT. This means if a single NAK is received, input operations return with an error instead of waiting for 5 seconds.

With this change usb.Task() returns in <= 10 ms when there is no input from the device.

hidboot.h
@@ -464,6 +464,7 @@ void HIDBoot<BOOT_PROTOCOL>::EndpointXtract(uint32_t conf, uint32_t iface, uint3
        epInfo[index].deviceEpNum       = (pep->bEndpointAddress & 0x0F);
        epInfo[index].maxPktSize    = (uint8_t)pep->wMaxPacketSize;
        epInfo[index].epAttribs     = 0;
+       epInfo[index].bmNakPower    = USB_NAK_NOWAIT;

        TRACE_USBHOST(printf("HIDBoot::EndpointXtract : Found new endpoint\r\n");)
        TRACE_USBHOST(printf("HIDBoot::EndpointXtract : deviceEpNum: %lu\r\n", epInfo[index].deviceEpNum);)

Additional context

A pull request has been submitted with this change: #6

Additional reports

@bbx10
Copy link
Author

bbx10 commented Jan 20, 2016

@cmaglie @Lauszus

This issue is related to the following issue felis/USB_Host_Shield_2.0#184.

union {
  uint8_t epAttribs;
  struct {
    uint8_t bmSndToggle : 1; // Send toggle, when zero bmSNDTOG0, bmSNDTOG1 otherwise
    uint8_t bmRcvToggle : 1; // Send toggle, when zero bmRCVTOG0, bmRCVTOG1 otherwise
    uint8_t bmNakPower : 6; // Binary order for NAK_LIMIT value
  } __attribute__((packed));
};
epInfo[index].epAttribs = 0;

is equivalent to

epInfo[index].bmNakPower = epInfo[index].bmRcvToggle = epInfo[index].bmSndToggle = 0;

The patch restores bmNakPower to USB_NAK_NOWAIT so Usb.Task => inTransfer =>dispatchPkt do not hang waiting for mouse or keyboard input.

If you prefer I can remove epInfo[index].epAttribs = 0 and set each field as needed.

epInfo[index].bmNakPower = USB_NAK_NOWAIT;
epInfo[index].bmRcvToggle = epInfo[index].bmSndToggle = 0;

@Lauszus
Copy link
Contributor

Lauszus commented Jan 20, 2016

Yes I believe it would be better never to set epAttribs, as it is prone to errors if you set things in the wrong order. Please see the following PR: felis/USB_Host_Shield_2.0#185.

@bbx10 bbx10 linked a pull request Jan 20, 2016 that will close this issue
@solarPV

This comment has been minimized.

@solarPV

This comment has been minimized.

@solarPV

This comment has been minimized.

@solarPV

This comment has been minimized.

@Lauszus

This comment has been minimized.

@solarPV

This comment has been minimized.

@dewhisna
Copy link

I've encountered a similar problem to what you are experiencing. In my case, I have to leave calling the Usb.Task() function to go into a special bootloader mode to transfer some EEPROM data. During that time, keyboard input and console serial I/O is suspended.

The problem, I think, is how the Usb.Task() function handles the "delay" calculation with millis(). If that runs too long, it has to loop all the way around before it starts working again, which in my case causes the keyboard to not respond at all until that extra timeout for it to finish wrapping around and millis() to catch back up (a delay similar to what you are experiencing).

I can see three possible solutions -- 1) Modify Task() to handle wrapping of the delay and millis() calculations. This would be the cleanest solution, and I think it would also fix another problem that I believe exists, which is when millis() itself rolls over and the 'delay' value is large and millis() is small because it just rolled over. In other words, Task() should calculate the delta time since the last call and not compare against the absolute value of millis() itself. 2) Add a delay logic reset function to the class that the client can call after it has to enter a period of suspending USB I/O. This would be a bit clunky, but is better than nothing. 3) Make Usb.Task() callable from a timer interrupt rather than having to be polled.

I originally tried to put the calling of the Usb.Task() function inside an interrupt driven timer tick so that it could happen without being part of the main loop and still be called during the other processing periods when polling isn't possible -- i.e. make it interrupt driven instead of polled. But I quickly found that the Task() function doesn't like being called from within an interrupt handler, as there seems to be a problem with nested interrupts and it receiving data from the USB channel while inside the timer tick interrupt. If you turn on the trace and watch it, that's where it seems to stop and just hangs, and I couldn't find an easy fix for it without completely reworking Task().

So, in my opinion, the best overall set of fixes would be to first fix the delta vs. absolute time issue in Task() itself as described in solution option 1, and then later work on redoing Task() to allow it to be interrupt driven as described in solution option 3 to eliminate it from having to be polled by the main loop. I'm currently doing the polling in my stdio getchar() handler which is called from the libc read() function, and it would be nice to decouple the USB logic from stdio completely. That would be the best solution.

I'm going to look into at least implementing solution option 1 and I will try to follow this post up with a fix for that. I think that needs to be done regardless of making Task() work as interrupt driven instead of polled. Otherwise, I think there's still going to be problems where millis() itself happens to roll over, even when calling Task() is done sufficiently often.

@bbx10
Copy link
Author

bbx10 commented Feb 13, 2016

The USB Host Shield library has a next-generation UHS30 project which is interrupt driven. I do not know much about it though. The plan is to have one library for host shield hardware as well as for native USB (Due and Zero). But as far as I know there is no native USB support yet. If you decide to try interrupt driven, it might be useful to look at UHS30.

@dewhisna
Copy link

dewhisna commented Feb 14, 2016

Thanks for the suggestion on the UHS30 code. I'll have to find some time to look at that. In the meantime, I've implemented solution option 1 to fix the time wrapping issues in the Task() function and solve the problem I was experiencing. I've already submitted a pull-request to the libraries team, but it's at dewhisna/USBHost@ff3d990 (the usb_task_delta_time branch) if you'd like to pick it up too.

@bbx10

This comment has been minimized.

@bbx10
Copy link
Author

bbx10 commented Feb 16, 2016

I tried your fix with all pending PRs and it works fine. Delay processing makes more sense now.

The Arduino Zero USBHost library appears to need the same fix. If you do not have a Zero, I can test the fix if you submit a PR. The Zero USBHost library is built-in to the Zero board package. The USBHost library in this repo does not work on Zero.

https://github.com/arduino/ArduinoCore-samd/blob/master/libraries/USBHost/src/Usb.cpp

@dewhisna

This comment has been minimized.

@dewhisna
Copy link

I went ahead and updated the Usb.cpp file for the Arduino Zero to match my changes to the Arduino Due. I haven't yet tested it, since it's going to take me a bit to get all of my CMake scripts setup for the SAMD target (particularly I have to add all of the logic for openocd).

I forked the ArduinoCore-samd repo and put my changes on a similarly named "usb_task_delta_time" branch, so it's ready for you to go ahead and run the tests on it if you have your Zero board ready to run. I haven't created a pull-request for it yet. That way, I don't have to rework the pull-request if you happen to find any issues: dewhisna/ArduinoCore-samd@f4d9526.

Let me know what you find, and I'll either create the pull request or try to fix it accordingly.

@bbx10
Copy link
Author

bbx10 commented Feb 23, 2016

I ran my USB keyboard programs and all work fine using your branch. The code looks good (same as on Due).

@vavabiker

This comment has been minimized.

@bbx10

This comment has been minimized.

@vavabiker

This comment has been minimized.

@peppegti

This comment has been minimized.

@tinkerBOY-git

This comment has been minimized.

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Mar 20, 2025
@per1234 per1234 changed the title 5 second blocking delay USBHost::Task blocks for 5 seconds Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants