feat(components): add assignment methods (c++) #224

SprGrf · 2026-01-14T16:25:20Z

Description

Assignments #221

This PR introduces a few basic wrapper functions to add, trigger and publish assignments for a consumer. There are some comments in places where I would definitely appreciate an opinion.

Review guidelines

Estimated Time of Review: 15 minutes

Checklist before merging:

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

github-actions · 2026-01-14T16:25:32Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

SprGrf · 2026-01-14T16:30:13Z

I have read and understood the Contributor License Agreement and hereby sign it

source/modulo_core/include/modulo_core/Assignment.hpp

domire8

Looking at this, I would find it programmatically much easier to treat the assignment like a parameter:

// header
state_representation::ParameterMap assignment_map_;

// source
void add_assignment(name, ParameterType) {
  // checks that you already added correctly
  assignment_map_.set_parameter(std::shared_ptr<ParameterInterface>(name, type));
}

void trigger_assignment(name, value) {
  assignment_map_.set_parameter_value(name, value); // this will already handle all type compatibility for us and raise otherwise
  modulo_interfaces::msg::Assignment message;
  message.node = this->node_base_->get_fully_qualified_name();
  message.assignment = name;
  message.value = modulo_core::translators::write_parameter(assignment_map_.get_parameter(name)).to_parameter_msg().value;
}

No additional Assignment class would be required
@eeberhard

source/modulo_components/include/modulo_components/ComponentInterface.hpp

SprGrf · 2026-01-16T10:31:34Z

No additional Assignment class would be required

It's a fair point, since all it does for now is to check for a valid type. I guess it would be needed if we (plan to) add more functionality, such as checking for the last assigned value and avoid republishing (which has its own problems). Personally, I think it's more future-proof and extensible to keep the extra class.

domire8 · 2026-01-16T10:39:36Z

checking for the last assigned value and avoid republishing (which has its own problems)

I don't think we should have a mechanism to avoid republishing if the value hasn't changed. Thinking of an assignment like an output parameter, it should definitely publish any value if the user wants that. If we think of a classification component that has an assignment representing the detected class, I want it to publish the result every time (potentially at quite high intervals) it classifies an image

eeberhard

I agree with @domire8 that it would certainly be easier to store assignments like a parameter map. That way we get the type and the value, plus more code re-use.

Right now you don't store the last assignment value, which is technically fine because that was not a requirement for the API spec. But if you do store the assignment value, then it would allow us to have get_assignment for components to check for themselves what the last assignment was, and use that for example to conditionally decide not to republish the assignment.

It also makes using the Parameter message (instead of ParameterValue) for the message even simpler - we care about a named and typed value, which the parameter provides.

The analogy of course is set_predicate and get_predicate, where components could check if the predicate has changed or has a current true or false value without needing to store values explicitly or interact with the ROS API.

eeberhard · 2026-01-16T11:15:31Z

source/modulo_components/src/ComponentInterface.cpp

+  if (assignment_name.empty()) {
+    RCLCPP_ERROR(this->node_logging_->get_logger(), "Failed to add assignment: Provide a non empty string as a name.");
+    return;
+  }


We should validate or sanitize the assignment name. The schema allows assignments to have the following pattern: ^[a-zA-Z0-9]([a-zA-Z0-9_.-]?[a-zA-Z0-9])*$

I didn't check the existing code but I suppose this kind of sanitization already exists for signal or parameter names

source/modulo_components/include/modulo_components/ComponentInterface.hpp

domire8 · 2026-01-19T16:40:13Z

source/modulo_components/src/ComponentInterface.cpp

+    const std::string& assignment_name, const state_representation::ParameterType& type) {
+  // Reusing parse_topic_name. Not very elegant. Could add a validate_assignment_name
+  // similar to services but it's a lot of code duplication
+  std::string parsed_name = modulo_utils::parsing::parse_topic_name(assignment_name);


This is fair only if the parsing in there allows the patterns specified in Enricos previous review

As long as it's a valid subset of the permissible assignment names in the schema, it's fine to keep (and can be made more complete and comprehensive later). But if it allows invalid names, then it will be annoying to change later.

domire8 · 2026-01-19T16:41:57Z

source/modulo_components/src/ComponentInterface.cpp

  return true;
 }

+void ComponentInterface::add_assignment(


Another design point, TBD with @eeberhard
Now that I see this, I think that the add_assignment should even be templated, like parameters.

add_assignment<double>("double_assignment")

This is more in line with parameters and templating we use across the components already. We would then add to the assignments_map_ with Parameter<T>(name) instead of ParameterInterface(name, type)

I think I am in favor.

yes, I think that would be very nice for C++, and saves the user from having to deal with ParameterType as an arg.

domire8

Looking good

domire8 · 2026-01-20T05:28:59Z

source/modulo_components/include/modulo_components/ComponentInterface.hpp

+  /**
+  * @brief Get an assignment value.
+  * @tparam T The type of the assignment value   
+  * @param assignment_name The name of the assignment to get


This could raise for two reasons (existence and type), we should catch it and throw an AssignmentException

Yes, I expected this comment and I had a question. The get_parameter will throw a InvalidParameterException if it does not exist, and then the get_parameter_value a InvalidParameterCastException in case of casting to wrong type, or EmptyStateException if the parameter has not been set yet. So I was wondering whether I should catch and rethrow or leave as is.

It's probably best to catch all and throw and AssignmentException to avoid confusing a user with Parameter exceptions

While we could define InvalidAssignmentException and InvalidAssignmentCastException, this will probably have much less usage that get_parameter, so I would be in favor of a single InvalidAssignmentException covering both not existing and not matching type (i.e., catch InvalidParameterException and InvalidParameterCastException and rethrow as AssignmentException).

The case of EmptyStateException should be documented as a possible @throws but not caught IMO since it's already general and informative.

Agreed on the dedicated AssignmentException in general.
Just a thought on the empty state; I think a need might come up to poll the assignment and check if it has been used. Are we going to do this by catching the EmptyStateException, or we should make something dedicated?

I think a need might come up to poll the assignment and check if it has been used

since the component is the only one who gets to set assignments, there are plenty of other ways to check this (using a bool attribute as a flag for example). This is different to parameters whose values can be changed externally, or predicates whose values might be based on some callback function and change arbitrarily during runtime. So throwing an exception if the assignment has not been set is correct behavior in my opinion, while still keeping a distinction between the AssignmentError and EmptyStateException to disambiguate an undeclared assignment from an unused assignment.

source/modulo_components/include/modulo_components/ComponentInterface.hpp

domire8 · 2026-01-20T05:35:21Z

source/modulo_components/include/modulo_components/ComponentInterface.hpp

+  }
+  try {
+    assignment->set_parameter_value<T>(assignment_value);
+  } catch (const state_representation::exceptions::InvalidParameterCastException&){


Are you sure this exception might be thrown? From what I can see, this is only used in get_parameter?

As far as I tested, yes, that's what is thrown. Tests also succeed.

Interesting

source/modulo_components/src/ComponentInterface.cpp

Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>

eeberhard

Looking good!

eeberhard · 2026-01-20T10:52:11Z

source/modulo_components/include/modulo_components/ComponentInterface.hpp

+  /**
+  * @brief Get an assignment value.
+  * @tparam T The type of the assignment value   
+  * @param assignment_name The name of the assignment to get


While we could define InvalidAssignmentException and InvalidAssignmentCastException, this will probably have much less usage that get_parameter, so I would be in favor of a single InvalidAssignmentException covering both not existing and not matching type (i.e., catch InvalidParameterException and InvalidParameterCastException and rethrow as AssignmentException).

The case of EmptyStateException should be documented as a possible @throws but not caught IMO since it's already general and informative.

eeberhard · 2026-01-20T10:53:38Z

source/modulo_components/src/ComponentInterface.cpp

  return true;
 }

+void ComponentInterface::add_assignment(


yes, I think that would be very nice for C++, and saves the user from having to deal with ParameterType as an arg.

eeberhard · 2026-01-20T10:55:04Z

source/modulo_components/src/ComponentInterface.cpp

+    const std::string& assignment_name, const state_representation::ParameterType& type) {
+  // Reusing parse_topic_name. Not very elegant. Could add a validate_assignment_name
+  // similar to services but it's a lot of code duplication
+  std::string parsed_name = modulo_utils::parsing::parse_topic_name(assignment_name);


As long as it's a valid subset of the permissible assignment names in the schema, it's fine to keep (and can be made more complete and comprehensive later). But if it allows invalid names, then it will be annoying to change later.

source/modulo_components/include/modulo_components/ComponentInterface.hpp

source/modulo_components/test/cpp/test_component_interface.cpp

source/modulo_components/include/modulo_components/ComponentInterface.hpp

Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>

domire8

This looks good to me, pending approval of Enrico 👍

bpapaspyros

pending Enrico and a CHANGELOG entry perhaps

eeberhard

This looks great! Sorry for the late final review.

pending Enrico and a CHANGELOG entry perhaps

It's going into a staging branch so as long as we can include the (combined) changelog entry before going into main it will be fine

SprGrf added 3 commits January 12, 2026 19:05

Add first assignments methods

9067bd7

fix: format

ea8fe12

fix: use modulo translator method

6b08424

SprGrf requested review from bpapaspyros, domire8, eeberhard and yrh012 January 14, 2026 16:25

github-actions bot added a commit that referenced this pull request Jan 14, 2026

@SprGrf has signed the CLA in #224

2cc3a1f

bpapaspyros reviewed Jan 14, 2026

View reviewed changes

source/modulo_core/include/modulo_core/Assignment.hpp Outdated Show resolved Hide resolved

domire8 reviewed Jan 15, 2026

View reviewed changes

source/modulo_components/include/modulo_components/ComponentInterface.hpp Outdated Show resolved Hide resolved

SprGrf added 2 commits January 16, 2026 09:59

fix: simplifying assignment message

029d77d

add basic assignment tests

e1b1a2f

eeberhard reviewed Jan 16, 2026

View reviewed changes

SprGrf added 4 commits January 16, 2026 15:26

fix: add name parsing, exception and tests

d289e31

fix: remove redundant include

7d17eab

turn assignments into a parameter map

2bddc3e

fix: rename method and fix tests

68b9355

domire8 reviewed Jan 19, 2026

View reviewed changes

SprGrf added 3 commits January 19, 2026 20:01

add get_assignment

1bceff4

fix: remove assignment class

64848fe

fix: apply suggestions from review

7d12473

domire8 reviewed Jan 20, 2026

View reviewed changes

SprGrf and others added 2 commits January 20, 2026 09:44

fix: apply suggestions from code review

ccfc8e3

Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>

fix: suggestions from code review

d306504

eeberhard reviewed Jan 20, 2026

View reviewed changes

SprGrf added 2 commits January 20, 2026 15:15

add assignment exception

0eb61f2

templated add_assignment method

2d0b676

domire8 reviewed Jan 21, 2026

View reviewed changes

SprGrf and others added 3 commits January 21, 2026 09:44

fix: apply suggestions from code review

da0d20f

Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>

fix: updated error message

f6608ab

fix: review comments on tests

bfacbf7

domire8 approved these changes Jan 21, 2026

View reviewed changes

domire8 changed the title ~~feat(components): add assignment methods~~ feat(components): add assignment methods (c++) Jan 21, 2026

bpapaspyros approved these changes Jan 22, 2026

View reviewed changes

eeberhard approved these changes Jan 27, 2026

View reviewed changes

SprGrf merged commit 5af7f22 into staging/assignments Jan 27, 2026
3 of 4 checks passed

github-actions bot locked and limited conversation to collaborators Jan 27, 2026

domire8 deleted the feat/assignment-methods branch January 27, 2026 09:32

feat(components): add assignment methods (c++) #224

feat(components): add assignment methods (c++) #224

Uh oh!

Conversation

SprGrf commented Jan 14, 2026

Description

Review guidelines

Checklist before merging:

Uh oh!

github-actions bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SprGrf commented Jan 14, 2026

Uh oh!

Uh oh!

domire8 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SprGrf commented Jan 16, 2026

Uh oh!

domire8 commented Jan 16, 2026

Uh oh!

eeberhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domire8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eeberhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2026 •

edited

Loading

domire8 left a comment •

edited

Loading