Configurable QoS for Publishers and Service Clients #3

gavanderhoorn · 2022-09-13T11:08:55Z

This is for #1.

A bit of a WIP, as I haven't tested it (yet) and I've not ran clang-format.

I've tried to follow the style of bab270d.

I'll add some in-line comments to aspects I'm not sure about.

Please review and let me know whether this would be acceptable.

gavanderhoorn

Just some comments on parts I was unsure about.

gavanderhoorn · 2022-09-13T11:09:44Z

include/qml_ros2_plugin/publisher.hpp

@@ -51,6 +58,8 @@ class Publisher : public QObjectRos2

  quint32 queueSize() const;

+  void setQueueSize( quint32 value );


I've kept the name QueueSize here even though I believe it should be HistoryDepth?

Yes, I've kept the name queueSize for backwards compatibility. Not sure if it makes sense to have a cut there and use the new terminology.
It's not like these libraries are out for that long so making such a change is still possible.
However, the publisher was not really intended to be modified after creation.
The only reason Subscription supports modification is because it can be instantiated as a QML item.
The publisher receives all required information in the constructor to ensure that it will be in a consistent state from the start.
Not sure what the use case would be to change such properties dynamically.

Yes, I've kept the name queueSize for backwards compatibility. Not sure if it makes sense to have a cut there and use the new terminology.
It's not like these libraries are out for that long so making such a change is still possible.

so change to HistoryDepth, or keep QueueSize?

However, the publisher was not really intended to be modified after creation.
The only reason Subscription supports modification is because it can be instantiated as a QML item.
The publisher receives all required information in the constructor to ensure that it will be in a consistent state from the start.
Not sure what the use case would be to change such properties dynamically.

things will only get less complex if it can't be changed, so I'd be in favour of removing the setter(s).

so change to HistoryDepth, or keep QueueSize?

or remove it completely and add properties to QoS to get these values.
However, QoS is currently a Q_GADGET which means it does not support signals so no changed notify signals.
I don't think they are necessary for the QoS settings, though, and don't justify dealing with the complications that would arise when changing QoS to Q_OBJECT.

As I'm not a Qt aficionado I'm going to need some guidance in this case.

If you'd rather have properties in QoS, that would be fine by me.

Currently, QoS is a Q_GADGET class. The difference to a standard QObject based class is that we don't have all that QObject overhead at the cost of not being connected to the signal mechanism.
Hence, we can have properties (I think) but they can't notify of changes since the Q_GADGET can't have signals.
I don't think we need them though and switching to QObject would make building the QoS more complicated since the gadget is passed by value whereas QObject is always passed by pointer and I'm currently using the value mechanism to enable that you can start configuring QoS use them for one publisher and add some more settings for the second one without these changes being reflected on the first publisher. I think that behavior is closer to the cpp version.

So, conclusion: keep QoS a Q_GADGET class, but add (more) properties to it?

Yes, I think adding properties for the available options would make most sense.
Since humble there are getters for properties such as the history but they have the same name as the setter.
I don't think that will work with QML. Will have to get around that.

include/qml_ros2_plugin/ros2.hpp

include/qml_ros2_plugin/service_client.hpp

src/publisher.cpp

src/ros2.cpp

gavanderhoorn · 2022-09-13T11:15:18Z

src/service_client.cpp

@@ -40,7 +40,7 @@ void ServiceClient::onRos2Initialized()
  try {
    rclcpp::Node &node = *Ros2Qml::getInstance().node();
    client_ =
-        babel_fish_.create_service_client( node, name_.toStdString(), service_type_.toStdString() );
+        babel_fish_.create_service_client( node, name_.toStdString(), service_type_.toStdString(), qos_.getQoS().get_rmw_qos_profile() );


I'm not entirely sure why create_service_client(..) needs a rmw_qos_profile_t, while create_subscription(..) et al. accept rclcpp::QoS?

Is this something that should change in ros2_babel_fish? Or is it as-intended?

Mainly because the ROS2 service client required the C struct in its constructor but I guess it makes sense to change that in ros2_babel_fish to make it more consistent.
I will change it but keep the old function for a while with a deprecation message.

gavanderhoorn · 2022-09-13T11:18:49Z

I can extend ImageTransportSubscription in a similar way once we get the style-issues sorted out.

include/qml_ros2_plugin/ros2.hpp

src/publisher.cpp

gavanderhoorn · 2022-09-13T13:31:30Z

Addressed most of the comments I believe. Force-pushed the update.

gavanderhoorn · 2022-09-13T14:21:17Z

I can extend ImageTransportSubscription in a similar way once we get the style-issues sorted out.

hm. If I understand the code correctly here, we should now also check for compatible QoS to figure out whether a subscription already exists?

StefanFabian · 2022-09-14T07:39:05Z

hm. If I understand the code correctly here, we should now also check for compatible QoS to figure out whether a subscription already exists?

Hm, you're right, it's a bit more complicated for image transport and I think this would create some error potential if you could accidentally create multiple subscribers differing only in their QoS which would increase the bandwidth requirements.
Maybe it makes more sense to have a fixed QoS there, e.g., a best effort, volatile subscription which would be compatible with every publishing mode.

gavanderhoorn · 2022-09-14T10:28:10Z

Maybe it makes more sense to have a fixed QoS there, e.g., a best effort, volatile subscription which would be compatible with every publishing mode.

yeah, that might work.

I'm having a bit of difficulty pin-pointing where the QoS is configured right now though in the ImageTransportSubscription. Could you point it out?

StefanFabian · 2022-09-14T15:43:49Z

I'm having a bit of difficulty pin-pointing where the QoS is configured right now though in the ImageTransportSubscription. Could you point it out?

I think I will just do that outside of this PR since it doesn't really have anything to do with configurable QoS.

gavanderhoorn changed the title ~~Configurable QoS for Publisher~~ Configurable QoS for Publishers and Service Clients Sep 13, 2022

gavanderhoorn commented Sep 13, 2022

View reviewed changes

gavanderhoorn mentioned this pull request Sep 13, 2022

Support for specifying QoS per subscriber/publisher/etc? #1

Open

StefanFabian reviewed Sep 13, 2022

View reviewed changes

include/qml_ros2_plugin/ros2.hpp Outdated Show resolved Hide resolved

src/publisher.cpp Outdated Show resolved Hide resolved

gavanderhoorn added 2 commits September 13, 2022 15:28

Configurable QoS for Publisher

80cb62b

Configurable QoS for service clients

10ecaac

gavanderhoorn force-pushed the more_qos branch from f9ce190 to 10ecaac Compare September 13, 2022 13:29

gavanderhoorn mentioned this pull request May 11, 2023

Fixed missing C interfaces to obtain service and action type support. ros2/rosidl_typesupport#114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable QoS for Publishers and Service Clients #3

Configurable QoS for Publishers and Service Clients #3

gavanderhoorn commented Sep 13, 2022

gavanderhoorn left a comment

gavanderhoorn Sep 13, 2022

StefanFabian Sep 13, 2022

gavanderhoorn Sep 13, 2022

StefanFabian Sep 13, 2022

gavanderhoorn Sep 13, 2022

StefanFabian Sep 14, 2022

gavanderhoorn Sep 14, 2022

StefanFabian Sep 14, 2022

gavanderhoorn Sep 13, 2022

gavanderhoorn Sep 13, 2022

StefanFabian Sep 20, 2022

gavanderhoorn commented Sep 13, 2022

gavanderhoorn commented Sep 13, 2022

gavanderhoorn commented Sep 13, 2022

StefanFabian commented Sep 14, 2022

gavanderhoorn commented Sep 14, 2022

StefanFabian commented Sep 14, 2022

		@@ -51,6 +58,8 @@ class Publisher : public QObjectRos2

		quint32 queueSize() const;

		void setQueueSize( quint32 value );

Configurable QoS for Publishers and Service Clients #3

Are you sure you want to change the base?

Configurable QoS for Publishers and Service Clients #3

Conversation

gavanderhoorn commented Sep 13, 2022

gavanderhoorn 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

gavanderhoorn commented Sep 13, 2022

gavanderhoorn commented Sep 13, 2022

gavanderhoorn commented Sep 13, 2022

StefanFabian commented Sep 14, 2022

gavanderhoorn commented Sep 14, 2022

StefanFabian commented Sep 14, 2022