feat!: add support for generic outputs #141

bpapaspyros · 2024-09-26T13:40:01Z

Description

The motivation for this PR is to provide the necessary "plumbing" that allows for adding generic (but supported by ROS) output types.

add_output for generic message types #133

This PR solves the issue by:

moving to C++20 as our main C++ standard
introducing C++20 concepts to allow for compile-time decisions regarding the message and data types
restructuring the PublisherHandler and PublisherInterface
introducing compile-time condition logic to re-route the the execution logic according to message/data types, including (but not limited):
- PublisherHandler and PublisherInterface
- MessagePair and MessagePairInterface

Review guidelines

Estimated Time of Review: 20 minutes

Checklist before merging:

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

domire8

Looks nice! I'm curious if we could

make this PR target a different staging branch
only change C++ version, and modulo core without changing anything in modulo_components

If we can do that (basically enable generic publishing and message pairs without changing anything in the interface) then we should go for it and close this very first stage

source/modulo_components/package.xml

bpapaspyros · 2024-09-29T04:22:40Z

source/modulo_core/include/modulo_core/concepts.hpp

+#include <state_representation/space/cartesian/CartesianState.hpp>
+#include <state_representation/space/joint/JointState.hpp>
+
+namespace modulo_core::concepts {


@domire8 @eeberhard We should agree on concept names/naming convention before merging as well.

This is not really exposed to the user and derived components right? Just to know the importance of naming

Well, it is included in Component.hpp so it is exposed and can be used in that respect, yes. But it is not something the user needs to interact with to use the library (they will not have to provide them anywhere).

source/modulo_core/include/modulo_core/communication/MessagePair.hpp

domire8 · 2024-09-29T16:26:04Z

Please target a copy of main branch here, not main directly. Will be better to stage everything and fast forward to main when we have the complete feature in cpp and python ready and tested

domire8

Very nice, some minor comments

domire8 · 2024-09-30T05:40:51Z

source/modulo_core/include/modulo_core/communication/MessagePair.hpp

@@ -52,6 +64,48 @@ class MessagePair : public MessagePairInterface {
   */
  void set_data(const std::shared_ptr<DataT>& data);

+protected:


Shouldn't this be private?

yes yes, you are right.

source/modulo_core/include/modulo_core/communication/MessagePair.hpp

domire8 · 2024-09-30T05:48:17Z

source/modulo_core/include/modulo_core/communication/PublisherHandler.hpp

-   * @throws modulo_core::exceptions::NullPointerException if the publisher pointer is null
-   */
-  void on_activate();
+     * @brief Activate ROS publisher.


I liked the previous docstrings better, or we just do @copydoc from the parent class

domire8 · 2024-09-30T05:49:32Z

source/modulo_core/include/modulo_core/communication/PublisherInterface.hpp

-  PublisherType type_;                                ///< The type of the publisher interface
+  PublisherType type_;///< The type of the publisher interface
+
+protected:


Can we have public, protected, private in that order please

domire8 · 2024-09-30T05:55:18Z

source/modulo_core/include/modulo_core/communication/PublisherHandler.hpp

+  if (this->message_pair_ == nullptr) {
+    throw exceptions::NullPointerException("Message pair is not set, cannot deduce message type");
+  }
+  if (this->message_pair_->get_type() == MessageType::CUSTOM_MESSAGE) {


I don't get this block, why do we need it?

Ok, indeed this is a bit hacky, but I don't have a cleaner solution yet. We can give it a moment of thought later on in this PR or I can leave a TODO to refactor.

What it does is:

The problem starts from the fact that for generic types we need to have MsgT or explicitly know it. This is ok for translated types, hence PublisherInterface::publish can be used (else block).

For generic types we need to

override the publish in PublisherHandler otherwise we can't deduce MsgT

and then explicitly call publish(MsgT&) which is compatible for all types

Why the constexpr block

Since both generic and translated types will go through this publish function, it means that the compiler needs to resolve write<MsgT, MsgT> for all MessageTypes even though there is a runtime condition for MessageType::CUSTOM_MESSAGE.

The solution is to introduce a secondary compile-time condition to hide this block when translated types are used.

I currently don't have a better alternative to this.

domire8 · 2024-09-30T06:03:42Z

source/modulo_core/include/modulo_core/concepts.hpp

+    || std::same_as<DataT, std_msgs::msg::Float64> || std::same_as<DataT, std_msgs::msg::Float64MultiArray>
+    || std::same_as<DataT, std_msgs::msg::Int32> || std::same_as<DataT, std_msgs::msg::String>;
+
+template<typename DataT>


I guess the TranslatedOrEncodedDataT and NonTranslatedDataT will be required for the integration in the components?

Also, why not add || std::same_as<DataT, EncodedState> directly to TranslatedDataT?

I will now even go as far as saying that the unused concepts could be removed from this PR such that it doesn't go out of scope

I do need to type check separately, but I do agree that this is a bit out of scope with the new form of this PR. I will remove it now and then we can discuss again in the corresponding PR

domire8 · 2024-09-30T06:04:14Z

source/modulo_core/include/modulo_core/concepts.hpp

+    || std::same_as<DataT, bool> || std::same_as<DataT, std::string> || std::same_as<DataT, std::vector<double>>;
+
+template<typename DataT>
+concept CoreDataT = StateRepresentationDataT<DataT> || CorePrimitivesT<DataT>;


Suggested change

concept CoreDataT = StateRepresentationDataT<DataT> || CorePrimitivesT<DataT>;

concept CoreDataT = std::derived_from<DataT, state_representation::State> || CorePrimitivesT<DataT>;

Could we get rid of the StateRepresentationDataT like that or will it be required somewhere else?

It is honestly never used directly (apart from what you see). So yes, we can cut down. I think NonTranslatedDataT is also the same case. I'll clean this up

domire8 · 2024-09-30T06:13:54Z

source/modulo_core/include/modulo_core/concepts.hpp

+
+// Translation concepts
+
+template<typename DataT>


Ah also, now I get what was confusing me a bit here. In the translation concepts we should use MsgT instead of DataT and TranslatedMsgT instead of TranslatedDataT (and so on) no?

I am actually not sure here. I use them to both type check DataT and MsgT. Perhaps we should drop Data/Msg altogether and stay generic by saying TranslatedTypeT and so on. What do you think?

source/modulo_core/src/communication/PublisherInterface.cpp

bpapaspyros requested review from eeberhard and domire8 September 26, 2024 13:40

bpapaspyros self-assigned this Sep 26, 2024

bpapaspyros force-pushed the feat/generic_msgs branch 2 times, most recently from a7eaf5f to 448a862 Compare September 27, 2024 12:37

bpapaspyros and others added 2 commits September 28, 2024 15:36

feat: support generic add_output

e01d256

feat: support generic publishers

a9328b1

bpapaspyros force-pushed the feat/generic_msgs branch from c7026c4 to a9328b1 Compare September 28, 2024 13:56

domire8 reviewed Sep 28, 2024

View reviewed changes

source/modulo_components/package.xml Outdated Show resolved Hide resolved

bpapaspyros added 2 commits September 29, 2024 06:01

fix: relax lifecycle template conditions for future proofing

564fd37

revert: restore component changes (excluding C++20 update)

0682ace

bpapaspyros changed the title ~~feat!: add support for generic inputs/outputs~~ feat!: add support for generic outputs Sep 29, 2024

docs: update CHANGELOG.md

5b09b63

bpapaspyros commented Sep 29, 2024

View reviewed changes

source/modulo_core/include/modulo_core/communication/MessagePair.hpp Outdated Show resolved Hide resolved

This was referenced Sep 29, 2024

feat: expose generic outputs to components #143

Draft

feat: add support for generic subscriptions #144

Draft

bpapaspyros marked this pull request as ready for review September 29, 2024 12:09

bpapaspyros changed the base branch from main to feat/generic_pubsub_dev September 29, 2024 20:31

docs: add docstrings for new MessagePair functions

f40385a

domire8 reviewed Sep 30, 2024

View reviewed changes

bpapaspyros added 6 commits September 30, 2024 08:57

fix: reduce visibility to new write methods in MessagePair

1eef7c8

docs: update docstrings in PublisherHandler

a44ebb8

fix: mark message_pair_ as private in PublisherHandler

0b41f93

refactor: tidy up PublisherInterface declarations

6bd7adf

refactor: update CoreException messages in PublisherInterface

4ab5ec4

refactor: clean up concept definitions

297fd1a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add support for generic outputs #141

feat!: add support for generic outputs #141

bpapaspyros commented Sep 26, 2024 •

edited

Loading

domire8 left a comment

bpapaspyros Sep 29, 2024

domire8 Sep 29, 2024

bpapaspyros Sep 29, 2024

domire8 commented Sep 29, 2024

domire8 left a comment

domire8 Sep 30, 2024

bpapaspyros Sep 30, 2024

domire8 Sep 30, 2024

domire8 Sep 30, 2024

domire8 Sep 30, 2024

bpapaspyros Sep 30, 2024

domire8 Sep 30, 2024

domire8 Sep 30, 2024

bpapaspyros Sep 30, 2024

domire8 Sep 30, 2024

bpapaspyros Sep 30, 2024

domire8 Sep 30, 2024

bpapaspyros Sep 30, 2024

	concept CoreDataT = StateRepresentationDataT<DataT> \|\| CorePrimitivesT<DataT>;
	concept CoreDataT = std::derived_from<DataT, state_representation::State> \|\| CorePrimitivesT<DataT>;

feat!: add support for generic outputs #141

Are you sure you want to change the base?

feat!: add support for generic outputs #141

Conversation

bpapaspyros commented Sep 26, 2024 • edited Loading

Description

Review guidelines

Checklist before merging:

domire8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domire8 commented Sep 29, 2024

domire8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpapaspyros commented Sep 26, 2024 •

edited

Loading