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 support for generic message types in Components and LifecycleComponents #133

Closed
eeberhard opened this issue Jul 17, 2024 · 3 comments · Fixed by #145
Closed

add support for generic message types in Components and LifecycleComponents #133

eeberhard opened this issue Jul 17, 2024 · 3 comments · Fixed by #145
Assignees

Comments

@eeberhard
Copy link
Member

Modulo inputs and outputs are wrappers around ROS 2 subscribers and publishers, respectively. The first point is that they declare a _topic parameter for deciding the signal topic dynamically. The second point is that they can also bind the respective message data to an object in memory (i.e. a shared_ptr that is automatically updated from an input subscription or is automatically periodically written to an output). This second point only works for selected message types, as we need to know how to convert a message to and from the respective object.

This issue is about providing the provide convenience of the first point (which is to declare and associate a _topic parameter) to an add_output function for any generic message type.

For inputs, we already have a generic way of doing this, by binding a callback function which simply takes the message itself as an argument:

  template<typename MsgT>
  void add_input(
      const std::string& signal_name, const std::function<void(const std::shared_ptr<MsgT>)>& callback,
      const std::string& default_topic = "", bool fixed_topic = false
  );
    def add_input(self, signal_name: str, subscription: Union[str, Callable], message_type: MsgT, default_topic="",
                  fixed_topic=False, user_callback: Callable = None):

However, there is no such construct for outputs. The current definitions for add_output only support the following signatures:

  template<typename DataT>
  void add_output(
      const std::string& signal_name, const std::shared_ptr<DataT>& data, const std::string& default_topic = "",
      bool fixed_topic = false, bool publish_on_step = true
  );
    def add_output(self, signal_name: str, data: str, message_type: MsgT,
                   clproto_message_type=clproto.MessageType.UNKNOWN_MESSAGE, default_topic="", fixed_topic=False,
                   publish_on_step=True):

To use generic message types, the alternative is to first manually add_parameter and then create_publisher, but this is error prone and more complex for lifecycle publishers. Instead, one could consider a new function signature which is tempted by the message type:

  template<typename MsgT>
  void add_output(
      const std::string& signal_name, const std::string& default_topic = "", bool fixed_topic = false,
      bool publish_on_step = true
  );

Under the hood, this might perform the same add_parameter and create_publisher steps. However, the user would still need to manually publish the message with some publish_output<MsgT>(string signal_name, MsgT message) helper function, since there is no object representation of the data to bind to and automatically publish.

We might then consider holding a shared pointer of the message as the object to publish from, allowing modification of the message in memory with automatic publishing.

  template<typename MsgT>
  void add_output(
      const std::string& signal_name, const std::shared_ptr<MsgT>& msg, const std::string& default_topic = "",
      bool fixed_topic = false, bool publish_on_step = true
  );

From the outside, this then has the same signature as add_output<DataT>(...), but it would require an internal refactor and checks to see if the data type is supported by the message translators or otherwise treat is as a direct message.

@domire8 domire8 assigned domire8 and bpapaspyros and unassigned domire8 Aug 21, 2024
@domire8
Copy link
Member

domire8 commented Aug 22, 2024

On second thought, not everything in this issue description is correct.

  1. For inputs, we already have a generic way of doing this: Not correct, we only have that for message types that are supported by modulo_core::SubscriptionInterface since the internal map that holds the subscriptions cannot hold any ros subscription.
  2. We might then consider holding a shared pointer of the message as the object to publish from, allowing modification of the message in memory with automatic publishing.

  template<typename MsgT>
  void add_output(
      const std::string& signal_name, const std::shared_ptr<MsgT>& msg, const std::string& default_topic = "",
      bool fixed_topic = false, bool publish_on_step = true
  );

From the outside, this then has the same signature as add_output(...),

This also confuses me since the template of this function is MsgT which is usually EncodedState and that is not the same as DataT which is usually a shared pointer of a state representation object. We can't have the user deal with the translation between encoded state and state representation, otherwise we loose the power of modulo

So the main issue here is that there is no way to hold a map of subscriptions and publishers without declaring them first or have a set of supported types.

@domire8
Copy link
Member

domire8 commented Aug 22, 2024

It is currently all about the MessagePair. We support inputs and outputs for any supported message pair type since in both add_output and add_input we create a message pair. So we have equality between what we support for inputs and outputs

@bpapaspyros bpapaspyros changed the title add_output for generic message types add support for generic message types in Components and LifecycleComponents Oct 3, 2024
@bpapaspyros
Copy link
Member

After the recent developments (see PRs that referenced this issue), and the branch where all these changes now live (#145), I updated the title of this issue to reflect the extent of the changes that were required. I am also briefly restating the problem below.

Supporting generic inputs and outputs in Python is somewhat more straightforward since variables can be passed around without a lot of type constraints. For the sake of keeping this relatively short, I am skipping the Python-related development summary, but it almost mirrors the C++ one.

For C++ supporting generic inputs and outputs means that types need to be inferred at compile-time and be filtered/rerouted according to expected behaviour and current API implementation. In an effort to do this without breaking existing interface, that requires C++20 features that allow for a fine level of type deduction at compile time.

So, add_output and add_input can be easily adjusted to not change the interface, and then it quickly indeed becomes an issue of the MessagePair and Publisher/SubscriberHandlers and how those can direct compilation or execution flow towards the built-in or generic types.

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 a pull request may close this issue.

3 participants