-
Notifications
You must be signed in to change notification settings - Fork 69
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 GUI #415
Add GUI #415
Conversation
…a /rosout on humble
This PR contains the first working version of a swarm management tool with some basic functionalities working across cpp, cflib, and sim backends. (The sim is still a bit limited, as it doesn't support default topics such as 'status'). The basic functionality is:
The most notable thing that is missing IMO is reboot, but since that requires significant backend changes, it should be a separate PR. |
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.
I'm getting an error when trying out the cflib backend:
[gui.py-6] Exception in thread Thread-1 (ros_main):
[gui.py-6] Traceback (most recent call last):
[gui.py-6] File "/usr/lib/python3.10/threading.py", line 1016, in _bootstrap_inner
[gui.py-6] self.run()
[gui.py-6] File "/usr/lib/python3.10/threading.py", line 953, in run
[gui.py-6] self._target(*self._args, **self._kwargs)
[gui.py-6] File "/home/kimberly/Development/collaborations/crazyswarm2/ros2_ws/install/crazyflie/lib/crazyflie/gui.py", line 212, in ros_main
[gui.py-6] rclpy.spin(node)
[gui.py-6] File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/__init__.py", line 222, in spin
[gui.py-6] executor.spin_once()
[gui.py-6] File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 713, in spin_once
[gui.py-6] raise handler.exception()
[gui.py-6] File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/task.py", line 239, in __call__
[gui.py-6] self._handler.send(None)
[gui.py-6] File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 418, in handler
[gui.py-6] await call_coroutine(entity, arg)
[gui.py-6] File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 332, in _execute_timer
[gui.py-6] await await_or_execute(tmr.callback)
[gui.py-6] File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 107, in await_or_execute
[gui.py-6] return callback(*args)
[gui.py-6] File "/home/kimberly/Development/collaborations/crazyswarm2/ros2_ws/install/crazyflie/lib/crazyflie/gui.py", line 118, in on_timer
[gui.py-6] t = self.tf_buffer.lookup_transform(
[gui.py-6] File "/opt/ros/humble/lib/python3.10/site-packages/tf2_ros/buffer.py", line 136, in lookup_transform
[gui.py-6] return self.lookup_transform_core(target_frame, source_frame, time)
[gui.py-6] tf2.LookupException: "world" passed to lookupTransform argument target_frame does not exist.
1) unused code removal 2) Better error handling of tf errors 3) Improved handling of simulation time 4) Unified error handling in on_timer
Thanks for the swift review and trying it out. I had tested it with the cflib backend and hadn't seen this error, but it can also be related to timing. I took the time to overhaul the whole error checking in particular with respect to tf a bit. I hope it works now for you as well. This latest change also includes a few improvements for simulation (proper handling of sim vs. real time). |
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.
I was not able to find the issue either so indeed probably related to timing... but I did notice that some things were not properly initialized like the status logging, and I found why that was. See my line comment.
@@ -67,24 +67,31 @@ def __init__(self): | |||
self._ros_parameters = self._param_to_dict(self._parameters) | |||
|
|||
self.uris = [] | |||
self.cf_dict = {} | |||
# for logging, assign a all -> all mapping | |||
self.cf_dict = { |
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.
This causes issues for the if statement on line 367
if self.swarm.fully_connected_crazyflie_cnt == len(self.cf_dict):
self.get_logger().info("All Crazyflies are fully connected!")
self._init_parameters()
self._init_logging()
self.add_on_set_parameters_callback(self._parameters_callback)
else:
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 catch, I think the whole "augmenting" of cf_dict is super scary. Perhaps I can create an additional dict that is just used for logging?
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.
the size of fully connected crazyflie is no longer the same as cf_dict. We should probably do a -1
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.
Sure, the question is just did I miss any other place:-)? I did look for all places when of cf_dict being used and clearly I did not see that one. I also tested with that backend, but only on the desk - no flight.
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.
I pushed a fix for that and another related issue that I found by going through the code again.
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.
Ah woops actually the 'the size of fully connected crazyflie is no longer the same as cf_dict. We should probably do a -1' comment was just an augmentation of my earlier comment, not an answer to your question, as I didn't see that you responded so quickly :P But yes it is dangerous to have a dynamic list like that. Not sure what we can do as an alternative though
One other issue I found is that the start-up is not very reliable. There seems to be a race condition between when the server is launched and when the gui is launched. Sometimes the GUI doesn't 'see' any crazyflie because of that. I'll work on a fix and re-request a review once that is included. |
Ah oke no worries. Let me know if you'd like me to check again. That is why we have the reviews in place:) |
The startup issue is tracked in #422. I would prefer to fix it with a separate PR, because it does touch a lot of code. For now, I don't launch the gui with launch.py anymore (so users have to run it manually after the server is running). |
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.
seems to work now! the only thing that still doesn't work for the cflib is the unicast reporting, but perhaps let's save that for another time and make an issue. Also something we can fix in the future, that the red indication of the crazyflie is sometimes unclear, and perhaps we can make the text red that is causing it (like low battery or such)
Thanks! I added two new issues for these things, so we keep the PRs small:-) |
No description provided.