USBHost_t36 USB Mass Storage Driver Experiments

yeahtuna said:
I'll try it out today, but I feel like it's using duct tape to fix a problem. Obviously commands need to begin in user code, but subsequent calls to queue_Data_Transfer should probably be done inside the callbacks so that they are initiated in the isr().

With the fix to pipeIn, my non-blocking USBDrive initialization is finally working. I must confess, however, that the pipeIn fix has made initialization about 500% faster, so the non-blocking initialization is not so important.

@wwatson
Are your drives initializing any faster, too?
Click to expand...

Note: most of the cases that this would hopefully fix are those that are initiated from the user code. Like: please write 4096 bytes to the file... Things like Reads are mostly handled by first doing the queue out of the command, and then all data that comes back is under the control of the ISR. However I understand that some of MassStorageDriver the read requeue is done after the data is processed... Some of this code could be reorganized, to process the data on a callback, and state machine... But I believe a lot of this code is modelled after other code bases. Where for example if you wish to read 4096, you simply do the request and when the data is there your code continues, which fits pretty well in systems with some for of OS of threads or...

wwatson said:
@yeahtuna - I just got home from work, I might be called back in tonight:( I am really glad this has solved your issues:) I will try it out tonight if I can. I also have some retirement things to take care of first. Will be glad when I finally retire and have more time to help...

Edit: And learn:)
Click to expand...
Again assuming what you said is you are about to retire. Congrats. I survived that for several years now!
 
KurtE said:
Note: most of the cases that this would hopefully fix are those that are initiated from the user code. Like: please write 4096 bytes to the file... Things like Reads are mostly handled by first doing the queue out of the command, and then all data that comes back is under the control of the ISR. However I understand that some of MassStorageDriver the read requeue is done after the data is processed... Some of this code could be reorganized, to process the data on a callback, and state machine... But I believe a lot of this code is modelled after other code bases. Where for example if you wish to read 4096, you simply do the request and when the data is there your code continues, which fits pretty well in systems with some for of OS of threads or...
Click to expand...

Ya, I agree that user code should have to wait while the data is read / written. Things would get pretty messy otherwise. In my non-blocking USBDrive init, I do the initialization as a state machine inside of Task() and then call startFileSystem() after the initialization is complete, which has the same issues as doing everything in user code. If I find more energy to work on it again, I'll see about moving more of the queuing to the callbacks.

BTW, I added the code to disable the IRQ in queue_Data_Transfer() and it seems to be fine. Everything is snappy as it should be. If any issues crop up, I'll let you know.
 
KurtE said:
Note: I just pushed up a new fork/branch: https://github.com/KurtE/USBHost_t36/tree/queue_transfer_protected
Where I updated:
Code: 
bool USBHost::queue_Data_Transfer(Pipe_t *pipe, void *buffer, uint32_t len, USBDriver *driver)
{
	Transfer_t *transfer, *data, *next;
	uint8_t *p = (uint8_t *)buffer;
	uint32_t count;
	bool last = false;
	
	// We always want to do this while the interrupt is disabled. 
	// But only re-enable if it was enabled coming in. 
	[COLOR="#FF0000"]bool irq_was_enabled = NVIC_IS_ENABLED(IRQ_USBHS);
	NVIC_DISABLE_IRQ(IRQ_USBHS);[/COLOR]

	// TODO: option for zero length packet?  Maybe in Pipe_t fields?

	//println("new_Data_Transfer");
	// allocate qTDs
	transfer = allocate_Transfer();
	if (!transfer) {
	[COLOR="#FF0000"]	if (irq_was_enabled) NVIC_ENABLE_IRQ(IRQ_USBHS);[/COLOR]
		return false;
	}
	data = transfer;
	for (count=((len-1) >> 14); count; count--) {
		next = allocate_Transfer();
		if (!next) {
			// free already-allocated qTDs
			while (1) {
				next = (Transfer_t *)transfer->qtd.next;
				free_Transfer(transfer);
				if (transfer == data) break;
				transfer = next;
			}
			[COLOR="#FF0000"]if (irq_was_enabled) NVIC_ENABLE_IRQ(IRQ_USBHS);
            return false;[/COLOR]
        }
		data->qtd.next = (uint32_t)next;
		data = next;
	}
	// last qTD needs info for followup
	data->qtd.next = 1;
	data->pipe = pipe;
	data->buffer = buffer;
	data->length = len;
	data->setup.word1 = 0;
	data->setup.word2 = 0;
	data->driver = driver;
	// initialize all qTDs
	data = transfer;
	while (1) {
		uint32_t count = len;
		if (count > 16384) {
			count = 16384;
		} else {
			last = true;
		}
		init_qTD(data, p, count, pipe->direction, 0, last);
		if (last) break;
		p += count;
		len -= count;
		data = (Transfer_t *)(data->qtd.next);
	}
	bool return_value = queue_Transfer(pipe, transfer);
	[COLOR="#FF0000"]if (irq_was_enabled) NVIC_ENABLE_IRQ(IRQ_USBHS);
	return return_value;[/COLOR]
}
The idea is that this code will always run with the IRQ_USBHS disabled. But it only should re-enable the IRQ if the IRQ was enabled when this method
was called.

If anyone gets a chance to try it out, it would be great. Obviously too late for B4, but,,,
Click to expand...

I tried something similar but used a class - disable/store original interrupt state in the constructor, restore in the destructor. Saves having to catch all the exit points of the function.
I also tried just reference counting the number of entries/exits into these "important" code sections with the ISR resetting the count to zero, and never once saw evidence that the ISR was interrupting them and causing corrupted lists. But still being unsure that it couldn't happen, I started dropping the core speed... and found that at 24MHz, USB mass storage writes simply don't work. The CBW goes out, the sector/data packet goes out, but the interrupt to indicate it was sent never arrives. I haven't figured out why but suspect there might be an issue with the way new transfers overwrite the pipe's halt qTD with the new transfer information.

I really think a lot of this code could benefit from refactoring. I know there's been a lot of work done by many people on various drivers, but it's a mess - host controller code is tangled up with drivers, the filesystem handling is tangled up with storage, bluetooth is tied to HID, etc. If it was redone so all host controller management is handled by a separate thread, the priority could be set so the USBHS IRQ couldn't interrupt it. Or at the very least, the list handling could be made to use exclusive load/stores to detect contention (although this wouldn't work for double-linked lists like the followups). I would really like to try adding support for isochronous transfers (microphone/camera in support) but the current code just has too many existing issues.
 
Also, I tried plugging in a drive that uses 4KB sectors - instant reboot the first time it tries to read a sector. Seems the SdFat code is hardcoded to use 512byte sectors so it causes a buffer overflow, probably trashes the stack and causes a jump to address 0.
 
I managed to make it so that commands are initiated in user code (the first call queue_Data_Transfer), but then all the subsequent calls related to the command are handled in the callbacks. Seems to work nicely. I'll clean up my code and share in case anyone wants to try it out.
 
As far as I can tell, readSectorsCallback() is still forcing the user to wait. It would probably be more useful it was a non-blocking operation that triggered the callback when the read was complete.
 
jmarsh said:
Is it just me or are there major race conditions in ehci.cpp? Lots of list manipulation happening without interrupts disabled and isr() being triggered can easily modify those same lists...

As a basic example, look at USBDriverTimer::Start(). Imagine there's already one active timer, so our timer will be added after it. Instruction flow reaches line 512 and the timer interrupt triggers for the already active timer; when the interrupt handler returns, our timer gets added after the now retired timer and will never be triggered.
Click to expand...

I'm quite certain that the issue you highlight here is responsible for a problem I'm seeing where my T3.6 device will hang when sending MIDI to multiple devices connected to its host port.
https://forum.pjrc.com/threads/73129-USBHost-send-MIDI-Issue

Do you have a fix in mind for USBDriverTimer::Start()?

Here's a link to the code on GitHub
https://github.com/PaulStoffregen/U...a1c6bfe91758776e77fa1a00bf05c20/ehci.cpp#L518
 
You must log in or register to reply here.
Back
Top