Re: USB: Question about usb_write() API


Paul Sokolovsky
 

On Tue, 30 Jan 2018 14:36:40 +0000
"Cufi, Carles" <Carles.Cufi@nordicsemi.no> wrote:

[]

I'd suggest to look at this as a generic problem, not specific to
USB subsystem (e.g., there's the same issue in networking
subsystem, etc.), and consider the known best practices dealing
with it. For example, POSIX, when dealing with streams-of-bytes,
[]

Although you raise a good point, I'm not convinced that this applies
here. I think that what Sundar wants to know is whether this
particular API call, usb_write(), was originally designed to be able
to buffer data and fragment it into smaller USB-sized packets or
instead it should mirror the state of whatever hardware FIFO it is
using below, returning a number of bytes written different from the
ones the caller asked to send. Beyond that there's the issue of the
API call being blocking or not, which seems unclear given the
different ways it is used in Zephyr. To sum up, I thin he's trying to
clarify the original intentions of the API designer in order to adapt
the driver he's working on to it, not looking to modify the API
itself.
But clarification of the API is also what I'm talking about. Other
points are also collinear, for example, POSIX supports concept of short
reads/writes exactly to let drivers avoid blocking (or avoid extensive
resource usage in general).

usb_write() is described as:

* @brief write data to the specified endpoint
*
* Function to write data to the specified endpoint. The supplied
* usb_ep_callback will be called when transmission is done.
*
* @param[in] ep Endpoint address corresponding to the one listed in the
* device configuration table
* @param[in] data Pointer to data to write
* @param[in] data_len Length of data requested to write. This may be zero for
* a zero length status packet.
* @param[out] bytes_ret Bytes written to the EP FIFO. This value may be NULL if
* the application expects all bytes to be written

That's not explicit enough. The best reading I can get of it is that it
supports *both* of the scenarios above: a) if bytes_ret is non-NULL, it
may perform short write; b) if it's non-NULL, short write is not
allowed. But in all fairness, there's a bit of "reading between the
lines" in that. And such specification puts additional burden on a
driver: it must support both not-looping/not-blocking and
looping-blocking operation. Besides problems with testing these both
modes, we'd find that some drivers simply omit implementation of one or
another case (I see that in Zephyr all the time).

A good clarification thus would be:

1. bytes_ret is always not-NULL.
2. There can always be short writes, number of bytes written is
returned in *bytes_ret, caller must retry with remaining data as needed.

Note that this doesn't call for updates to existing drivers - they can
keep accepting bytes_ret as NULL, and loop/block inside. But clients
of this API definitely need to be updated, because otherwise they
simply won't work with some drivers.


Thanks,

Carles


--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog

Join devel@lists.zephyrproject.org to automatically receive all group messages.