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 GUI #415

Merged
merged 15 commits into from
Feb 9, 2024
Merged

Add GUI #415

merged 15 commits into from
Feb 9, 2024

Conversation

whoenig
Copy link

@whoenig whoenig commented Feb 1, 2024

No description provided.

@whoenig whoenig marked this pull request as ready for review February 5, 2024 09:48
@whoenig
Copy link
Author

whoenig commented Feb 5, 2024

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:

  • CFs are shown in a 3D view (based on tf)
  • Each drone is either green (everything is fine) or red (some issues were detected)
  • When clicking on a drone in the 3D view or on the tab, the drone is enlarged and more detailed info is shown (battery voltage, supervisor bits, communication status, relevant logging data)

The most notable thing that is missing IMO is reboot, but since that requires significant backend changes, it should be a separate PR.

Copy link
Collaborator

@knmcguire knmcguire left a 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.

crazyflie/scripts/gui.py Outdated Show resolved Hide resolved
crazyflie/scripts/gui.py Outdated Show resolved Hide resolved
1) unused code removal
2) Better error handling of tf errors

3) Improved handling of simulation time
4) Unified error handling in on_timer
@whoenig
Copy link
Author

whoenig commented Feb 6, 2024

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).

Copy link
Collaborator

@knmcguire knmcguire left a 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 = {
Copy link
Collaborator

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:

Copy link
Author

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?

Copy link
Collaborator

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

Copy link
Author

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.

Copy link
Author

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.

Copy link
Collaborator

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

@whoenig
Copy link
Author

whoenig commented Feb 7, 2024

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.
Sorry - this PR is also getting rather large now:-/

@knmcguire
Copy link
Collaborator

Ah oke no worries. Let me know if you'd like me to check again. That is why we have the reviews in place:)

@whoenig
Copy link
Author

whoenig commented Feb 8, 2024

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).

Copy link
Collaborator

@knmcguire knmcguire left a 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)

@whoenig whoenig merged commit e4bb0cf into main Feb 9, 2024
3 checks passed
@whoenig
Copy link
Author

whoenig commented Feb 9, 2024

Thanks! I added two new issues for these things, so we keep the PRs small:-)

@whoenig whoenig deleted the feature_gui branch February 9, 2024 10:57
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