UsbClass::poll not always called on UsbDevice::poll

[TimNN]

Contrary to what is documented on UsbClass::poll ("Called whenever the UsbDevice is polled."), it is only called by UsbDevice::poll when UsbBus::poll returns PollResult::Data.

Is that working as intended with the documentation being wrong or does the documentation need to be updated?

[TimNN]

Maybe to provide a bit of context: I've been working USB Serial Logger implementation. The logs are initially written to an internal buffer. Whenever endpoint_in_complete is called, the buffer is checked and if logs are present they are written.

The issue is, of course, when there are no logs to be written. In that case endpoint_in_complete will never be called again. Based on the documentation, I implemented poll so that it would periodically also check the buffer and trigger an initial write if necessary. I few hours of debugging later, I noticed that my poll method wasn't actually called 😑.

[Disasm]

Slightly offtopic: have you tried usbd-serial for this task?

[TimNN]

The Logger is actually a wrapper around usbd-serial.

[Disasm]

Then it should start sending when you write to it.

[TimNN]

Then it should start sending when you write to it.

It does. The issue is that the initial write is never triggered because it happens during UsbClass::poll.

[Disasm]

@mvirkkunen, any thoughts? I think that the current behavior is correct and the documentation is wrong, but who knows.

[mvirkkunen]

Hmm.. good question! Calling it only when data is received is somewhat redundant because the specific methods (control_in, endpoint_out) etc will also have been called at that point if the class needs to know about data.

However the correct way to implement this would be to proactively write into the endpoint outside of poll, even if UsbClass::poll was called on every poll. The reason is that it's OK for users to only call UsbDevice::poll in a USB interrupt handler, which may not be executed at all if there is no traffic. usbd-serial writes a packet immediately after the first buffer is written into the port this and while this may result in the first write of a batch being inefficient (possibly only one byte), other implementations would require either help from the user (a guaranteed periodic call to a method) or some kind of generic timer system which is perhaps out of scope.

Honestly I don't remember what UsbClass::poll was meant to be used for. It might be a candidate for removal now to avoid confusion.

[TimNN]

Honestly I don't remember what UsbClass::poll was meant to be used for. It might be a candidate for removal now to avoid confusion.

I'd be ~fine with that, I was mostly annoyed at the confusion / misleading information.

However the correct way to implement this would be to proactively write into the endpoint outside of poll, even if UsbClass::poll was called on every poll. The reason is that it's OK for users to only call UsbDevice::poll in a USB interrupt handler, which may not be executed at all if there is no traffic.

Yes, my solution here was to simply trigger the interrupt manually, if necessary.

---

The reason for triggering the initial write during the interrupt is simplified ownership of the UsbClass implementation (it's only available during the interrupt).

The reason for having the logic execute automatically during UsbClass::poll is so that the user of the UsbClass implementation only needs to call one poll method instead of two.

[mvirkkunen]

That sounds like a feasible use case. I wonder if it'd be best for UsbClass::poll to be called absolutely every time, or are there some cases when it shouldn't. For instance in the "bus reset" case the handling is likely very different and fn reset() should handle that.

[TimNN]

Yeah, I'm not sure about the best design here either. Maybe one option would be to keep UsbClass::poll and really call it always, with two changes:

1. Document that this is a low-level method that normally does not need to be / shouldn't be used.
2. Add at least the PollResult and maybe also the current UsbDeviceState as an argument.

[mvirkkunen]

In the next version (in the endpoint-trait branch), UsbClass::poll has been changed so that it is called on every poll while the device is connected, and it gets an "event" object that has the device state as well as methods for checking which endpoints have has events occur (the data from PollResult, and it now replaces endpoint_out/endpoint_in_complete)

[ianrrees]

I just ran in to this same issue, the endpoint-trait approach sounds like a good solution!