HID SOF interrupt support

[Xelus22]

Describe the PR Adds HID SOF support.

Additional context ~~Tested and working on CH32V307~~

Questions

1. Unsure if it should be called hidd_sof_isr or hidd_sof_cb? (used isr since the audio one uses that too)
2. Should I pass in uint8_t rhport into hidd_sof_isr?

[Xelus22]

Hi hathach, ive added in the SOF callback to the HID driver specifically for fast mouse response and minimum latency.

I've put it here since I would like to use tinyUSB to follow the current fastest open-source mouse firmware on the market in terms of click latency (after razer)

https://github.com/zaunkoenig-firmware/m2k-firmware/blob/main/mouse/Src/main.c

you can see here they wait for a SOF interrupt before starting the routine and getting data from the sensor/buttons then send it out. Since there is only 125us frame time between packets at 8khz polling rate.

Edit: Apparently I didnt do the handling of the SOF packet in usbd.c silly me. Need to do that ...

[perigoso]

I don't see the reason to add sof callback to hid driver ? Could you explain why this is needed. @hathach

I'll chime in, as I've worked on high performance firmware for input devices before. I do see usefulness in a SOF interrupt, we can use it to assemble the HID packets at the very last second to minimize latency (Yes it is a very small latency gain, but the latency is there nonetheless), that said the vast majority of use cases will not need this so I think it might make sense to gate it behind a configuration option.

Honestly a SOF interrupt could be useful as a configurable option for all drivers, but that's beyond the scope of this PR.

[mitchellcairns]

This would be very useful. Working on a gamepad at the moment and this would help a ton with reducing latency at lower interval inputs.

[Xelus22]

This would be very useful. Working on a gamepad at the moment and this would help a ton with reducing latency at lower interval inputs.

I would assume the GP2040-CE firmware would also benefit as well. Pretty sure the GP2040-CE is also one of the fastest on the market already since it uses the 2 cores of the RP2040 which is cool.

[Rocky04]

• There should be only necessary things placed in an interrupt. Everything else should be done in the main loop.
• USB Data response should be done in an asynchronous manner.
• The USB driver only guarantee bandwidth (for isochronous transfers) and uniform timings with a specific speed depending accuracy (for interrupt transfers).

Meaning a HID device can be polled at any time as long as the rate is constant while it's unclear when that actually happens between two SOFs packages. So the SOF interrupt should be only used to save the time when it happened and the callback should be invoked later on from TinyUSB in the main loop handler if needed. Then realtime stuff can be done by checking when the SOF occur and how much time has passed since then. For HID devices the SOF can be used to synchronize the polling to the scanning and so to minimize derivation and so making the delay more consistent. But it would not necessarily reduce the latency. For example the interface can be polled right after the SOF and so there is likely not enough time to read the sensor and process it's data. Then the data would be always one frame behind. And you don't want that the polling maybe gets skipped because the scanning hasn't finished.

For reducing the latency a real time approach would be needed. Like to figure out when the specific interface is polled and to read the sensor perfectly timed right before. Of course for that the time for the scanning and it's processing must be known and be consistent.

Edit: So because two clocks are never the same it may works to figure out when an interface is polled compared to the SOF and then to use the time of the last SOF package to calculate when the next sensor read should take place to avoid that the clock difference causes shifts over time.

Edit 2: I agree that a general and driver independent SOF callback would be useful, but as suggested for any time critical stuff the time when that actually happened compared to when the callback is executed needs to be known too. There is also the question how precise and consistent the polling actually is due to jitter caused by the timing inaccuracy. So likely that at least every now and then the time between the polling and the SOF needs to be checked again.

Edit 3: On top of that the host is able to poll the device more frequently than the defined polling rate of the endpoint. If a device has a polling rate of 10 ms it can be polled by the host each 8 ms (or even something between 1 and 10 ms after the last polling). Meaning as long as the polling rate isn't maxed out there is no guarantee of a precise rate of delivery. While this causes derivation it can also have a negative impact on the delay if the scanning is scheduled on a kind of slow static rate.

[Xelus22]

I understand what you're saying, but for that to happen a SOF callback here I think would exactly help facilitate that. For the user to implement this RTOS-esque functionality if required and not be TinyUSBs responsibility.

Unless you mean this should be extended to all device types.

In addition, the USB IN interrupt is sent directly after the SOF packet.

https://microchipdeveloper.com/usb:transfer#interrupt-transfers

[Rocky04]

As far as I know it's not guaranteed that interrupt in transfers for non-control endpoints are always scheduled directly after the SOF, even if that is typically done this way. I'm not aware that the USB specification specifies when within a frame interrupts transfers must take places as long as the timing is ok.

Imagine a lot of devices with a lot of interrupt interfaces. The more there are the longer it takes to handle all and the last one can be scheduled very late compared to the first one. So depending when the interface is scheduled compared to others it will have a big impact how much time has passed since the last SOF.

Even your linked info didn't state that interrupt packages are always scheduled after the SOF. It's not even mentioned with a single word. They are just using an example picture where this is the case and that is all.

Edit: A report for a HID interface can be big, like 256 byte, so multiple transactions are needed. It can only be taken for granted that one transactions happens each frame, while this is typically done in a row where the entire report is send altogether. I barely remember that such transactions can cause a huge derivation when other interfaces are polled.

Also it can happen that no interfaces has something to send one frame while on the next frame each may has. This would also may cause a huge delay which gets bigger the later an endpoint would be scheduled for polling. A host driver may prevent this by spacing the polls a bit apart.

[Rocky04]

• I think a general SOF callback can be helpful for any application. It's likely needed for any realtime application. As the other callbacks they are handled by the tud_task() in the main loop. So that can't be used as a time reference for any time critical stuff because you don't know the time between when the SOF interrupt was triggered by the MCU and when this callback is actually executed. Of course for non-critical stuff it should be fine and better as nothing.
• I don't see why this should be limited to a specific device type or driver at all. It can be implemented the same way as the other optional callbacks. To add the event in the query and then let tud_task() call it the main loop independent from the device driver if the callback is present.
• I'm somehow confused about the class driver stuff of TinyUSB. I would expect that if there is a driver SOF callback for an audio device then there should be one too for the video, but this isn't the case. I also don't get why the device driver callback for the SOF is tied to the device type and not to the isochronous transfer type regardless what data is send. Like if a double buffer is used to determine when the buffer to store the new data and for sending the old data should be swapped. I don't see why the SOF would be useful to a particular driver and so be driver specific.

[perigoso]

@Rocky04 I don't understand where you are trying to get, please be concise, and avoid massive information dumps and rambling like you are doing, it's hard to parse to parse without the context of what you're thinking, also try to reach a conclusion, ideally something actionable, right now you're just wasting everyone's time.

I think you're suggesting that the SOF interrupts not reliable, and you're right, that's a given some architectures don't even implement it... but I fail to realize why that matters here, perhaps we can't use to assemble a packet just in time, you can still use it as synchronizing signal, even if like you are mentioning it's not guaranteed to be within a defined interval, either way all of that is out of the scope of the PR.

What is being suggested here is to simply expose the SOF interrupt to "user-space", it is implementation defined what is to be done with it, it is not something the class driver or other core functionality would rely on.

You also seem to be confusing the USB specification domain with the implementations, this is about the SOF interrupt, which is not part of the USB specification, and is completely tied to the USB core architecture (the implementation).

No offense, but I'm going to mark your comments as off topic

[Rocky04]

@perigoso Here, as simple as I can summaries it, just for you...

• A general SOF callback should be added. It makes absolutely no sense to implement that in a HID specific way.
• It would help making the delay more consistent but likely add latency. Please check out how Motion Sync works on a modern mouse.
• Your statement is contrary. This PR is all about adding a HID specific callback and not about a general one.
• I'm aware of the USB specification, how a typically implementation for a USB device works and also about latency on input devices. So no, I'm not confused but rather aware of the potential time specific impacts.

No offence, but unlike you I prefer to keep it to myself what I think when I read your post.

[Rocky04]

@Xelus22 The function hidd_sof_isr() as configured in the class driver is called in dcd_event_handler() and so executed when the SOF interrupt is handled. That's why the name is "isr". Due to this there would not elapse much time between the ISR and the function execution. But that approach isn't good because only very important stuff should be done in an ISR and as less as possible execution time should be spend in them in general.

The added code from you in the tud_task_ext() function on the other side handles the callbacks which are executed in the main loop by tud_task(). Because of this change the SOF handling is actually done two times, basically immediately and some time later.

I created an example PR #2213 which implements a general SOF callback independent from the class device driver.

Edit: In the case you want a HID specific callback which is executed within the ISR you only need to set the SOF function pointer in the corresponding class driver.

[perigoso]

Here, as simple as I can summaries it, just for you...

This is better, you should make extra effort from the start.

A general SOF callback should be added. It makes absolutely no sense to implement that in a HID specific way.

I agree, I made the same suggestion.

It would help making the delay more consistent but likely add latency. Please check out how Motion Sync works on a modern mouse.

This is precisely what I noted as being an implementation detail.

Your statement is contrary. This PR is all about adding a HID specific callback and not about a general one.

It's not contrary, I made no mention of "HID specific" or "general callback", just that this is about exposing the SOF, and you seemed to be talking about firmware implementation details.

I'm aware of the USB specification, how a typically implementation for a USB device works and also about latency on input devices. So no, I'm not confused but rather aware of the potential time specific impacts.

Ok.

No offence, but unlike you I prefer to keep it to myself what I think when I read your post.

This is not twitter, I did not give my thoughts, I asked you to explain yourself better because I could make nothing of it.

[Rocky04]

@perigoso

Upf, details matter... An implementation is always about the details. This is a PR, so side effects and specifics need to be taken into account. The underlying goal for the desired feature of this PR was all about input delays on USB. So with the provided info I was basically questioning the purpose of the PR as is, even I referred to a use case which would be implementation specific.

I can understand that there is likely a language barrier and that maybe not all things are easy to understand, but this is also not a discussion about a potential simplified approach for a concept. So no, I don't see any reason why to keep it as simple as possible. I'm also very technical for a reason, to either point out a potential misconception from others or that I can be corrected in the case I'm wrong or rely on false assumptions.

In the first comment on this PR hathach was questioning why there is a need of a HID specific SOF callback in the driver part of the code. You mentioned that this would be useful to all drivers but that the scope of this PR is just for the HID driver. So no, to me your suggestion in your second post was contrary to the first one.

Strange, according to your wording I thought you handled it like Twitter (or X).

[perigoso]

You're the one repling like you're on twitter, this isn't personal

[HiFiPhile]

Please calm down friends, it's just a hobbits project we are not NASA :)

It happens sometime we have different working style / understanding of a problem, not means others is wrong.

[hathach]

late response as usual, seems like there is a bit of heated dicussion in this issue, I hope we are still cool and don't take that personal.

Hi hathach, ive added in the SOF callback to the HID driver specifically for fast mouse response and minimum latency. I've put it here since I would like to use tinyUSB to follow the current fastest open-source mouse firmware on the market in terms of click latency (after razer) https://github.com/zaunkoenig-firmware/m2k-firmware/blob/main/mouse/Src/main.c you can see here they wait for a SOF interrupt before starting the routine and getting data from the sensor/buttons then send it out. Since there is only 125us frame time between packets at 8khz polling rate.

Thank you for explanation, my take on this is we should use generic SOF. I did actually add API for generic SOF previously when doing audio sof for feedback. Though I back out on generic API since there is no usage back then https://github.com/hathach/tinyusb/pull/1381/files#diff-290e049d139d4c4ba09224b43d2f53d2ea705c59ba979c3881143be80d5402deR91

Maybe it is good time to re-add it.

[Rocky04]

@hathach What's missing and should be re-added? The linked PR is merged. Do you mean the function to dynamically set the SOF ISR function?

If that is supposed to be controlled by the individual implementation why is there one already present just for audio?

[hathach]

@hathach What's missing and should be re-added? The linked PR is merged. Do you mean the function to dynamically set the SOF ISR function?

If that is supposed to be controlled by the individual implementation why is there one already present just for audio?

I just removed that in one of follow-up PR along with other managing code, forgot which one. Yeah, we can re-add that generic code to enable SOF

void tud_sof_isr_set(tud_sof_isr_t sof_isr);

Though, maybe we should continue to use the weak, named callback. I forgot why I went with dynamic function pointer last time (which is not consistent with other driver callback). I will re-evalate this later.

Sum up we could add SOF callback, just not under the name of HID since HID isn't required SOF to function properly like audio feedback.

[Rocky04]

I would prefer to have both, a more general SOF callback which is weak and executed by tud_task() and so within the main loop and one separated from this in the ISR for driver specific things. For the first there is already a PR #2213.

Edit: As already mentioned the weak functions so far with the prefix "_cb" seems to be only used for the callbacks which are executed by tud_task(). The functions which are executed directly within the ISR seems to have the suffix "_isr". So for me there is a clear difference between them and what's why a function which is executed in the ISR should not be named as a callback and so be weak. This is also the reason why I would prefer to have the SOF function pointer for the driver within the ISR named back to "sof_isr" so it's clear where this is executed.

So I think the SOF ISR function of the driver should be set dynamically. So for this only that function need to be re-added.

[Rocky04]

The issue right now with setting the function pointer for a driver specific SOF callback (here used in a common sense), which is executed within the ISR, is that this would need to know to which driver it should be applied in the case multiple configurations are used. The driver name is only present on a debug level so that can't be used.

Because of this I think it would be better to let it as is so that the SOF ISR is purely driver related. Meaning if that behavior should be changed or controlled that should only be possible by using a vendor defined driver. For that the SOF ISR should be opened up just for that. Here is a draft for this #2220.

[hathach]

I would prefer to have both, a more general SOF callback which is weak and executed by tud_task() and so within the main loop and one separated from this in the ISR for driver specific things. For the first there is already a PR https://github.com/hathach/tinyusb/pull/2213.

I used to think of deferred SOF, but from last work with audio feedback. I am not quite sure if we still need that. what is point of SOF where is not invoked in the start of frame, other transferred is probably completed by the time callback is invoked in thread context. Until we really need it, we won't add it.

Because of this I think it would be better to let it as is so that the SOF ISR is purely driver related. Meaning if that behavior should be changed or controlled that should only be possible by using a vendor defined driver. For that the SOF ISR should be opened up just for that. Here is a draft for this https://github.com/hathach/tinyusb/pull/2220.

sof_isr in usbd driver is only available for driver that can be managed by usbd, e.g audio with feedback enpoint is enabled, sof is enabled, and disable along with the interface. I don't see the point to add sof_isr for vendor. Vendor manage SOF in application and should use the generic tud_sof_enable()

[Rocky04]

I'm not really sure what you mean...

For me there are two places, executed a bit later in the main loop via a general callback and directly as fast as possible when the MCU handles the SOF interrupt via a specific driver callback for specific things. I don't get why you mentioning tud_sof_enable() because controlling if the MCU should react to a SOF package or not and is independent from what the application should do when this occur and can be handled. Which of these two should used just depends on the implementation, like what's executed there and how time critical it is.

I also think a vendor defined driver should have full control and so I don't see any reason why the SOF ISR should not be exposed for CFG_TUD_VENDOR. For example, what is if someone wants to create his own audio driver? Right now the framework code would need to be modified for this. I think it would be better to just set CFG_TUD_VENDOR and implementing the vendor code by using the code from CFG_TUD_AUDIO as a base. Like the vendor code can map to the audio code and may only differ for the SOF ISR.

Edit: For example I can imagine an implementation which uses both, the SOF ISR to store the exact time when the MCU handles it while performing the function via SOF CB in the main loop and then take into account how much time has passed between the ISR occurrence and the execution.

[hathach]

not all thing we thought might be needed will be added though. Only thing that we are sure people will use. I don't see the point to calculate start of SOF ISR with SOF_CB. When you need SOF_ISR, you need it because you have to do something on-time, otherwise something else will break.

[marcpabst]

Hi, I'm really sorry to revive this issue but I tought it might be a good place to figure out what the current status on a generic SOF callback is? Aplogies if I missed anyhting, but I couldn't relaly find any information on this.

[HiFiPhile]

Thanks for everyone's effort.

Generic user land SOF support has been merged in #2213.

One can use bool tud_sof_cb_enable(bool en) and void tud_sof_cb(uint32_t frame_count) to use SOF callback.