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

New teleop plugin implementation. #245

Merged
merged 11 commits into from
Jul 19, 2021
Merged

Conversation

LolaSegura
Copy link
Contributor

@LolaSegura LolaSegura commented Jun 28, 2021

Signed-off-by: LolaSegura lsegura@ekumenlabs.com

🎉 New feature

Closes #186

Summary

The user can insert the GUI teleop plugin in a world that has a vehicle and setting a desire topic, can send commands by pressing buttons, using the keyboard or with sliders.

Test it

You can test it loading a world in ignition gazebo that has a vehicle like ign gazebo diff_drive_skid.sdf, selecting the Teleop plugin, configuring the desired topic to send the twist commands, /model/vehicle_blue/cmd_vel.

OpenPlugin.mp4

To try the buttons, you have to set the desired linear and angular velocity and then click on the direction buttons. More than one button can be selected in order to rotate while moving forward or backward.

To try the keyboard, first you have to enable it by clicking on the "keyboard" switch. The keys are W, A, D, X.

To use the sliders you have to enable them by clicking on the "Sliders" switch. The maximum and minimum velocity are configurable.

MovingCar.mp4

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
@osrf-triage osrf-triage added this to Inbox in Core development Jun 28, 2021
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jun 28, 2021
@francocipollone francocipollone self-requested a review June 28, 2021 17:57
@francocipollone
Copy link
Contributor

I will review it, in the meantime @LolaSegura is working on the tests

@chapulina chapulina moved this from Inbox to In progress in Core development Jun 28, 2021
@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #245 (7d73aee) into ign-gui3 (50c6253) will increase coverage by 0.74%.
The diff coverage is 85.08%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui3     #245      +/-   ##
============================================
+ Coverage     65.71%   66.46%   +0.74%     
============================================
  Files            23       25       +2     
  Lines          2835     2949     +114     
============================================
+ Hits           1863     1960      +97     
- Misses          972      989      +17     
Impacted Files Coverage Δ
src/plugins/teleop/Teleop.cc 84.95% <84.95%> (ø)
src/plugins/teleop/Teleop.hh 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50c6253...7d73aee. Read the comment docs.

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

The first iteration is done. I haven't review the test file yet.

src/plugins/teleop/CMakeLists.txt Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.hh Show resolved Hide resolved
src/plugins/teleop/Teleop.hh Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
Core development automation moved this from In progress to In review Jul 6, 2021
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
@LolaSegura LolaSegura force-pushed the LolaSegura/new_teleop_plugin branch from ffa7aee to ebbf1b5 Compare July 8, 2021 15:56
@francocipollone francocipollone marked this pull request as ready for review July 12, 2021 17:10
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! That's a lot of features!

src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.hh Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.qml Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.qml Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop.qml Show resolved Hide resolved
@francocipollone
Copy link
Contributor

@LolaSegura Nice Job!: Some general comments.

  • While testing the plugin I solved one bug that only happens when the first message was sent. The first message was missed. I also fix a minor reference in the qml code. I pushed the commit: 3543cde
  • Please take a look at the unresolved conversations and if they are addressed add a comment
  • Check that your last commit is unverified.

Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some more comments mainly related to the test file.

src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
LolaSegura and others added 2 commits July 16, 2021 12:45
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@LolaSegura LolaSegura force-pushed the LolaSegura/new_teleop_plugin branch from 07a98de to b8bccc0 Compare July 16, 2021 15:49
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
@LolaSegura LolaSegura force-pushed the LolaSegura/new_teleop_plugin branch from b8bccc0 to f953516 Compare July 16, 2021 19:13
Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!, I left some nits!

src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
src/plugins/teleop/Teleop_TEST.cc Outdated Show resolved Hide resolved
LolaSegura and others added 3 commits July 16, 2021 17:30
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looking good! I pushed some changes in 8328a17 which I hope will fix the Windows build.

@chapulina chapulina enabled auto-merge (squash) July 17, 2021 01:57
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina chapulina merged commit 81a0913 into ign-gui3 Jul 19, 2021
Core development automation moved this from In review to Done Jul 19, 2021
@chapulina chapulina deleted the LolaSegura/new_teleop_plugin branch July 19, 2021 07:52
@chapulina chapulina moved this from Done to Highlights in Core development Jul 19, 2021
@j-rivero j-rivero removed this from Highlights in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants