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

Add rate limiter to Radio link #259

Merged
merged 1 commit into from
Dec 23, 2021
Merged

Add rate limiter to Radio link #259

merged 1 commit into from
Dec 23, 2021

Conversation

ataffanel
Copy link
Member

This will help with P2P connection by making the radio leave some air-time for P2P packets.

A new query "rate_limit=" is added to the radio URI. For example to limit the radio polling rate to 100Hz the uri would be: radio://0/80/2M?rate_limit=100

Fixes #257

Copy link
Contributor

@jonasdn jonasdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Some questions:

  • How does a user discover this feature?

    • (Where do we document it?)
  • How do we tie this to p2p?

    • (How do people with p2p problem discover this solution?)

@@ -641,6 +649,11 @@ def run(self):
else:
waitTime = 0

# If there is a rate limit setup, sleep here to force the rate limit
if self.rate_limit:
time.sleep(1.0/self.rate_limit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, nowhere in the code does anything state that rate_limit is in Hz ... should this be more explicit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An SI rate is normally Hz so I did not think about precising it. Are you envisioning a comment or renaming the variable with "_hz" in the name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, perhaps just some cognitive help while reading this part:

            # If there is a rate limit setup, sleep here to force the rate limit (Hz)
            if self.rate_limit:
                time.sleep(1.0/self.rate_limit)

@ataffanel
Copy link
Member Author

Good questions!

  • How does a user discover this feature?
  • How do we tie this to p2p?

My though was to document it in the p2p page of the firmware: https://github.com/bitcraze/crazyflie-firmware/blob/master/docs/functional-areas/p2p_api.md, This should solve both questions.

@jonasdn
Copy link
Contributor

jonasdn commented Sep 15, 2021

Good questions!

  • How does a user discover this feature?
  • How do we tie this to p2p?

My though was to document it in the p2p page of the firmware: https://github.com/bitcraze/crazyflie-firmware/blob/master/docs/functional-areas/p2p_api.md, This should solve both questions.

So this does only make sense for p2p? Should it then be explicit? ?p2p_mode=1 rather than ?rate_limit=rate? Or do we think that rate limit can be useful for other areas and maybe we should document all query strings somewhere as well?

@whoenig
Copy link
Contributor

whoenig commented Sep 15, 2021

Looks great - thanks! I also added bitcraze/crazyflie-link-cpp#19, perhaps you want to create a similar one for the rust link.

@jonasdn: The idea of having a configurable limit is to allow a user-defined trade-off between p2p packets and host-computer packets. The rate_limit flag might also be useful in automated tests or benchmark to simulate a slow computer or lossy link.

@jonasdn
Copy link
Contributor

jonasdn commented Sep 23, 2021

I added a PR to add documentation to the firmware here: bitcraze/crazyflie-firmware#853

@jonasdn jonasdn merged commit aaa275a into master Dec 23, 2021
@jonasdn jonasdn deleted the ataffanel/issue257 branch December 23, 2021 07:52
@jonasdn jonasdn added this to the next-release milestone Jan 10, 2022
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.

Add P2P mode with relaxed CTRP timing
3 participants