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.

Sign up for GitHub

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

Jump to bottom

Feature/RosbagController #1791

Merged
merged 8 commits into from
Jan 24, 2019
Merged

Feature/RosbagController #1791

merged 8 commits into from
Jan 24, 2019

Conversation

ChenxiTU
Copy link

Status

DEVELOPMENT

Description

rivz plugin for rosbag recording (rosbag play module will be added in the future)

Two ways to set topics parameter for recording:

  1. Use .yaml configure file as configure_example.yaml file.
  2. Use Topic Refresh to real-time scan which topics are existing, then choose those you are interested in by checkboxes.

Note1: Configure file and topic from checkboxes can be used at same time, even some topics are repeated (plugin will do duplicate checking when using both of them)
Note2: If you want to clear configure file, you can restart plugin

Todos

  • Tests

Steps to Test

  1. Start rviz and choose Panels->Add New Panel->RosbagController
  2. You can use the plugin to record rosbags with specified topics (do not specify any topics means recording all)

@kitsukawa
Copy link

@ChenxiTU Autoware/ros/src/util/packages/autoware_rviz_plugins is empty. Can you push the code?

@ChenxiTU
Copy link
Author

Sorry, my mistake. Code could be seen now.

isamu-takagi
isamu-takagi requested changes Dec 26, 2018
View reviewed changes
Copy link
Contributor

@isamu-takagi isamu-takagi left a comment

Choose a reason for hiding this comment

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

Please check the following.

  • Specify the license in package.xml.
  • L.165-168 in src/autoware_rosbag_plugin.cpp have no effect.
    Is it right that append_date is always set to false?

These are different from the other variable naming conventions. Fix if you want.

  • L.65 in src/autoware_rosbag_plugin.h: ros::Duration record_time_duration_
  • L.83 in src/autoware_rosbag_plugin.h: Ui::Autoware_Rosbag_Plugin *ui
  • L.89 in src/autoware_rosbag_plugin.h: bool record_status

@mitsudome-r
Copy link
Member

mitsudome-r commented Dec 28, 2018 •
edited
Loading

Good Wok!

Just concerned about follwoing:
Could you explain briefly what you have changed from original rosbag source code in recorder.cpp?
In other words, is there any particular reason that you did not use original "rosbag record"? (e.g. by QProcess )
I would prefer to do so since that would keep us from updating this package every time the original rosbag source code changes.

@ChenxiTU
Copy link
Author

@isamu-takagi
Thanks for your careful review.

  • I Added Apache 2.0 license in package.xml.
  • I deleted L.165-168 in src/autoware_rosbag_plugin.cpp, which is useless. Personally, I'd like to set append_date always false so we can obtain a succinct filename.
    And thank you for your advice of variable naming.
  • I fix L.89 in src/autoware_rosbag_plugin.h: bool record_status into bool record_status_. It was a mistake.

@mitsudome-r
Thank you for your review and advice.
The reason why I do not use original rosbag source code is that just using original is hard to control the start and stop (especailly stop) from GUI well.
What I have changed is:

  • Add two methods void Recorder::stop() and int Recorder::start() to start and stop recording

@amc-nu
Copy link
Member

amc-nu commented Jan 10, 2019

@ChenxiTU thanks for fixing

Can you please merge latest develop branch into yours, and push again.?
Thanks

@ChenxiTU
Copy link
Author

@amc-nu

Sorry, my mistake. Latest develop was merged now.

@isamu-takagi
Copy link
Contributor

@ChenxiTU

I find warning and aborting. Can you merge this?

  • Warning when refresh button is pushed
  • Aborting when parsing yaml file is failed

@ChenxiTU
Copy link
Author

@isamu-takagi
Thank you. I merged you commits.

@isamu-takagi
Copy link
Contributor

isamu-takagi commented Jan 11, 2019 •
edited
Loading

@ChenxiTU
Thank you for merging. Looks good. This is useful tool!

@kitsukawa
Copy link

@ChenxiTU Can you modify to list topics in alphabetical order?

@kitsukawa
Copy link

@ChenxiTU Thank you. Can you add license notice (Apache 2.0)in the code?

@kfunaoka
Copy link

Would you declare at autowarefoundation/autoware_ai#476 if working for v1.11?

@kitsukawa kitsukawa merged commit 92425d7 into develop Jan 24, 2019
@kitsukawa kitsukawa deleted the feature/rosbag_controller branch January 24, 2019 01:31
@kitsukawa kitsukawa mentioned this pull request Jan 24, 2019
Closed
1 task
@kfunaoka kfunaoka mentioned this pull request Jan 24, 2019
Merged
@esteve esteve mentioned this pull request Jan 29, 2019
Closed
1 task
@sgermanserrano sgermanserrano mentioned this pull request Mar 14, 2023
Closed
17 tasks
anubhavashok pushed a commit to NuronLabs/autoware.ai that referenced this pull request Sep 7, 2021
@ChenxiTU
Feature/RosbagController (autowarefoundation#1791)
068fe0c 
* add rviz plugin RosbagController

* add rosbag_controller

* refine core and add specify the license

* Fix runtime warning and aborting

* sort topics alphabetically

* add license in .cpp .h
@mitsudome-r mitsudome-r added the version:autoware-ai Autoware.AI label Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isamu-takagi isamu-takagi isamu-takagi approved these changes

@YamatoAndo YamatoAndo Awaiting requested review from YamatoAndo

Assignees
No one assigned
Labels
version:autoware-ai Autoware.AI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ChenxiTU @kitsukawa @mitsudome-r @amc-nu @isamu-takagi @kfunaoka