-
Notifications
You must be signed in to change notification settings - Fork 324
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 simple ROS-based simulator (using RVIZ for visualization) #504
base: master
Are you sure you want to change the base?
Conversation
Usage: Terminal 1: roslaunch crazyswarm sim.launch Terminal 2: python3 example_distributed.py
Usage: Terminal 1: roslaunch crazyswarm sim.launch Terminal 2: python3 example_distributed.py
* Improved example_distributed * The new ROS simulator can be used with $ roslaunch crazyswarm hover_swarm.launch sim:=True $ (python-script-without --sim-argument) * Implements some missing features
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.
Functionality looks good! Design-wise, I am not sold on the use of inheritance. I think composition would be more clear. Is there some place I am missing where inheritance is making the code much simpler?
Thanks - good point on inheritance vs. composition. I didn't actively think about it and I do agree that it would be worth the (small) effort to change it to make the code more readable. I'll update the PR and let you know once I am done for a second round. |
This also addresses some other minor points raised by jpreiss in #504
Thanks, this looks good to me. Re. documentation, I think this is a big feature that users might want for many reasons. If it's only mentioned in a distributed systems tutorial, some users might not be aware of it even though it would help them. I wonder if we need to add a top-level reference section or "system overview" where we can put reference-like material that is not part of the Python API? Re. Python2, are we using some Python3 feature that would be annoying to give up? I don't see any f-strings... If there is no real cost, I feel we might as well continue supporting it. If it becomes a burden I agree we can start introducing Py3-only features. |
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.
Updates for composition and other nits look good. Still a few points for discussion before merging
This feature opens up the opportunity to test the ROS side of Adding the ability to run unit tests in this mode would also test the new code introduced in this PR, so I think it is worth the effort to get it working before merging into |
This can be used as foundation to add more documentation for PR #504
The PR is updated and based on the latest master branch. This new version now also includes documentation for this new feature. Re Python versions: I don't really use any specific Python3 features. However, I don't have Python2 installed and it did reach its end-of-life. So I'd vote to either discontinue support, or have it somehow part of the CI so that we don't manually have to test if we break it. Re unit tests: Agreed - this would be nice. Could be either part of this PR, or a separate effort. |
I implemented testing on https://github.com/USC-ACTLab/crazyswarm/tree/dev-sim-ros-testing-rebase. Please merge into this branch if you agree with the design. Currently a lot of tests are failing. There are many possible reasons for test failures:
We also get a complete hang in Ubuntu 18 / Python 2, but fixing that is probably straightforward compared to the test failures. I am not at a ROS PC right now but I can debug the tests in the next day or two. If any failures require significant engineering to fix, we can just use pytest marking to skip them in the ROS simulation for now, and fix them in separate commits. |
something odd is happening with the ROS tests. When I run a single test like But if I run its whole containing file like I try to kill the whole ROS process tree in the test fixture, but it seems like something is persisting. Or, there is something persisting in the client ( |
I wonder if this is some strange ROS1 bug. From the logs in |
Switching from tf to tf2 fixes those issues. There are still plenty of failures left, most of them seem to be weirdly numerical mismatches. Perhaps a delay caused by tf? |
yes it seems like the non-blocking nature of |
It is blocking in the sense that it waits until a transform is available and then returns the latest one. It would be possible to also pass in the current timestamp, but that might just add latency in the position() call. |
Yes, I think some of the test failures happen because the We could fix this by adding a little extra time to all |
I skipped some tests (notably all collisionAvoidance) and modified some other ones to accommodate ROS. Saving simulator time for later. Now the test suite has no failures on my Ubuntu 20 machine, but it's still failing in CI... |
When using the debug shell on the Github runner, I was able to make the tests pass by increasing the |
The wait just blocks until the process is killed, which takes on my machine less than 1s. According to the ROS logs, all the processes are terminated and relaunched (on a local machine that is). Perhaps we can run the tests in CI with the |
The re-base somehow messed up the diff, so re-opening a new PR here (see #350 for earlier discussion)
This is a much improved version, with:
roslaunch crazyswarm hover_swarm.launch sim:=True
example_distributed.{py,launch}
What's missing: