make USB HID period configurable (and default is now 8ms)

[TheRealMoeder]

No description provided.

[somewhatlurker]

Fun. Still gotta fix test stubs then, and I have no clue what to do for 7e and f7.
I guess I'll leave the latter to someone who knows what the hell they're doing. There's no rush because after all this isn't even tested yet anyway. (previous discussion in #981)

[TheRealMoeder]

@goebish can you help out with the linker finding everything in modular build? I tried, but something seems messed up with my build environment :(

[goebish]

I can take a look but I'll be back home in 10 days

[TheRealMoeder]

I think I found the issue. Can't simply make usb_endpoint_descriptor hid_endpoint non-const

Solution:
1.) add EXTERN(HID_SetInterval) to protocol/exports.ld
2.) remove static from struct usb_endpoint_descriptor hid_endpoint

While at it, we should also remove extern void HID_Write(s8 *packet, u8 size); from usbhid.c and instead add the declaration to target.h and also add HID_Write to exports.ld (not sure if this is really necessary)

[somewhatlurker]

seems to be linking properly; that's good

[somewhatlurker]

so all this needs is testing that it works as expected then

[TheRealMoeder]

I'll try to do so this weekend.

[TheRealMoeder]

Couldn't resist. Works perfect down to 1 ms in non modular build (t8sg as my testing device for development here). I have no modular devo right now, I might try building a modular build for my t8sg for testing purposes.

edit: while trying out the modular build I bricked the (unified) bootloader on my t8sg, so I'll have to dig out the ST-Link to continue testing. Might take couple of days.

[TheRealMoeder]

I noticed some interesting behavior in lower update rates. For example, with 50 ms, the average ist indeed 50 ms, but it actually alternates between 35 and 65 ms. But since nobody wants these slow update rates it's probably not worth investigating. From 1 to 10 ms i noticed very exact update rates.

[somewhatlurker]

I'm not really sure about how USB endpoints work, but I think that's related to how the OS polls the device.

For example, Windows seems to take the provided bInterval and round it down to a closest power of two (or always 32 if larger than 32).
That does sound like the jitter you're seeing, and should be do different to the original behaviour which used a 32ms bInterval.

It should be possible to hide that by always requesting a lower polling rate or only letting the user choose power-of-two periods somehow, but I don't know if it's worth adapting to "features" of USB stacks that don't really harm operation.

[eried]

but it actually alternates between 35 and 65 ms

How are you measuring this?

From 1 to 10 ms i noticed very exact update rates.

True, and this is similar to an 8bitdo controller I tested as a example to compare the "laggy" usb, so we are good with 8ms :D

[TheRealMoeder]

I measured using Wireshark on MacOS, but honestly only exemplary on settings 1-10 and 50 ms. But I'm not deep into USB stuff, just wanted a proof of the changes working. Also tried these settings in Picasim simulator though wine on the same system, which shows the same behavior as the Wireshark results suggested. Since 50 ms isn't usable at all, it really is not worth investing there. Maybe we should limit the protocol option to 32.

[somewhatlurker]

Yeah, I'd agree that limiting it at 32 or even 16 is probably more sane than 64. (at least for a new project)
It really just boils down to whether we care about keeping the old timing accessible even though it wasn't good.

[somewhatlurker]

Hmm... I realised I didn't make mixers run more often than the default (5ms?) when the HID period is decreased. That might be a good idea.
Probably not strictly necessary, but I should've had the foresight to copy that stuff.

[TheRealMoeder]

I took another T8SG lying around (actually a Radiomaster TX8, but they are all the same to me ;) and flashed it.

edit: now got Wireshark working correctly. As you suspected, update rates are always the closest power of 2, so when in-between, it changes between the closest powers of 2 to match the specified usb period in average. I don't thinks this is really a good behavior, as the protocol option suggest more control than it actually has. If we limit it to 16 the maximum error is 7 ms. Or maybe 6 ms if we limit the protocol options to steps of 2 (2-16). What do you think?

edit2: modular build still not working yet...

[TheRealMoeder]

Finally managed a working modular build for the T8SG. Works very well with the same behavior as non modular builds. Let's decide on the period setting and we're good for merging.

[TheRealMoeder]

Concerning the protocol option:
Using steps of 2 isn't really good, as it still doesn't do what we expect it to do. Maybe the hackish
static const char * const usbhid_opts[] = { _tr_noop("Period (ms)"), "32", "16", "2", "4", "8", NULL, NULL }; gives the best user experience defaulting on 8 ms.

[somewhatlurker]

I think it's best to keep everything in a sequential order here, such as "16", "8", "4", "2", "1", or even changing it to Hz for ascending too ("62.5", "125", "250", "500", "1000").

I think the first option will become the true default when all is complete, right? (I don't see any way to set an alternate default without breaking the first listed option unless we write a callback in the protocol)

[TheRealMoeder]

The issue is that the last entry is the default...

[somewhatlurker]

It doesn't seem to behave that way on other protocols in emulator.
For example, the default radio in testrf is CYRF6936 for me.

I suspect you accidentally loaded a model that already had config data or left in the code that sets it to 8 when it was previously unset.

[TheRealMoeder]

Reading up on USB... Actually the period depends on the operating speed of the USB device and host. On full and high speed we can have 1,2,4,8,16,32 but for low speed it is only 8, 16 or 32. So setting period in protocol interval doesn't necessarily reflect the true transfer interval of the hid.

[somewhatlurker]

I believe all targets with HID support are running at full speed, and it would be extremely unlikely for someone to use a host device without full speed support.

[somewhatlurker]

let's see if this actually improves anything...
might actually reduce battery life a little if it works as expected, but it'll be fine.. USB HID should be using less power than most protocols by not running any RF anyway. (that's if it's even a measurable difference lol)

[TheRealMoeder]

While this might make a little improvement, I'm a little concerned in yet another rom size increase in the modular platforms (especially devo7e). These are really tight on rom space, and the addition of HID_setinterval already adds a bit. It might even be useful to make thr default option of 8 ms hard-coded for the modular builds and add the protocol option, which needs HID_SetInterval, only in non modular builds

[somewhatlurker]

Hmm.. I see...
I really have no good sense for what's significant or not at these sizes, but I'll see if I can find some way to get it back down before removing functionality.

[TheRealMoeder]

You last commit didn't add too much, it's just something we (unfortunately) always need to have in mind.

[somewhatlurker]

Yeah, I get what you mean. It's easy to add things that use up space now, but if we always do that it becomes hard to get the space later for something that really needs it.

[TheRealMoeder]

We put a lot of effort into code cleanup, because ROM space for devo7e was absolutely used. Since then, things have gotten a little relaxed (and deviation development more or less subsided due to not many active developers). But as you notice in target_defs, a lot of features are already deactivated for the modular builds.

[somewhatlurker]

Looks like devo 8 is getting pretty close to fully used too, and t8sg not far behind... I suppose the only way to avoid the increase on non-modular is to simply not add code though right?

[somewhatlurker]

Anyway, the increase seems pretty minimal for 7e now, so if that was the primary concern and the code is still working, I guess we're good.

[somewhatlurker]

Hmm... I think I actually figured out a nice enough way to give us a callback without needing to run mixers in the actual USB endpoint callback -- the USB endpoint callback can start a clock to run the protocol callback in an interrupt later.
That should be simple to implement, and hopefully be reliable.

[somewhatlurker]

I'll play around tomorrow and see if I can get it to compile.

[somewhatlurker]

Hopefully I didn't just break everything lol

[somewhatlurker]

Perhaps this PR could use a revised title due to the now larger scope of changes, and possibly incorporate an increase to 8 button inputs with it being more than a simple change now anyway.
Or should it stay limited to just timing-related changes and be followed up by a smaller PR for more channels later?

[TheRealMoeder]

I'd prefer seperate Pull requests for timing and inputs.

[TheRealMoeder]

99fb44b broke USBHID for modular as well as non-modular builds, while 5c0a6bd works for both.

[somewhatlurker]

Does the callback method work with 1 instead of 0 in the CLOCK_StartTimer call on devo_hid.c line 146?

[TheRealMoeder]

Nope.

[somewhatlurker]

Hmm... I'd like to see the callback method working, but I can't see what I'm doing wrong.
Should I just revert to what works for now?

[TheRealMoeder]

Yes, I also expected it to work, but haven‘t spent time debugging. Since none of the the protocols currently runs mixers on demand and the benefit is small and should only noticeable with 500 Hz and above, I‘m fine with the previous solution. The more changes to the „main“ code, the higher the change for introducing issues.

[somewhatlurker]

Running mixers on demand was already present since before 5c0a6bd. It was copied from sbus/sumd like a lot of other stuff. The callback from USB code was just to try guaranteeing perfect sync to USB host polling.

I will go ahead and revert the two most recent commits for the sake of having a working system though.

[TheRealMoeder]

Yes I know, I didn't mean running mixers on demand but synchronizing callbacks. Mixer run time is about 1 ms worst case, I'm not too worried about this. #532 is some interesting read.

[TheRealMoeder]

I can't approve as I created this PR, but I'm good with this.