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

[jsk_pcl_ros] Delete subclass's updateDiagnostic method #2323

Merged
merged 1 commit into from Aug 29, 2018

Conversation

iory
Copy link
Member

@iory iory commented Aug 24, 2018

In jsk_pcl_ros, depth_calibration_nodelet, line_segment_detector_nodelet and line_segment_collector_nodelet are not updating diagnostics.
Before this PR, they output error of diagnostics with "No message was set" message and
we can not change the error level.
Like this

image

After this PR, we can change the error level.

image

@iory iory requested a review from furushchev August 24, 2018 05:00
Copy link
Member

@furushchev furushchev left a comment

Choose a reason for hiding this comment

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

In the case that a subclass only calls super class method, You can instead simply remove the method (from both source / header files)

@iory
Copy link
Member Author

iory commented Aug 24, 2018

Thank you for your review!
Refer to your comment, I delete the methods.

@furushchev
Copy link
Member

@iory Could you also rebase commits and update the title of this PR?

@iory iory changed the title [jsk_pcl_ros] Add diagnostics update [jsk_pcl_ros] Delete subclass's diagnosticUpdate method Aug 24, 2018
@iory iory changed the title [jsk_pcl_ros] Delete subclass's diagnosticUpdate method [jsk_pcl_ros] Delete subclass's updateDiagnostic method Aug 24, 2018
@iory
Copy link
Member Author

iory commented Aug 24, 2018

OK. I rebased and modified title.

@furushchev
Copy link
Member

@iory Thanks for iterating. Now LGTM!

@k-okada
Copy link
Member

k-okada commented Aug 27, 2018

I think this change assumes some recent version of upstream package. If so, can you add that information to build/run depend of package.xml?

@iory
Copy link
Member Author

iory commented Aug 27, 2018

This change assumes jsk_topic_tools 2.2.7, and it is already added in package.xml.
8af921a#diff-9073b2db22a93942cb7774fd0fb2bbafL26

@iory
Copy link
Member Author

iory commented Aug 28, 2018

@k-okada
Please check this PR

@k-okada
Copy link
Member

k-okada commented Aug 28, 2018 via email

@iory
Copy link
Member Author

iory commented Aug 28, 2018

@k-okada
Travis passed. Please merge this.

@k-okada k-okada merged commit 9fa41be into jsk-ros-pkg:master Aug 29, 2018
@iory iory deleted the diag branch April 10, 2022 14:27
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.

None yet

3 participants