I looked briefly at your pull request. You've made choices which require a massive number of changes throughout all the files.
Thanks for taking a look at the code changes.
Yes, the changes are extensive. However, I tried multiple entirely different approaches, and I really don't think it's possible to do this in any other reasonable way. Also I think these changes will improve the ability to change and improve the library in the future, by making the library more generic, and therefore more flexible and modifiable. The use of global memory allocations in the trunk version that are not tied to any specific USBHost instance is really not ideal.
Let me explain why the changes are so extensive though. There is a domino effect of required changes due to the topology of dependencies between the hardware registers, code, and memory allocations. Starting from just the very first requirement of using different registers and constants for each host port, every other step pretty much inevitably follows.
(1) There are 30 different register addresses and constants that are different for USB1 and USB2, e.g.
USBHS_USBCMD needs to expand to either
USB1_USBCMD or
USB2_USBCMD depending on which host port is targeted. This means that these macros need a new host port number parameter, and each
USBHost instance needs to store an integer
host_port, so that the correct registers and constants can be used. (In general programs may instantiate multiple
USBHost instances, one for each host port.)
(2) However,
USBHost in your git repo contains only static methods, and no instance variables. Adding an instance variable like
uint8_t host_port requires making some of these methods non-static so that they can write to hardware registers. Then there's a long and complex process of figuring out the entire call tree of which currently-static methods call these non-static methods, since only non-static methods can call non-static methods. This ends up turning the vast majority of methods in
USBHost into non-static methods.
(3) Once most of
USBHost is no longer static, it becomes obvious that the total number of global memory preallocations will be insufficient if the number of host ports increases. But more importantly, the memory ownership has to be looked at closely: for example, mixing transfers or pipes from multiple different host ports into a single linked list could cause all sorts of issues. So really every host port needs its own copy of the global memory allocations. Initially I tried making an array of multiple versions of these allocations, one per host port, then I indexed into these allocations using the
USBHost::host_port field. But this was much uglier, and I realized the real issue here was that the topology of memory allocations needed to reflect the physical topology of the hardware. Therefore, I created the
USBHostPort class, which moved all the global memory allocations into a new object instance, one per host port. The USBHost class now contains
USBHostPort* usb_host_port, and
uint8_t host_port was moved into
USBHostPort, since the port number is a property of the port. All other host state variables were moved into
USBHostPort too, e.g. the enumeration state.
(4) Tons of the methods in
USBHost now contained code that repeatedly looked up
usb_host_port->something, just so that the correct registers could be written and/or the correct memory allocations could be used. It became obvious that a lot of these methods exclusively acted on memory allocations and state variables in
USBHostPort -- which makes conceptual sense,
because they were doing work specific to a host port instance, and not something general to all active USB hosts. This made it obvious that a lot of the USBHost methods should actually be moved to USBHostPort, starting at the deepest call stack frames that wrote to hardware registers, and moving up from there.
(5) This all leads inevitably to the deepest and probably the trickiest change. Two methods are used as callbacks:
isr() is attached as an interrupt vector, and
enumeration() is added as a callback function that is called when a transfer is complete. However both of these need to access host-port-specific registers, which means that they need access to the host port number. However pointers to these functions have to be stateless -- you cannot use instance methods of a specific
USBHostPort instance as a function pointer, due to C++ limitations. In other words, there need to be N different function pointers for each of the
isr() and
enumeration() functions, given N available host ports. This is what I am using C++ capture-less lambdas for, which you spotted in
imxrt_usbhs.h. Each lambda expands to a piece of code that looks up the
isr() or
enumeration() instance method for a specific
USBHostPort instance. That instance method now has access to its own host port number, so the correct host-port-specific registers and constants can be used.
(6) The only real change that I made that was arguably optional was that at the end of all the above refactoring,
USBHost didn't contain many methods anymore, except for a large number of static
println_ and
printf_ methods etc. -- I moved these into a new class,
PrintDebug, just to make things cleaner. This change doesn't have much impact, ultimately, because every source file that used them had
#define println USBHost:: println_ etc. at the top, this could just be changed to
#define println PrintDebug:: println_. This change could be in a separate pull request, but I had already refactored out so much of
USBHost that it made sense to move these last static methods that were unrelated to
USBHost into their own class.
The very first step is to look for whether you're getting "Async Followup" printed.
Code:
if (stat & USBHS_USBSTS_UAI) { // completed qTD(s) from the async schedule
[B][COLOR="#B22222"]println("Async Followup");[/COLOR][/B]
//print(async_followup_first, async_followup_last);
Transfer_t *p = async_followup_first;
while (p) {
if (followup_Transfer(p)) {
Sorry, I should have explained this better -- when I said in the PR that
"the isr() callback is never called for async-scheduled transfers, i.e. where (stat & USBHS_USBSTS_UAI) is nonzero in the isr() function", I was trying to say that this "Async Followup" code is never called. I then mentioned
"However, isr() is still called for other non-async transfer types", which is referring to the fact that other blocks in this same method, such as
(stat & USBHS_USBSTS_PCI) for "port change detected", are in fact called. This is why this is so mysterious: the
isr() callback does in fact work, but only for non-async interrupts.
If the interrupt is happening, add more prints inside that while loop so you can see if it's actually finding the expired qTDs.
I see you've already uncommented printing inside followup_Transfer(). If that too is printing, so the interrupt is happening and the expired qTDs are found, then add more printing here:
followup_Transfer() is not being called for asynchronous transfers, because
isr() is never called when
(stat & USBHS_USBSTS_UAI) is nonzero. I expect if the isr() call was called when it was supposed to be, all the other followup mechanisms would work fine.
I see you changed the callback function pointer to a C++ lamba scheme. So if you're getting all the way to "// this transfer caused an interrupt", then maybe all you need to do is consider whether testing the pointer for non-NULL works with your changes to the callback, and whether this code is the proper way to call the completion function.
Yes, I looked at that too, but that code is never even called, for the above reason.
Of course if followup_Transfer() isn't getting called but you do get the async interrupt, then dig into the code which maintains the list of transfers.
I looked over the code that maintains the list of transfers many times, but I didn't change that code, as far as I can tell. The only thing I can think I may have missed is some assumption about the way you were doing memory allocations, which broke something due to changing reuse of an object to non-reuse, or vice versa, once I moved the global memory allocations to instance variables of
USBHostPort.
I ran into one issue like this already in
driver_ready_for_device(USBDriver *driver). Depending on how I called the library, it was possible for drivers to try to add themselves twice to the list of available drivers, which meant that the same
Driver_t allocation could be added twice to
available_drivers. In that case the driver's
next field would point back to the driver itself -- therefore anything traversing this list would run into an infinite loop. Making the following changes fixed the infinite loop:
Code:
void USBHostPort::driver_ready_for_device(USBDriver *driver)
{
driver->device = NULL;
driver->next = NULL;
if (available_drivers == NULL) {
available_drivers = driver;
} else if (available_drivers != driver) {
// append to end of list
USBDriver *last = available_drivers;
while (last->next) {
if (last == driver) {
// Driver already in list (avoid infinite loop if added twice)
return;
}
last = last->next;
}
last->next = driver;
}
}
However for the issue of async callbacks not working, I haven't found any other infinite loops that may be getting triggered -- the transfer is completely scheduled, then the scheduling code exits.
Do you have any other ideas?