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

Add and_scale_ros package #119

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pazeshun
Copy link
Contributor

@pazeshun pazeshun commented Aug 14, 2017

Depends on jsk-ros-pkg/jsk_recognition#2193

This PR adds ROS interface for A&D scales (only supports EK-i/EW-i series currently).

Based on https://github.com/start-jsk/jsk_apc/blob/master/jsk_arc2017_common/node_scripts/ekew_i_driver.py

Copy link
Member

@k-okada k-okada left a comment

Choose a reason for hiding this comment

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

Thanks , added comments

and_scale_ros/package.xml Outdated Show resolved Hide resolved
and_scale_ros/package.xml Outdated Show resolved Hide resolved
<license>BSD</license>

<buildtool_depend>catkin</buildtool_depend>
<run_depend>jsk_recognition_msgs</run_depend>
Copy link
Member

Choose a reason for hiding this comment

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

Please check whole dependency graph, may be jsk_recognition depend on jsk_3rdparty , so this makes circular dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think jsk_recognition_msgs doesn't depend on jsk_3rdparty nor jsk_recognition, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found no circular dependencies:
screenshot from 2017-08-15 15 05 18

Copy link
Member

Choose a reason for hiding this comment

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

if one of packages in jsk_recognition repository depends on some of the jsk_3rdparty package, then we'll have trouble on releasing, specially when we move to new distro.
for example, when you update jsk_3rdparty and travis need binary version of jsk_recognition, we have to release jsk_recognition before that, I do not think that's going to happen.

Copy link
Contributor Author

@pazeshun pazeshun Aug 16, 2017

Choose a reason for hiding this comment

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

So what should I do?
weight_candidates_refiner in jsk-ros-pkg/jsk_recognition#2193 have to receive weight message from ekew_i_driver

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 it is time to move jsk_recognition_msgs to jsk_common_msgs.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 to @wkentaro

@@ -0,0 +1,77 @@
#!/usr/bin/env python

Copy link
Member

Choose a reason for hiding this comment

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

Add license here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

# unstable
rospy.logdebug('Scale value is unstable')
msg.weight.stable = False

Copy link
Member

Choose a reason for hiding this comment

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

Add else and logdebug to show read data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for confusing condition branches.
Please see latest source code.

stamp = rospy.Time.now()
if len(data) != 17:
rospy.logerr('Serial read timeout')
rospy.signal_shutdown('Serial read timeout')
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this node expect to run as set respawn true, anyway add launch file to explain how to use this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default value of timeout is None, so this shutdown never happens in default.
Anyway, I'll add launch file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

and_scale_ros
=============

ROS Interface for A&D scales
Copy link
Member

Choose a reason for hiding this comment

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

Add link to product page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@k-okada
Copy link
Member

k-okada commented Aug 16, 2017 via email

@pazeshun
Copy link
Contributor Author

  1. Our message file in this package

I'll do this. Then jsk_perception depends on and_scale_ros.

  1. Find necessary package in common_mags or a package commonly used .

Currently, I can't find suitable one.

@pazeshun
Copy link
Contributor Author

  1. Our message file in this package

Now I think this is strange because Weight.msg shouldn't depend on A&D scale itself.
I think #119 (comment) is smartest way.
@k-okada What do you think?

@k-okada
Copy link
Member

k-okada commented Aug 18, 2017

Now I think this is strange because Weight.msg shouldn't depend on A&D scale itself.

This true, but

I think #119 (comment) is smartest way.

I think it is time to move jsk_recognition_msgs to jsk_common_msgs.

will require us to wait new release of jsk_common_msgs when some node in jsk_recognition needs new messages. I'm afraid someone may complain this situation.

@k-okada What do you think?

So for me, best way for now is to add Weight.msg to this layer, even if that does not make sense.

Best idea is to add Weight.msg to common_msgs, but that's will be very hard.
ros/common_msgs#86 (comment)
ros/common_msgs#49
May be create unit_msgs that support all international standard unit (https://en.wikipedia.org/wiki/International_System_of_Units) is one idea.

Another way is to use Wrench.msg
http://docs.ros.org/api/geometry_msgs/html/msg/Wrench.html

@wkentaro
Copy link
Member

wkentaro commented Aug 18, 2017

Now I think this is strange because Weight.msg shouldn't depend on A&D scale itself.
This true, but

I think #119 (comment) is smartest way.

I think it is time to move jsk_recognition_msgs to jsk_common_msgs.
will require us to wait new release of jsk_common_msgs when some node in jsk_recognition needs new messages. I'm afraid someone may complain this situation.

Already jsk_recognition depends on jsk_common, and we need to wait release of packages for 3 month, for example when jsk_pcl_ros requires latest jsk_topic_tools.
Also, it is unlikely that we change construction of msg so frequently, so moving msg to jsk_common_msgs must not be a big problem.

If you really concern about the above problems, possible solution is:

  1. synchronization of the release of different repositories of (jsk_common, jsk_common_msgs, jsk_recognition, jsk_visualization, jsk_3rdparty)
  2. allow master branch to use from source of depending packages and synchronization of the release of different repositories
  3. allow master branch to use shadow-fixed apt target for depending packages

In my opinion, the 3 is the simplest solution.

@pazeshun
Copy link
Contributor Author

I made #120 and jsk-ros-pkg/jsk_recognition#2198 which follow #119 (comment).
We can select
#120 + jsk-ros-pkg/jsk_recognition#2198
or
#119 + jsk-ros-pkg/jsk_recognition#2193 + Another PR to move jsk_recognition_msgs into jsk_common_msgs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants