HID SOF interrupt support #2180

Xelus22 · 2023-07-25T13:49:59Z

Describe the PR
Adds HID SOF support.

Additional context
~~Tested and working on CH32V307~~

Questions

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

hathach

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

Xelus22 · 2023-07-26T06:14:30Z

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 · 2023-07-26T13:00:47Z

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 · 2023-08-02T22:17:37Z

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 · 2023-08-03T12:54:44Z

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.

perigoso · 2023-08-08T15:46:40Z

@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 · 2023-08-08T18:07:24Z

@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.

Rocky04

My complains as a review...

Rocky04 · 2023-08-08T18:41:05Z

src/class/hid/hid_device.c

+  (void) rhport;
+  tud_hid_sof_cb(frame_count);
+}
+#endif


A function executed directly in a synchron manner isn't a callback. Meaning don't put a "_cb" in an "_isr" because these names refer to different things.

In this function only code should be placed which needs to be executed inside the interrupt service routine right after when the start of frame occurred.

This is a callback because it's a configurable function defined by the user, not because of how it is called

There are naming conventions for a reason. So people who read a function name have an expectation what the function does. Messing around with that only causes issues. It has nothing to do if it's configurable or not. The word callback itself refers to an inverted direction.

Just have a look at the other TinyUSB callbacks, like tud_mount_cb(). The event for it is invoked (but the function isn't executed) in the ISR. It's later called asynchron in the main loop from tud_task(). But audiod_sof_isr() on the other hand is executed in the ISR. So the "_cb" functions are executed in the main loop and the "_isr" functions within the interrupt service routine and these are two different things.

You cant just make up conventions

https://en.m.wikipedia.org/wiki/Callback_(computer_programming)

It's the way how it used here in this repro. Feel free to prove me wrong by pointing to a TinyUSB standard function referred as callback in its name which isn't called indirectly in the main loop. Would be appreciated...

Don't get me wrong, it's technical a callback in a general sense but not in the context of this repro. Here there is a distinction.

Rocky04 · 2023-08-08T18:42:45Z

src/device/usbd.c

+            driver->sof(event.rhport, event.sof.frame_count);
+          }
+        }
+        break;


Here the callback should be executed but instead the ISR function is executed a second time.

Rocky04 · 2023-08-08T18:50:18Z

src/device/usbd.c

@@ -119,7 +119,7 @@ tu_static usbd_class_driver_t const _usbd_driver[] =
    .open             = hidd_open,
    .control_xfer_cb  = hidd_control_xfer_cb,
    .xfer_cb          = hidd_xfer_cb,
-    .sof              = NULL
+    .sof              = hidd_sof_isr


The assigned ISR function at this place gets called here:

tinyusb/src/device/usbd.c

Line 1132 in cfe8ca3

driver->sof(event->rhport, event->sof.frame_count);

Rocky04 · 2023-08-08T18:58:35Z

src/class/hid/hid_device.h

@@ -118,6 +118,8 @@ TU_ATTR_WEAK bool tud_hid_set_idle_cb(uint8_t instance, uint8_t idle_rate);
 // Note: For composite reports, report[0] is report ID
 TU_ATTR_WEAK void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, uint16_t len);

+// Invoked when received SOF interrupt
+TU_ATTR_WEAK void tud_hid_sof_cb(uint32_t frame_count);


As a callback it should be invoked in the ISR due to the SOF event and called here:

tinyusb/src/device/usbd.c

Line 599 in cfe8ca3

case DCD_EVENT_SOF:

perigoso · 2023-08-08T19:07:20Z

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.

hathach · 2023-08-14T06:36:15Z

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 · 2023-08-14T08:20:47Z

@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 · 2023-08-14T10:01:32Z

@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 · 2023-08-14T11:04:24Z

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 · 2023-08-14T13:41:55Z

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 · 2023-08-14T14:10:01Z

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.

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 #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 · 2023-08-14T14:19:05Z

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 · 2023-08-14T14:54:47Z

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.

hathach

we should only add sof_isr that can be auto managed (on/off when needed) to class driver e.g audio with feedback enpoint on/off along with its audio streaming interface.

Since HID does not need it to function properly (thing breaks with audio if not), we can add an generic sof cb in isr for this purpose for app to turn it on and off when it wants. You don't need to make any changes for this PR. I remembered added that sometime ago (then reverted), I could try to re-add it when possible. Thank you for brining up this PR and issue.

marcpabst · 2024-03-01T22:27:05Z

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 · 2024-05-10T09:07:48Z

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.

Initial HID SOF support

551fe95

Xelus22 changed the title ~~Initial HID SOF support~~ Initial HID SOF interrupt support Jul 25, 2023

Xelus22 changed the title ~~Initial HID SOF interrupt support~~ HID SOF interrupt support Jul 25, 2023

hathach reviewed Jul 26, 2023

View reviewed changes

Update usbd.c

cfe8ca3

This comment was marked as off-topic.

Sign in to view

Rocky04 mentioned this pull request Aug 8, 2023

Adding support for a generic SOF callback #2213

Merged

Rocky04 reviewed Aug 8, 2023

View reviewed changes

This comment was marked as off-topic.

Sign in to view

hathach requested changes Aug 14, 2023

View reviewed changes

HiFiPhile closed this May 10, 2024

HiFiPhile mentioned this pull request May 17, 2024

Some stm32_fsdev optimizations #2178

Draft

HID SOF interrupt support #2180

HID SOF interrupt support #2180

Conversation

Xelus22 commented Jul 25, 2023 • edited Loading

hathach left a comment

Choose a reason for hiding this comment

Xelus22 commented Jul 26, 2023 • edited Loading

perigoso commented Jul 26, 2023 • edited Loading

mitchellcairns commented Aug 2, 2023

Xelus22 commented Aug 3, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

perigoso commented Aug 8, 2023

This comment was marked as off-topic.

Rocky04 commented Aug 8, 2023 • edited Loading

Rocky04 left a comment • edited Loading

Choose a reason for hiding this comment

Rocky04 Aug 8, 2023

Choose a reason for hiding this comment

perigoso Aug 8, 2023

Choose a reason for hiding this comment

Rocky04 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

perigoso Aug 8, 2023

Choose a reason for hiding this comment

Rocky04 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Rocky04 Aug 8, 2023

Choose a reason for hiding this comment

Rocky04 Aug 8, 2023

Choose a reason for hiding this comment

Rocky04 Aug 8, 2023

Choose a reason for hiding this comment

perigoso commented Aug 8, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

hathach commented Aug 14, 2023

Rocky04 commented Aug 14, 2023 • edited Loading

hathach commented Aug 14, 2023

Rocky04 commented Aug 14, 2023 • edited Loading

Rocky04 commented Aug 14, 2023

hathach commented Aug 14, 2023

Rocky04 commented Aug 14, 2023 • edited Loading

hathach commented Aug 14, 2023

hathach left a comment • edited Loading

Choose a reason for hiding this comment

marcpabst commented Mar 1, 2024

HiFiPhile commented May 10, 2024

Xelus22 commented Jul 25, 2023 •

edited

Loading

Xelus22 commented Jul 26, 2023 •

edited

Loading

perigoso commented Jul 26, 2023 •

edited

Loading

Rocky04 commented Aug 8, 2023 •

edited

Loading

Rocky04 left a comment •

edited

Loading

Rocky04 Aug 8, 2023 •

edited

Loading

Rocky04 Aug 8, 2023 •

edited

Loading

Rocky04 commented Aug 14, 2023 •

edited

Loading

Rocky04 commented Aug 14, 2023 •

edited

Loading

Rocky04 commented Aug 14, 2023 •

edited

Loading

hathach left a comment •

edited

Loading