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

Multiple analog sensor ROS publishers #2172

Merged
merged 4 commits into from
Jan 14, 2020

Conversation

randaz81
Copy link
Member

@randaz81 randaz81 commented Jan 8, 2020

Added the following new devices: IMURosPublisher , MagfieldRosPublisher , PoseRosPublisher , TemperatureRosPublisher , WrenchRosPublisher

@randaz81 randaz81 added PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Component: Devices labels Jan 8, 2020
@randaz81 randaz81 requested a review from traversaro January 8, 2020 19:11
@randaz81 randaz81 self-assigned this Jan 8, 2020
@randaz81
Copy link
Member Author

randaz81 commented Jan 8, 2020

[WIP] some computations (e.g. deg2rad etc.) still need to be checked!

@Nicogene Nicogene changed the title Multiple analog sensor ROS publishers [WIP] Multiple analog sensor ROS publishers Jan 9, 2020
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Some comments.

Small comment on the naming, feel free to ignore: now the relation between the ROS message type name and the YARP publisher does not follow a clear rule, could it make sense to always name the device as <MessageType>RosPublisher for uniformity, by changing the names of the following devices:

  • PoseRosPublisher --> PoseStampedRosPublisher
  • WrenchRosPublisher --> WrenchStampedRosPublisher
  • MagfieldRosPublisher --> MagneticFieldRosPublisher
  • ImuRosPublisher --> IMURosPublisher

@traversaro
Copy link
Member

Just to understand, using more then one of these devices in the same yarprobotinterface can trigger robotology/yarp-ros#8 ?

@randaz81
Copy link
Member Author

Just to understand, using more then one of these devices in the same yarprobotinterface can trigger robotology/yarp-ros#8 ?

Generally speaking no, this issue refers to a specific case in which different nodes, belonging to the same process, publish data on the same topic. So, if the user chooses a different topic name for each publisher, everything is safe.

Additionally, issue robotology/yarp-ros#8 is pretty old, many things changed and many parts of the ports/publisher core have been refactored. We need to check if this elusive bug has been fixed or not.

@traversaro
Copy link
Member

Just to understand, using more then one of these devices in the same yarprobotinterface can trigger robotology/yarp-ros#8 ?

Generally speaking no, this issue refers to a specific case in which different nodes, belonging to the same process, publish data on the same topic. So, if the user chooses a different topic name for each publisher, everything is safe.

Ok, perfect then.

@randaz81
Copy link
Member Author

Some comments.

Small comment on the naming, feel free to ignore: now the relation between the ROS message type name and the YARP publisher does not follow a clear rule, could it make sense to always name the device as <MessageType>RosPublisher for uniformity, by changing the names of the following devices:

  • PoseRosPublisher --> PoseStampedRosPublisher
  • WrenchRosPublisher --> WrenchStampedRosPublisher
  • MagfieldRosPublisher --> MagneticFieldRosPublisher
  • ImuRosPublisher --> IMURosPublisher

Good suggestion!

@randaz81 randaz81 force-pushed the multiple_ROS_publishers branch from 78f93e1 to 400bbca Compare January 14, 2020 10:59
@randaz81 randaz81 requested a review from traversaro January 14, 2020 10:59
@randaz81 randaz81 changed the title [WIP] Multiple analog sensor ROS publishers Multiple analog sensor ROS publishers Jan 14, 2020
@randaz81
Copy link
Member Author

@traversaro @drdanz @Nicogene the PR is ready for merge (hopefully)

@randaz81 randaz81 force-pushed the multiple_ROS_publishers branch from 400bbca to 3ae20da Compare January 14, 2020 11:05
@randaz81 randaz81 merged commit 99827ea into robotology:master Jan 14, 2020
@randaz81 randaz81 deleted the multiple_ROS_publishers branch January 14, 2020 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Devices PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Resolution: Merged Target: YARP v3.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants