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

[jsk_topic_tools] Python ConnectionBasedTransport #1178

Merged
merged 6 commits into from
Oct 13, 2015
Merged

[jsk_topic_tools] Python ConnectionBasedTransport #1178

merged 6 commits into from
Oct 13, 2015

Conversation

wkentaro
Copy link
Member

No description provided.

@wkentaro
Copy link
Member Author

This PR contains:

  • rostest for cpp ConnectionBasedNodelet
  • implementation of python ConnectionBasedTransport
  • rostest for python ConnectionBasedTransport

wkentaro
wkentaro reviewed Oct 12, 2015
View reviewed changes
jsk_topic_tools/CMakeLists.txt
@@ -98,6 +98,8 @@ if (NOT $ENV{ROS_DISTRO} STREQUAL "indigo")
add_rostest(test/test_hz_measure.test)
add_rostest(test/test_block.test)
endif(NOT $ENV{ROS_DISTRO} STREQUAL "indigo")
Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, I don't know why there is condition about indigo here..

Copy link
Member

Choose a reason for hiding this comment

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

older version of catkin could not execute add_rostest

Copy link
Member Author

Choose a reason for hiding this comment

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

So you mean the catkin version on hydro is newer than that on indigo?
I wonder why NOT ... STREQUAL.

Copy link
Member

Choose a reason for hiding this comment

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

newer than default hydro but older than current latest indigo.

It should be fixed the latest indigo catkin.

Copy link
Member

Choose a reason for hiding this comment

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

see ros/catkin#703

Copy link
Member Author

Choose a reason for hiding this comment

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

I see.
So at this time, should I write add_rostest lines in side the if condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the commit.

@garaemon
Copy link
Member

I think this feature should be implemented in jsk_recognition_utils

@wkentaro
Copy link
Member Author

So the location of connectionbasednodelet is also wrong?

@garaemon
Copy link
Member

connectionbasednodelet

It does not expect sensor_msgs/Image.

@wkentaro
Copy link
Member Author

ConnectionBasedTransport does not expect sensor_msgs/Image either.
I used sensor_msgs/Image in the sample.

@garaemon
Copy link
Member

I used sensor_msgs/Image in the sample.

I see

@wkentaro wkentaro force-pushed the python-connection-based-transport branch from f1e66df to 55ee6fa Compare October 12, 2015 13:14
garaemon
garaemon reviewed Oct 12, 2015
View reviewed changes
jsk_topic_tools/src/jsk_topic_tools/transport.py
class ConnectionBasedTransport(rospy.SubscribeListener):
def __init__(self, node_name):
super(ConnectionBasedTransport, self).__init__()
rospy.init_node(node_name)
Copy link
Member

Choose a reason for hiding this comment

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

I think programmer should call rospy.init_node before creating ConnectionBasedTransport.
It would be problem if user want to create several instances of ConnectionBasedTransport

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted thanks. I will change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think programmer should call rospy.init_node before creating ConnectionBasedTransport.
It would be problem if user want to create several instances of ConnectionBasedTransport

I updated the commit.

@wkentaro
Copy link
Member Author

I updated the commit about add_rostest in CMakeLists.txt.

@garaemon
Copy link
Member

I updated the commit about add_rostest in CMakeLists.txt.

uncomment if-block after this PR is merged.

@wkentaro
Copy link
Member Author

uncomment if-block after this PR is merged.

Which line's comment should I remove?

@wkentaro wkentaro force-pushed the python-connection-based-transport branch from 783fd2b to d787f42 Compare October 12, 2015 13:23
@garaemon
Copy link
Member

Which lines comment should I remove?

https://github.com/jsk-ros-pkg/jsk_common/blob/master/jsk_topic_tools/CMakeLists.txt#L90

We may have trouble in running test like #1177

@wkentaro
Copy link
Member Author

https://github.com/jsk-ros-pkg/jsk_common/blob/master/jsk_topic_tools/CMakeLists.txt#L90

So you mean comment out the if() and endif() ? Because the bug has been fixed in the latest catkin?

We may have trouble in running test like #1177

like #177 ? You mean a bug of catkin?

@garaemon
Copy link
Member

I'm just afraid that these tests are not stable.
I don't want to add unstable test code and PR should be separated to see what will happen in testing.

@wkentaro wkentaro force-pushed the python-connection-based-transport branch 3 times, most recently from 6cb3345 to 5f7c712 Compare October 12, 2015 15:24
@wkentaro
Copy link
Member Author

I see.
And do you mean I should split the PR?

2015年10月12日月曜日、Ryohei Uedanotifications@github.comさんは書きました:

I'm just afraid that these tests are not stable.
I don't want to add unstable test code and PR should be separated to see
what will happen in testing.

—
Reply to this email directly or view it on GitHub
#1178 (comment)
.

和田 健太郎 / Kentaro Wada
http://wkentaro.com

@garaemon
Copy link
Member

If it raises strange error, split PR.
If not, you don'nt need to do

@wkentaro wkentaro force-pushed the python-connection-based-transport branch from 5f7c712 to 0ed0eee Compare October 12, 2015 16:38
@wkentaro
Copy link
Member Author

python-enum34 is not released on apt for precise.
ros/rosdistro#9635

@wkentaro wkentaro mentioned this pull request Oct 12, 2015
Merged
@wkentaro wkentaro force-pushed the python-connection-based-transport branch from f271c9c to 609fed6 Compare October 12, 2015 18:54
garaemon added a commit that referenced this pull request Oct 13, 2015
@garaemon
Merge pull request #1178 from wkentaro/python-connection-based-transport
00b1b8a 
[jsk_topic_tools] Python ConnectionBasedTransport
@garaemon garaemon merged commit 00b1b8a into jsk-ros-pkg:master Oct 13, 2015
@wkentaro wkentaro deleted the python-connection-based-transport branch October 13, 2015 09:11
wkentaro added a commit to wkentaro/jsk_common that referenced this pull request Oct 18, 2015
@wkentaro
[jsk_topic_tools] add_rostest problem should be fixed in latest c…
4529699 
…atkin

For jsk-ros-pkg#1178 (comment)
wkentaro added a commit to wkentaro/jsk_common that referenced this pull request Oct 19, 2015
@wkentaro
[jsk_topic_tools] add_rostest problem should be fixed in latest c…
9a0b0d8 
…atkin

For jsk-ros-pkg#1178 (comment)
@wkentaro wkentaro mentioned this pull request Oct 19, 2015
Merged
@k-okada k-okada mentioned this pull request Jun 20, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wkentaro @garaemon