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

Prioritize broadcast commands #30

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Prioritize broadcast commands #30

merged 1 commit into from
Jan 30, 2024

Conversation

whoenig
Copy link
Contributor

@whoenig whoenig commented Mar 9, 2023

The current scheduler several issues:
1.) automatic pings are sent in a round robin fashion. For example, consider the case where we have 10 CFs, with only one being busy to send commands. Then, we will send pings to 9 CFs, artificially delaying the 1 important connection.
=> Solution: Only send a ping if all connection queues are empty.
2.) Broadcasts are handled like unicast connections, even though these are typically low-latency connections.
=> Solution: Prioritize all broadcasts before sending any unicast commands
3.) We sometimes reconfigure the radio, even if nothing needs to be done
=> Solution: check before the radio is reconfigured

The current scheduler several issues:
1.) automatic pings are sent in a round robin fashion. For example, consider the case where we have 10 CFs, with only one being busy to send commands. Then, we will send pings to 9 CFs, artificially delaying the 1 important connection.
=> Solution: Only send a ping if all connection queues are empty.
2.) Broadcasts are handled like unicast connections, even though these are typically low-latency connections.
=> Solution: Prioritize all broadcasts before sending any unicast commands
3.) We sometimes reconfigure the radio, even if nothing needs to be done
=> Solution: check before the radio is reconfigured
@whoenig whoenig requested a review from ataffanel March 9, 2023 15:04
@whoenig
Copy link
Contributor Author

whoenig commented Mar 9, 2023

Adding @ataffanel, since this is important also for Crazyradio2 on-board scheduling. I hope that there will be proper support for broadcasts:-)?

@ataffanel
Copy link
Member

Packet scheduling is an interesting topic and indeed a good thing to think about in the context of Crazyradio2.

As for your current changes I have some comments:

  • in the 1st case, there should be a mechanism to make sure no connection is starved since ping are used to carry down-link . I could see a case where one trajectory is uploaded to one Crazyflie which keeps one up-link queue full, it would lock (and so increase latency) for the down-link of all 9 other Crazyflies until the trajectory upload is done. This is something that should likely be tested to see if it is a real-world problem or just a theoretical one. Overall this is a latency fairness decision: pure round-robin ping guarantees that all Crazyflie gets similar up-link and down-link latency at the expense of minimum latency and max-bandwidth. Optimizing for minimal latency on the uplink will make the latency much less predictable (it will depend of the traffic) and will potentially dramatically increase down-link latency (my point earlier).
  • In the second, broadcast, case, I would have a similar comment: there might need to be some though on not starving the regular connections. This is a bit less problematic though since it is very easy to explain to the user that broadcast have priority, so if there is too many broadcast packets, no regular connection can be served.
  • The 3rd point is great and will not be required anymore with Crazyradio 2 (the channel/address will be part of the packet on USB).

@whoenig
Copy link
Contributor Author

whoenig commented Mar 14, 2023

  1. True, but in which use-cases do we actually care about down-link latency? The up-link has the commands, which one usually expects to have very low latency, while the down-link has mostly status-information, often timestamped (e.g., when enabling logging).
  2. This is actually on purpose, since I saw the opposite: the (unimportant) unicast connections delayed the broadcasts, causing undesired behavior. We use broadcasts mostly for the motion capture and for good performance it's crucial to send the data with the lowest latency possible. I believe it also makes sense to use broadcasts (to a single CF) for other low-level commands (cmd_vel for example), since safelink and other things don't actually make sense in that context.

For both cases, I am actually planning to add some monitoring component that can warn the user if a connection is starved. It might even be possible to measure latency using the echo port semi-continuously and issue a warning if starving occurs (which likely means that there are too many connections per radio). For Crazyswarm2, this is tracked in IMRCLab/crazyswarm2#209. I think we would need something similar for Crazyradio2 (i.e., a way to query statistics that we can use to warn users in undesirable cases).

It might also make sense to revive the realtime aspect of CRTP to formally address 1. and 2. - the current ports don't reflect priorities well, but we could document the "proper" ordering and then relax the conditions in this PR to only starve connections for high-priority/low-latency packets.

@ataffanel
Copy link
Member

Sorry for the delayed answer.

  1. I can imagine that anyone doing centralized control (taking decision on the PC) will be interested about both up and down-link latency being predictable. Most importantly though, very spiky latency can be very surprising and very hard to understand for the user: you expect communication to just work in both direction and that is what the current default behavior is implementing. So prioritizing up-link might be good for some (most?) use-case but I think it should be an option rather than the default.
  2. Priority on broadcast is less of a problem form me since this makes a lot of sense as soon as you want to use broadcast and it would delay or block all communication evenly so it is much easier to understand from a user point of view.

As for CRTP, we are currently starting to work on the new comm stack, the first step is to separate CRTP from the rest of the firmware: handling binary packet in on separate module and calling APIs in the Crazyflie. This should make experimentation, like realtime, much easier in the future and will also allow to test other protocol. Realtime and prioritization requirements are actually very interesting to think about when we make a new protocol.

@whoenig
Copy link
Contributor Author

whoenig commented Aug 28, 2023

Was there any progress at the new com stack @ataffanel ?

How would you feel about this prioritization, if a user of this library can provide a priority for each packet? Then the client library can decide what's important (and Crazyswarm2 would use the lowest priority for broadcasts). This also solves the problem that the CRTP priorities are not necessarily logically anymore, i.e., a user-library can re-order to make cmd's more important than logging.

@ataffanel
Copy link
Member

Was there any progress at the new com stack @ataffanel ?

Not much unfortunately, we will likely be focusing on it from January together with the new unified lib.

How would you feel about this prioritization, if a user of this library can provide a priority for each packet? Then the client library can decide what's important (and Crazyswarm2 would use the lowest priority for broadcasts). This also solves the problem that the CRTP priorities are not necessarily logically anymore, i.e., a user-library can re-order to make cmd's more important than logging.

This sounds good to me.

@ataffanel ataffanel merged commit d403883 into master Jan 30, 2024
@ataffanel ataffanel deleted the prio_broadcast branch January 30, 2024 14:37
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