-
Notifications
You must be signed in to change notification settings - Fork 200
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
Use Arguments in launch files and enable their reuse. #76
Conversation
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.
happy with the changes once there's some examples
25d9d6f
to
2883667
Compare
@destogl Do you need help on this pull request ? |
1ec974c
to
a550d90
Compare
* Fix wrong macro in rrbot_system_multi_interface.urdf * First correction to README.md (forward_command_controller_position) * Bump ros2 CI actions to 2 for setup-ros Co-authored-by: Olivier Stasse <[email protected]>
@olivier-stasse would also like to get review from you. |
Again some ros2 issues on the CI... |
Dear @destogl I was just reporting the constraints from the interface: it was saying that one reviewer with write access should validate the PR. You, @bmagyar and @Karsten1987 are doing an incredible job, I was just trying to avoid having PR piling up without proper grounds. Thanks again for your time and dedication. |
So we have to wait that the package number is updated (I checked the number of the package required and indeed it does not correspond) ? Or is it a transient situation with the buildfarm ? |
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.
Demos work fine for me aside for a few minor details.
Current Example enumeration goes like this: 1 -> 1 -> 2 -> 10 :D
README.md
Outdated
The two illegal controllers demonstrate how hardware interface declines faulty claims to access joint command interfaces. | ||
|
||
|
||
### Example 10: "Differential drive mobile robot" |
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.
For this demo the default rviz configuration file did not set up correctly the fixed frame (I'm guessing it's odom
) nor did it showed the Robot Model.
After fixing this and sending the velocity command the demo runs fine.
One thing I noticed is the velocity being correctly updated in the /joint_states
while the wheel positions is fixed to zero.
header:
stamp:
sec: 1623246367
nanosec: 753984445
frame_id: ''
name:
- right_wheel_joint
- left_wheel_joint
position:
- 0.0
- 0.0
velocity:
- 50.0
- 43.33333333333333
effort:
- .nan
- .nan
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.
Thanks @jordan-palacios I will try to propose a fix by tomorrow morning.
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've made some minor comments on the documentation, hope you don't mind!
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 mentioned it inline, but I'd consider moving the description for a specific robot in their own README and link it from the top-level
PR #104 adresses the reviews. Thanks to all. |
Co-authored-by: Jordan Palacios <[email protected]>
This setup ensured the quality of packages in general. Nevertheless, we value every review because this guarantees more eyes and even higher quality and sensibility of an implementation :) |
Co-authored-by: mahaarbo <[email protected]> Co-authored-by: Karsten Knese <[email protected]>
* Implement classical differential wheel robot. * Add rviz configuration file. Co-authored-by: Olivier Stasse <[email protected]> Co-authored-by: Denis Štogl <[email protected]>
Co-authored-by: mahaarbo <[email protected]>
Thank you all for the input! Especially to:
It's really great to be a part of such a community :) |
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 agree with Olivier. Let's get this one in and follow up with other smaller increments in case it's still not fully accurate.
Phew, this was quite a marathon to review! :D merging... |
@Karsten1987 @bmagyar thanks for final approvals! |
@destogl did you ever test the gazebo demo of this PR?
I guess that's really on me though, I should have tested it correctly before approving. |
@Karsten1987 @destogl my bad too. I have a fix for starting Gazebo here olivier-stasse@4712995 I will try to test the controller probably having in mind the fixes in PR #103 . |
Also add parameters for starting FakeSystem to demonstrate its functionality.
Add documentation about this Example and integrate into control.ros.org.