MSC Read on slow data source

[hathach]

Discussed in #2034

^{Originally posted by Jubatian April 19, 2023}
Hi,

Would like to ask about the behaviour of tud_msc_read10_cb() in a scenario which seems to not work like I was expecting.

I have a relatively slow data source (an SPI Flash chip).

The existing architecture (with a different USB library I am migrating from), when a read request arrives from USB, calls a callback to notify the Data Source module that it needs to prepare "lba" sector.

The Data Source module later called from the main loop manages the read working through a state machine in a couple of main loop iterations until the data is ready. When so, it calls the USB library, which then begins sending the response.

Just to emphasize: There is no RTOS or interrupts in play, it is all sequential program flow.

Reading tud_msc_read10_cb(), it appeared like it should be easy, return 0 to it until the data is ready. However what I get is that seemingly it never returns! (The main loop freezes up for several seconds, with no return data observable by Wireshark).

Digging into TinyUSB's code I have a suspicion that it might not work the way I anticipated here (tud_task() doesn't return until tud_msc_read10_cb() is served a nonzero return, so there is no chance for other things called from the main loop to do their jobs, including preparing the data to be sent back).

Mainly would like to ask, whether I see this right? (To avoid spending more time on finding out why it doesn't work if this is it)

If it indeed works this way, any possible suggestions would be nice of course (maybe a way to tell data not yet ready to the host with an SCSI sense code? Not sure).

[hathach]

yeah, I think you are right about this. The way the msc driver is doing is queuing its self back using the same usbd event queue. Since tud_task() trying to process all the event before return. This cause an loop() without giving back the cpu. I think we could fix this by adding custom field for tud_task() to skip it until next time or so.

PS: I made an issue based on this discussion, since it is indeed an bug. Let move discussion there, thanks.

Originally posted by @hathach in #2034 (comment)

[Jubatian]

Wow, thank you for acting so quickly on this!

Wasn't anticipating so, glad it seems like the way I am trying to use it should be working if this can be fixed! :)

[Jubatian]

Just started to look at this more in-depth to understand what is happening.

Do I see the followings right?

• The queue is read here: https://github.com/hathach/tinyusb/blob/master/src/device/usbd.c#L487
• In case of having an RTOS, the while() loop here is meant to practically never exit, relying on osal_queue_receive() yielding whenever the queue becomes empty for optimal scheduling.
• In case of no RTOS, an empty queue results in osal_queue_receive() returning false, thus exiting, passing control back to the main loop.

If these are correct, it would seem to me that the situation isn't optimal even with an RTOS, it works, but ends up requiring the RTOS to preempt to make progress, tud_task() keeping spinning the CPU due to the MSC repeatedly putting the event on the back of the queue. And of course fails as observed with no RTOS.

Wouldn't it be something which rather the MSC driver shouldn't do?

Though if it doesn't do it, it would seem breaking the current interface, as due to the tud_msc_read10_cb() callback's description ( https://github.com/hathach/tinyusb/blob/master/src/class/msc/msc_device.h#L64 ), TinyUSB should keep polling for the data if read == 0, while there is no function to signal that the data is ready (which I feel would be the ideal even with RTOS, to avoid spinning the CPU relying on preemption).

EDIT: Just an idea studying the code. Might it be possible to add a "Yield" event? The MSC could then send it before the zero length transfer complete it uses to get tud_task() looping back to it. When tud_task() pops off a "Yield" from the queue, it yields (No RTOS: Returns; RTOS: Uses the appropriate method to yield if the RTOS has such, otherwise no effect).

[Jubatian]

Maybe a better, but possibly more elaborate idea for a probable solution:

• Add another special return, for example defined to INT32_MAX, for the possible returns of tud_msc_read10_cb(). This return would indicate that the availability of data will be signalled (while 0 would retain its current behaviour for compatibility). In proc_read10_cmd(), this special value would result in doing nothing (as compared to sending the zero length dcd_event_xfer_complete() for the 0 return).
• Add a signalling function (no parameters related to data, just a signal), which sends off the zero length dcd_event_xfer_complete() event to trigger a tud_msc_read10_cb() call next time tud_task runs.

In the case of an RTOS, this would seem the more appropriate solution as well (if people transition to this usage), as the availability of the data to return can trigger TinyUSB, it no longer polling for it.

Just ideas of course, I am not familiar enough with the codebase to see potential pitfalls as of now, sharing just in case one or another might look like a reasonable solution.

[hathach]

thank you for your idea. I am currently still busy with other works, will definitely taking your note into account the next time I work on this.

[abakosh]

Is this issue also present in the host module, I am assuming from a quick look it is there as well.

[Jubatian]

Didn't check the host, though I think it might not be a problem there as the host is driving the communication. On the device side, the problem manifests due to the host asking for data (without prior notice to prepare it), which needs to be served (which may take time to prepare, requiring the work of other modules).

I applied a patch locally, following my second suggestion of adding an interface to signal data readiness. It took just a few dozens of lines in the MSC device class (header and source), and seems to be doing just fine.

[Jubatian]

Will try to describe the patch here which worked for me, as was asked from me, and might be useful for others:

src/class/msc/msc_device.c and .h are involved.

In the header, the followings are added:

#define TUD_MSC_SIGNAL_DATA INT32_MAX
This is for tud_msc_read10_cb(), an additional return value indicating the application is not ready yet, for example disk I/O busy. The application would call tud_msc_data_ready() when the data becomes available (Should add this to the API description of tud_msc_read10_cb() as well to keep things nice).

void tud_msc_data_ready()
This is called by the application when the data is ready to be served.

In the .c source, the followings are added:

To the msc_interface_t structure:
uint8_t waitrhport;
bool waitdata;

The new tud_msc_data_ready() function:

void tud_msc_data_ready(void)
{
// Simulate a transfer complete so the tud_msc_read10_cb() callback will be fired again
if (_mscd_itf.waitdata)
{
_mscd_itf.waitdata = false;
dcd_event_xfer_complete(_mscd_itf.waitrhport, _mscd_itf.ep_in, 0, XFER_RESULT_SUCCESS, false);
}
}

And into proc_read10_cmd() to handle the TUD_MSC_SIGNAL_DATA case:

else if ( nbytes == TUD_MSC_SIGNAL_DATA )
{
// The application will signal when the data is ready by tud_msc_data_ready()
p_msc->waitrhport = rhport;
p_msc->waitdata = true;
}

For usage, you need to return TUD_MSC_SIGNAL_DATA in the tud_msc_read10_cb() callback if the data is not immediately available, then you will need to use the tud_msc_data_ready() function in the application (whatever is providing the data to the MSC) to signal once the data is ready.

[HiFiPhile]

You could try #2967

[602p]

We would really like if something along these lines could be landed

[HiFiPhile]

We would really like if something along these lines could be landed
Fixed by #2967