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

docs: improve signals page #200

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

docs: improve signals page #200

wants to merge 3 commits into from

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Dec 20, 2024

Description

This PR makes a few updates to the signals page in the docs.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

@domire8 domire8 requested a review from bpapaspyros December 20, 2024 09:13
@domire8 domire8 requested a review from eeberhard as a code owner December 20, 2024 09:13
@@ -16,15 +16,16 @@ Often, the message topic and message type is hardcoded within the implementation
difficult to rearrange the network topology of ROS nodes without modifying and recompiling the node implementation
itself.

<!-- TODO: we now support all message types and not just standardized -->
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 don't like this part a lot anymore since we now support any message type so especially the sentence right after this, line 20, is not really correct anymore. At this point, signals are just ROS 2 publishers and subscribers that take their topic name from a parameter

Copy link
Member

Choose a reason for hiding this comment

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

agree

library for Cartesian and joint state classes in C++ and Python.

<!-- TODO: all other state rep types too -->
Copy link
Member Author

Choose a reason for hiding this comment

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

To be done in a followup PR


When developing an AICA component, signals are automatically converted to and from the corresponding data object.
writing it back into a message can involve a fair amount of boilerplate code. When developing an AICA component, signals
are usually automatically converted to and from the corresponding data object.

## Basic signal types
Copy link
Member Author

Choose a reason for hiding this comment

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

Also in a follow up PR, I want to provide a table that explains what these message types are mapped to, e.g. double -> Float64

Copy link
Member

Choose a reason for hiding this comment

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

One of my concerns reading this is that we say:

Additionally, ROS 2 messages are data packets, not data objects. Parsing data from a message, manipulating it and writing it back into a message can involve a fair amount of boilerplate code. When developing an AICA component, signals are usually automatically converted to and from the corresponding data object.

but I don't find it accurate. ROS messages are structs after all. This also feels like we are deviating from ROS language a bit (which we eventually have to but we should make a clear connection first).

I would personally prefer/suggest something more along the lines of

ROS 2 messages are lean data structures with no additional functionality other than storing data. Therefore, parsing data from a message, manipulating it and writing it back into a message can involve a fair amount of boilerplate code. Conversely, the AICA System will typically convert these message structures to and from objects that encapsulate a lot more functionality for manipulating, transforming, and in general operating on their contents.

Copy link
Member

@bpapaspyros bpapaspyros left a comment

Choose a reason for hiding this comment

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

some concerns/comments.

Also, we say

ROS 2 topics are messages ...

which is not accurate. We could lightly edit by saying

ROS 2 messages flow through namespaced topics that publishers and subscribers can refer to ...

or something like that. I mention it in another comment, but I think we are deviating a bit from ROS language but that might not make much sense to ROS users (it might not affect newcomers, but it would still not portray the actual structure)

@@ -16,15 +16,16 @@ Often, the message topic and message type is hardcoded within the implementation
difficult to rearrange the network topology of ROS nodes without modifying and recompiling the node implementation
itself.

<!-- TODO: we now support all message types and not just standardized -->
Copy link
Member

Choose a reason for hiding this comment

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

agree


When developing an AICA component, signals are automatically converted to and from the corresponding data object.
writing it back into a message can involve a fair amount of boilerplate code. When developing an AICA component, signals
are usually automatically converted to and from the corresponding data object.

## Basic signal types
Copy link
Member

Choose a reason for hiding this comment

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

One of my concerns reading this is that we say:

Additionally, ROS 2 messages are data packets, not data objects. Parsing data from a message, manipulating it and writing it back into a message can involve a fair amount of boilerplate code. When developing an AICA component, signals are usually automatically converted to and from the corresponding data object.

but I don't find it accurate. ROS messages are structs after all. This also feels like we are deviating from ROS language a bit (which we eventually have to but we should make a clear connection first).

I would personally prefer/suggest something more along the lines of

ROS 2 messages are lean data structures with no additional functionality other than storing data. Therefore, parsing data from a message, manipulating it and writing it back into a message can involve a fair amount of boilerplate code. Conversely, the AICA System will typically convert these message structures to and from objects that encapsulate a lot more functionality for manipulating, transforming, and in general operating on their contents.


However, any ROS 2 message can be implemented as a signal using the `custom` signal type. As long as the custom type
between two connected components has the same name, the application will be valid.
However, any ROS 2 message type can be implemented as a signal in an AICA application. The syntax for doing this is
Copy link
Member

Choose a reason for hiding this comment

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

We say publishers/subscribers are signals, but here it feels like we now identify them with messages too. I see where we are going with this, but I think it's a bit inconsistent in its use.

@bpapaspyros
Copy link
Member

some concerns/comments.

Also, we say

ROS 2 topics are messages ...

which is not accurate. We could lightly edit by saying

ROS 2 messages flow through namespaced topics that publishers and subscribers can refer to ...

or something like that. I mention it in another comment, but I think we are deviating a bit from ROS language but that might not make much sense to ROS users (it might not affect newcomers, but it would still not portray the actual structure)

ROS uses the following:

Topics are a vital element of the ROS graph that act as a bus for nodes to exchange messages.

Topics are one of the main ways in which data is moved between nodes and therefore between different parts of the system

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.

2 participants