Mikael/remove topic partitions #171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Karsten1987
reviewed
Apr 28, 2018
| @@ -359,10 +359,10 @@ You can read more about partitions in RTI's documentation: | |||
|
|
|||
| Trade-offs (in comparison to using the whole ROS name along with the namespaces): | |||
|
|
|||
| - Splitting the ROS name into "namespace" and "base name", and placing the complete namespace into a single partition seemed un-natural. | |||
| - Splitting the ROS name into "namespace" and "base name", and placing the complete namespace into a field designed for another purpose seemed incorrect. | |||
There was a problem hiding this comment.
So I actually still think that we have to justify this statement more precisely. So one concrete reason why putting the namespace in a partition is incorrect is that the tuple of (partition, topic, type) has to be unique amongst a DDS environment. Fast-RTPS does not enforce this, however RTI does, hence the github issue below.
| - In general partitions are recommended to be used as a spare, but using partitions for all ROS names suggested otherwise. | ||
| - Major concern was reported in this [issue](https://github.com/ros2/rmw_connext/issues/234), where having two topics with same base name, although different namespace and different ypes caused problem. For example: topicA is `/camera/data` of type `Image` and topicB is `/imu/data` of type `Imu`. The base names for both topicA and topicB is `data`, generated errors as described in the [issue](https://github.com/ros2/rmw_connext/issues/234). | ||
| - Newer standards such as XRCE DDS might not have partitions at all. | ||
| - Major concern was reported in this [issue](https://github.com/ros2/rmw_connext/issues/234), where having two topics with same base name, although different namespace and different types caused problem. For example: topicA is `/camera/data` of type `Image` and topicB is `/imu/data` of type `Imu`. The base names for both topicA and topicB is `data`, generated errors as described in the [issue](https://github.com/ros2/rmw_connext/issues/234). |
There was a problem hiding this comment.
"... with the same base name ... caused a problem."
| @@ -202,7 +202,7 @@ The ROS topic and service name constraints allow more types of characters than t | |||
| These must be substituted or otherwise removed during the process of converting the topic or service name to DDS concepts. | |||
| Since ROS 2 topic and service names are expanded to fully qualified names, any balanced bracket (`{}`) substitutions and tildes (`~`) will have been expanded. | |||
| Additionally any URL related syntax, e.g. the `rostopic://` prefix, will be removed once parsed. | |||
| Previously forward slashes (`/`) were disallowed in DDS topic names, now the restriction has been lifted(see [issue](https://issues.omg.org/issues/lists/dds-rtf5#issue-42236) on omg.org) and therefore the ROS topic names are first prefixed with ROS Specific Namespace prefix (described below) are then mapped directly into DDS topic names. | |||
| Previously forward slashes (`/`) were disallowed in DDS topic names, now the restriction has been lifted (see [issue](https://issues.omg.org/issues/lists/dds-rtf5#issue-42236) on omg.org) and therefore the ROS topic names are first prefixed with ROS Specific Namespace prefix (described below) are then mapped directly into DDS topic names. | |||
There was a problem hiding this comment.
i'd remove now in the sentence.
|
feel free to merge if you want. |
|
Please feel free to edit directly on this branch 👍, I definitely don't have has much context as you |
|
Chatting with @Karsten1987 we concluded we'll merge this as is and iterate on #170 from there |
Karsten1987
pushed a commit
that referenced
this pull request
Jun 7, 2018
* removal of topic partitions, inital draft * Updated alternative strategy of using partitions * fixed suggestions, added limits for service_names * Mikael/remove topic partitions (#171) * unnecessary Now * XRCE DDS -> DDS-XRCE + link * try to make the sentence sound more naturel * attempt to clarify the 'un-natural' character of using partition * missing whitespace * avoid having same section and subsection title * highlight rti connext limitations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
some minor fixups and wording suggestions.
I didn't address the pending comment on the original PR about providing example of mapping from ROS Service name to DDS Request/Reply topic names