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

5 second blocking delay #2

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

5 second blocking delay #2

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

Comments

@bbx10
Copy link

bbx10 commented Jan 1, 2016

usb.Task() has a 5 second blocking delay in the KeyboardController and
MouseController examples. 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 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);)

A pull request has been submitted with this change.

@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
Copy link

solarPV commented Feb 1, 2016

Thank you for posting.

I tried both options, adding this line: epInfo[index].bmNakPower = USB_NAK_NOWAIT;
and also adding the two lines
epInfo[index].bmNakPower = USB_NAK_NOWAIT;
epInfo[index].bmRcvToggle = epInfo[index].bmSndToggle = 0;

but still having the same problem. I am trying to use it with a barcode Scanner a a touch screen and I have to keep on holding a button down for 5 seconds for the screen to react.

Any ideas? thank you

@solarPV
Copy link

solarPV commented Feb 1, 2016

Sorry, it was actually a problem with my code that was getting stuck in another place. Managed to get it working with no 5s delay. Thank you

@solarPV
Copy link

solarPV commented Feb 2, 2016

Well it doesn't really work correctly. As long as I don't act on the barcode reader there is no delay on my screen. But after the first time that I act on the barcode reader the 5s delay is back and the barcode reader stops working. If I disconnect the USB barcode the delay goes away and when I plug the barcode back it works again.

Any ideas? ty

@solarPV
Copy link

solarPV commented Feb 2, 2016

Ok so I think I found why it happens. In some actions of the touch screen I enter menus in which usb.Task() is not called. It seems that if I rest in those menus for too long, when I come back to the main loop where usb.Task() is, it no longer works. However If I only stay for a short time insided the menus when I come back it works normally. I have no idea why this happens. But maybe you guys with more info might now.

ty

@Lauszus
Copy link
Contributor

Lauszus commented Feb 2, 2016

You need to call Usb.Task() frequently. Are you using a delay anywhere in your code? If so please see the following guides: https://www.arduino.cc/en/Tutorial/BlinkWithoutDelay and https://learn.adafruit.com/multi-tasking-the-arduino-part-1/using-millis-for-timing.

@solarPV
Copy link

solarPV commented Feb 2, 2016

I was just comming to re-post. Yes I finally solving by calling usb.Task() inside each of the sub-menus which are inside a while until the exit button is pressed. Because I don't actually want a barcode to be scanned while inside these sub-menus I just added an if statement to not do anything when usb.Task() is called.

It now seems to work perfectly. ty

@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

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. You should be able to add my fork as a secondary remote and merge it easily enough. Or if you prefer, I could create a pull request for your fork too, since I originally forked your fork to get the control-key changes. (why isn't a pull request for a fork called a spoon, so that one can spoon it back on top of the original? lol)

@bbx10
Copy link
Author

bbx10 commented Feb 15, 2016

LOL, spoon! I will give the change a try. No need to do anything extra.

@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
Copy link

Actually, I do have a couple of Zero Pro boards here, but haven't done anything with them yet, as I've been working with so many different processors they are starting to come out my ears -- mostly been busy working on the STM32 micros lately.

The new F7 (Cortex M7) is one smoking processor and their STM32F746G Discovery board is a steal at $50 (with built-on 4.3" capacitive touch screen LCD, 8MB SDRAM in addition to 340KB SRAM and 1MB Flash, FS/HS USB, Ethernet, SD interface, etc) at 216 MHz with ART acceleration, DSP instructions, etc, and with Arduino Uno form-factor connectors too for easy interfacing with shields, but I digress.

The Due was actually a better fit for one of my (many) current projects and it needed a keyboard, which is how I got sidetracked on this library... But sure, I'll grab the Zero's source and make the same changes there. I usually don't like making commits until after I've thoroughly tested it, but it looks enough like the Due code that I think I can manage it. I'm sure I'll eventually need it for my Zero Pro boards.

I may even get inspired to go ahead and set things up and test it on my Zero boards. Though I will have to get my build scripts tweaked to add the SAMD target -- I use CMake and QtCreator, etc, on Linux and don't use the Arduino IDE. So far, I've only targeted the AVR and SAM with my CMake scripts. I've stubbed in the SAMD, but haven't tried it yet -- maybe this is a good excuse for me to do that and finish getting it working...

Give me a few days to finish up the I/O configuration and setup I'm currently working on for the Nucleo32-F303K8 board (a good alternative to the Nano) and I'll take a look at the Zero code's USB.

@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

Ok, I will be switching gears to Zero so I will give it look today.

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

BTW, I looked at the STM32F746G Discovery board specs and I am very tempted. I do not need another distraction but it is too good to ignore. I had no idea there are Cortex M series processors running over 200 MHz.

@vavabiker
Copy link

I have a problem with my scanner barcode....it could be a polling problem?
The keyboard works fine!!

`Device descriptor:
Descriptor Length: 12
Descriptor type: 01
USB version: 0200
Device class: 00
Device Subclass: 00
Device Protocol: 00
Max.packet size: 40
Vendor ID: 040B
Product ID: 6543
Revision ID: 0100
Mfg.string index: 00
Prod.string index: 00
Serial number index: 00
Number of conf.: 01

Configuration descriptor:
Total length: 0049
Num.intf: 02
Conf.value: 01
Conf.string: 00
Attr.: A0
Max.pwr: FA

Interface descriptor:
Intf.number: 00
Alt.: 00
Endpoints: 02
Intf. Class: 03
Intf. Subclass: 01
Intf. Protocol: 01
Intf.string: 00
Unknown descriptor:
Length: 09
Type: 21
Contents: 100100012224000705

Endpoint descriptor:
Endpoint address: 81
Attr.: 03
Max.pkt size: 0040
Polling interval: 01

Endpoint descriptor:
Endpoint address: 01
Attr.: 03
Max.pkt size: 0040
Polling interval: 01

Interface descriptor:
Intf.number: 01
Alt.: 00
Endpoints: 02
Intf. Class: 03
Intf. Subclass: 01
Intf. Protocol: 01
Intf.string: 00
Unknown descriptor:
Length: 09
Type: 21
Contents: 100100012241000705

Endpoint descriptor:
Endpoint address: 82
Attr.: 03
Max.pkt size: 0040
Polling interval: 01

Endpoint descriptor:
Endpoint address: 02
Attr.: 03
Max.pkt size: 0040
Polling interval: 01`

Thanks in advance..

@bbx10
Copy link
Author

bbx10 commented Mar 19, 2016

Max.pwr: FA

This means the barcode reader wants 500 mA of power. Try plugging in power using the barrel connector. My bar code reader works when the Due is powered via barrel connector but fails when powered via USB.

@vavabiker
Copy link

Hi, thanks for your reply.
I tried your solution but unfortunately my barcode scanner doesn't work.
I noticed that when I try the scanner with my PC, it sounds a single "beep", while when
I plugged into my arduino and try to read the barcode, it sounds TWO "beep".
Do you have any ideas?

Thanks a lot!!

@peppegti
Copy link

peppegti commented Feb 9, 2018

Hi, I want read from usb (acm terminal) a gps, but this work only I open the serial monitor. It's possible to work without open the serial monitor??

@tinkerBOY-git
Copy link

Is this fixed already? I'm using an Arduino Zero and there's still obvious delay.

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 a pull request may close this issue.

7 participants