-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add websockets for dotbot commands #201
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
+ Coverage 79.27% 81.02% +1.74%
==========================================
Files 21 26 +5
Lines 2157 2640 +483
==========================================
+ Hits 1710 2139 +429
- Misses 447 501 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Nice, but this doesn't solve the polling strategy that is used to update the status of all robots, right? |
|
Cool! Did you notice any speedup improvement? (More than 200 bots possible?) Agree with @aabadie's comment, maybe you could have the example code have a callback that receives ws updates (per-robot updates would be ideal[1]) and writes them to a thread-safe list or dict. [1] per-robot updates would maintain the whole architecture in a granular upstream flow of information: |
Yes! Dotbot polling only happens once per control loop, while sending commands such as waypoints or rgb_led scales as O(n) with the number of robots. |
|
|
||
|
|
||
| @api.websocket("/controller/ws/dotbots") | ||
| async def ws_dotbots(websocket: WebSocket): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add a test function for this new endpoint? server.py is 100% covered by test (there are not so many modules in this case, so let's not break that one ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Testcases added on:
|
Thanks for adding the tests. I believe this is ready to go! |
Update all commands to also have a websocket variant.
This makes us sending commands to hundreds of dotbots almost instantly. Before we needed to wait a connection from the pool, reopen, send the message, close it, etc.. It was slow for when the number of bots >100