Implemented rate limiter for HID reports. #39

ChrisVeigl · 2020-07-17T15:13:11Z

Added new config variables for KConfig:
-) enabling rate limiter
-) set interval in microseconds
-) Either accumulate mouse movements or discard reports when sent too fast

Added new config variables for KConfig: -) enabling rate limiter -) set interval in microseconds -) Either accumulate mouse movements or discard reports when sent too fast

ChrisVeigl · 2020-07-17T15:16:05Z

main/ble_hidd_demo_main.c

+						/// discard any packets. If we get them too fast, many of them are still sent with the current values.
+						#if CONFIG_MODULE_USERATELIMITER
+							//check interval, jst to be sure use the absolute value of the subtraction.
+							if(abs(esp_timer_get_time() - cmdBuffer->lastTimeHIDCommandSent) > timeIntervalHIDCommand)


is it guaranteed that the last (accumulated) data or button actions get sent if the rate limiter is active?
or can it happen that the last accumulated values are only sent when the next burst of mouse commands come in ?

That's a good question. My intention was, that even if the rate limiter is active, most (but not all) commands will be sent.
I guess it is very unlikely, that e.g., the button release report is sent as last one of too much (only in this case it would trigger the rate limiter).

If we would implement any async mechanism for sending the last movement values and mouse buttons, it would increase the complexity quite much:

New timer (I suggest a FreeRTOS timer)

Callback structure

Detecting if the timer should be started (how could we know in this command processor that this would be the last command?)

If we really need the proper transmission of every last movement value, I could implement it.
Nevertheless, I think if some external device hits the rate limiter, the chance of dropped values is quite high.

An easier approach:
If the rate limiter hits, just check if the button values have changed?

I've added a button state tracker, which sends the button values if they change, even if the report is received out of the interval.

i think that the rate limiter will always cause an unwanted behaviour if we do not guarantee that the most recent accumulated data is sent as soon as possible. for example: if movement data stays "in the pipeline" it will influence the next burst of mouse activities which come in, which might cause unintended deflection of the mouse cursor.
You could throw away acculumeted data if there was a pause, but IMO the only "clean" method to implement this is to trigger a timeout (a bit longer than the selected update rate) for a callback function where we check if accumulated coordinates are in the pipeline an send them if necessary.

Good point, I will implement the accumulation in a different way:

In uart_parse_command I will just accumulate the mouse values

A timer (endless loop) will check the accumulated values on a regular timebase (-> use the rate limiter value) and
send any accumulated values.

do you think this is working :-)?

sounds good :-)
in case the accumulator is empty, the report could be sent immediately in order to avoid unnecessary delay/jitter.

ChrisVeigl · 2020-07-17T15:17:08Z

main/ble_hidd_demo_main.c

+									if(cmdBuffer->accumValues[i] < -6000) cmdBuffer->accumValues[i] = -6000;
+								}
+							#else /* CONFIG_MODULE_RATELIMITERACCUMULATE */
+								ESP_LOGI(EXT_UART_TAG,"Rate limit: discard mouse report");


I think this might create a whole lot of log messages in case of slower rate limits.
maybe limit that output, too ;-)

I changed this output to log level verbose, which is off by default.

…racker

benjaminaigner · 2020-07-20T08:54:50Z

main/Kconfig.projbuild

+	config MODULE_RATELIMITERACCUMULATE
+		bool "Accumulate mouse HID packets if rate limiter hits (NOT RECOMMENDED)"
+		depends on MODULE_USERATELIMITER
+		default n
+		help
+			If enabled, relative mouse movements are accumulated and
+			sent on next occasion. If disabled, the HID report is discarded.


I think this feature should be disabled by default. I had many cases, that this accumulation winds up and there is no reasonable mouse movement possible....

i do not understand this comment...

L75: default n and not default y :-)

@robinkrens

…(LED report), closing #38 : - Added report ID for LEDs - Added callback event type for LED - Fixed the characteristic & added write no response (according to espressif/esp-idf#5369 THX @robinkrens for the hint!)

Added new config variables for KConfig: -) enabling rate limiter -) set interval in microseconds -) Either accumulate mouse movements or discard reports when sent too fast

…racker

…_keyboard into rateLimiter

…umulate mode not finished! Implemented a timer based solution for accumulating mode.

- moved to esp_hidd_prf_api.c - changed default values for time interval in KConfig - Implemented rate limiter callback - Fixed signed int8 problem - Synchronizing the accumulated values with a binary semaphore

benjaminaigner · 2020-07-21T10:54:38Z

I've moved all the rate limiter stuff to the esp_hidd_prf_api.c file, because it is more reasonable to it there.
The rate limiter is using an esp_timer in accumulate mode, discard mode is working just with a timestamp.

Every packet is sent via the timer, because the default value is set to 500us, which is half the time of the USB request (1ms).
With up to 50ms there is no noticeable delay (this setting should be sufficient for all devices, even for older ones).

ChrisVeigl · 2020-07-21T22:20:38Z

Every packet is sent via the timer, because the default value is set to 500us, which is half the time of the USB request (1ms).
With up to 50ms there is no noticeable delay (this setting should be sufficient for all devices, even for older ones).

OK!
i think we should test the response with different rate limits and devices.
if we want to apply it in the FlipMouse, we need to remove the current rate limiter (which currently just drops everything < 20ms) in bluetooth.cpp: https://github.com/asterics/FLipMouse/blob/56b854f2b889f4f2fac8f5503f65db2b070b8f90/FLipWare/bluetooth.cpp#L19

And it we could make the rate limit adjustable via uart / a new FlipMouse AT-command (eg. AT BC to transfer bluetooth commands to the esp32 - for example "AT BC $RA 80" for setting the rate limiter to 80 ms) ?

benjaminaigner · 2020-07-27T11:27:18Z

OK!
i think we should test the response with different rate limits and devices.
if we want to apply it in the FlipMouse, we need to remove the current rate limiter (which currently just drops everything < 20ms) in bluetooth.cpp: https://github.com/asterics/FLipMouse/blob/56b854f2b889f4f2fac8f5503f65db2b070b8f90/FLipWare/bluetooth.cpp#L19

I don't know if we should remove it completely.
If we do that, we might run into the issue, that the serial buffer is full and packets are dropped within the FM?

ChrisVeigl · 2020-07-27T12:14:30Z

I don't know if we should remove it completely.
If we do that, we might run into the issue, that the serial buffer is full and packets are dropped within the FM?

if we do not remove it, it will compromise the function of the rate limiter inside the esp32 add-on (becuaes it just drops commands), IMO the best implementation for the FlipMouse would be to limit the rate of commands as soon as possible (before USB or BT actions are created).

… from KConfig

benjaminaigner · 2023-02-16T12:46:47Z

This PR is also ~2years old and we don't have any problems with rate limiting.

Implemented rate limiter for HID reports.

a4cd7c4

Added new config variables for KConfig: -) enabling rate limiter -) set interval in microseconds -) Either accumulate mouse movements or discard reports when sent too fast

ChrisVeigl commented Jul 17, 2020

View reviewed changes

ChrisVeigl mentioned this pull request Jul 17, 2020

BLE - GATT transmission rate limiting #37

Open

benjaminaigner added 2 commits July 20, 2020 09:36

Changed rate limiter notification to verbose

0030205

Added notes on accumulate recommendation, added mouse button change t…

0f0c144

…racker

benjaminaigner reviewed Jul 20, 2020

View reviewed changes

benjaminaigner added 8 commits July 20, 2020 14:37

Added test buttons for consumer control keys

6deb4c5

Implemented rate limiter for HID reports.

10a5a57

Added new config variables for KConfig: -) enabling rate limiter -) set interval in microseconds -) Either accumulate mouse movements or discard reports when sent too fast

Changed rate limiter notification to verbose

25178a7

Added notes on accumulate recommendation, added mouse button change t…

0f6550d

…racker

Merge branch 'rateLimiter' of https://github.com/asterics/esp32_mouse…

f12a6e0

…_keyboard into rateLimiter

Moved all the rate limiter stuff into the esp_hidd_prf_api files, acc…

46d4db0

…umulate mode not finished! Implemented a timer based solution for accumulating mode.

working rate limiter with accumulate:

9b172c1

- moved to esp_hidd_prf_api.c - changed default values for time interval in KConfig - Implemented rate limiter callback - Fixed signed int8 problem - Synchronizing the accumulated values with a binary semaphore

benjaminaigner mentioned this pull request Jul 27, 2020

BLE: rate limiter / commands to addon module in general asterics/FLipMouse#38

Open

Changed rate limiter default to accumulate. Removed "NOT RECOMMENDED"…

9f58854

… from KConfig

benjaminaigner closed this Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented rate limiter for HID reports. #39

Implemented rate limiter for HID reports. #39

ChrisVeigl commented Jul 17, 2020

ChrisVeigl Jul 17, 2020

benjaminaigner Jul 20, 2020

benjaminaigner Jul 20, 2020

ChrisVeigl Jul 20, 2020

benjaminaigner Jul 20, 2020

ChrisVeigl Jul 21, 2020

ChrisVeigl Jul 17, 2020

benjaminaigner Jul 20, 2020

benjaminaigner Jul 20, 2020

ChrisVeigl Jul 20, 2020

benjaminaigner Jul 20, 2020

benjaminaigner commented Jul 21, 2020

ChrisVeigl commented Jul 21, 2020

benjaminaigner commented Jul 27, 2020

ChrisVeigl commented Jul 27, 2020

benjaminaigner commented Feb 16, 2023

Implemented rate limiter for HID reports. #39

Implemented rate limiter for HID reports. #39

Conversation

ChrisVeigl commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminaigner commented Jul 21, 2020

ChrisVeigl commented Jul 21, 2020

benjaminaigner commented Jul 27, 2020

ChrisVeigl commented Jul 27, 2020

benjaminaigner commented Feb 16, 2023