Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mqtt Command Throttling #1056

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jamidd
Copy link
Contributor

@Jamidd Jamidd commented Aug 19, 2024

This PR aims to prevent excessive command spamming via MQTT, which could potentially cause the device to crash.

@atroncoso-tucar atroncoso-tucar force-pushed the tucar/feature/MQTT-command-throttling branch 3 times, most recently from 1e00c93 to 4e47ec2 Compare August 27, 2024 14:04
@dexterbg
Copy link
Member

Jaime, I've got some issues with this.

  1. The variable name is confusing.
  2. The implementation will block all incoming messages from being processed.
  3. Incoming commands will not be throttled but dropped.

On 1: please generally take care naming variables according to their function: m_accept_command implies false/0 will block commands. Instead it's vice versa. A better name would be m_block_command, or even better m_command_block_ticker to also reflect it's a timer variable.

On 2: to properly throttle commands, move the throttling into OvmsServerV3::IncomingMsg(), and only apply it to the command/… topic scheme.

On 3: dropping commands is bad, as MQTT clients normally cannot check for responses and issue resubmissions easily. Instead of dropping the command, queue it. Add the queue check & delayed execution to the per second ticker handler.

While you're at it, making the delay and queue size configurable is also a good idea.

Regards,
Michael

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants