Update qos profile -- take 2 by ivanpauno · Pull Request #25 · osrf/ros2_java

ivanpauno · 2020-09-01T20:27:33Z

Follow up of #20.

Depends on Add convenient error handling macros #30.

Improve QoSProfile constructors (matching better with rclcpp and rclpy).
Add docs to QoSProfile class.
Add UNKNOWN enum value to policies.
Add fromRCL method to QoSProfile.
Use fromRCL to create the built-in qos profiles.

Some of this changes are required by #26, some are unrelated improvements.

ivanpauno · 2020-09-08T14:21:37Z

Converting to draft until #30 is merged.

* Add documentation to QoSProfile class * Add fromRCL method to QoSProfile * Use fromRCL to create the built-in qos profiles. Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

jacobperron · 2020-09-08T21:45:11Z

+ default:
+ std::ostringstream oss{"unknown history policy value: ", std::ios_base::ate};
+ oss << qos.history;
+ rcljava_throw_exception(env, "java/lang/IllegalStateException", oss.str());


It looks like we're only outputting the invalid value for the history policy and not in cases of invalid values for other QoS settings. I would update this (or the other default cases) to be consistent.

Yeah, this is a leftover. I used that while debugging.

jacobperron · 2020-09-08T21:59:20Z

+ * QoSProfile.sensorData(); // get predefined sensor data qos profile
+ */
 public class QoSProfile {
 // TODO(ivanpauno): Update all qos policies in a way that the objects are created from RCL,


I think this is done in this PR, right?

There were two TODOs:
Avoiding to hard code the default profiles, that is being fixed here.
Avoiding to hard code the policies enumeration numbers, that's not being fixed here.
This comment was referring to the second, if it's not clear I can try to rephrase it.

Do you think it's worth doing the second point at all? I don't see an issue using separate enum values within rcljava, though I found a similar TODO in rclpy.

Do you think it's worth doing the second point at all? I don't see an issue using separate enum values within rcljava, though I found a similar TODO in rclpy.
The issue is that you have to keep the numbers synced.
If the numbers get out of sync it might be a pretty hard bug to find, causing unexpected behavior to the user.

jacobperron · 2020-09-08T22:26:51Z

+ QoSProfile qos = new QoSProfile();
+ qos.nativeFromRCL(qos.nativeGetHandleFromName("default"));


Is it possible to make nativeFromRCL a static method? It might clean up the implementation a bit, but I'm not sure.
Then this can be rewritten as
returnnativeFromRCL(nativeGetHandleFromName("default"));
Further, if there's no other intended use for nativeGetHandleFromName, we could just provide a single native method instead, e.g.
returnnativeFromRCL("default");

I'm using the nativeFromRCL that takes a handle in #26:
ros2_java/rcljava/src/main/cpp/org_ros2_rcljava_graph_EndpointInfo.cpp
Lines 102 to 104 in 7bd6cc4
jmethodID qos_from_rcl_mid = env->GetMethodID(qos_clazz, "nativeFromRCL", "(J)V");
env->CallObjectMethod(jqos, qos_from_rcl_mid, &p->qos_profile);
env->SetObjectField(self, qos_fid, jqos);
The difference is that in this way nativeFromRCL can be called in an already constructed QoS object.
Maybe that's not quite useful, but I originally thought that I was going to use that.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2020-09-09T17:51:33Z

Going in, thanks for reviewing @jacobperron !

* Improve QoSProfile constructors * Add documentation to QoSProfile class * Add fromRCL method to QoSProfile * Use fromRCL to create the built-in qos profiles. Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno added the enhancement New feature or request label Sep 1, 2020

ivanpauno requested a review from jacobperron September 1, 2020 20:27

ivanpauno self-assigned this Sep 1, 2020

ivanpauno mentioned this pull request Sep 1, 2020
Add getPublishersInfo method to Node #26
Merged

ivanpauno force-pushed the ivanpauno/update-qos-profile-take-2 branch from 6a7ef48 to 8201da2Compare September 3, 2020 18:20

ivanpauno changed the base branch from foxy-devel to ivanpauno/add-convenient-error-handling-macrosSeptember 3, 2020 18:20

ivanpauno mentioned this pull request Sep 3, 2020
Add convenient error handling macros #30
Merged

ivanpauno marked this pull request as draft September 8, 2020 14:21

ivanpauno force-pushed the ivanpauno/update-qos-profile-take-2 branch from 8201da2 to 2648f10Compare September 8, 2020 21:13

Base automatically changed from ivanpauno/add-convenient-error-handling-macros to galactic-develSeptember 8, 2020 21:22

ivanpauno added 2 commits September 8, 2020 18:24

* Improve QoSProfile constructors
092f6bf
* Add documentation to QoSProfile class * Add fromRCL method to QoSProfile * Use fromRCL to create the built-in qos profiles. Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

update to new macro name
0a9fb5f
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno force-pushed the ivanpauno/update-qos-profile-take-2 branch from 2648f10 to 0a9fb5fCompare September 8, 2020 21:26

ivanpauno marked this pull request as ready for review September 8, 2020 21:28

jacobperron reviewed Sep 8, 2020
View reviewed changes

ivanpaunoand others added 9 commits September 9, 2020 11:18

Fix nit
faffe6e
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

Fix nit
b60d1fd
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

Fix nit
5de779f
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

Fix nit
46b025e
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

more nit fixing
18a7fb4
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

nit
0a4c3fb
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

Improve docblock
77a4ec2
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

Make error messages consistent
0a3f062
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

remove unneaded include
4e3e515
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno requested a review from jacobperron September 9, 2020 14:47

jacobperron approved these changes Sep 9, 2020
View reviewed changes

ivanpauno merged commit a6b5a0b into galactic-develSep 9, 2020

ivanpauno deleted the ivanpauno/update-qos-profile-take-2 branch September 9, 2020 17:51

ivanpauno mentioned this pull request Jun 27, 2022
Port many graph introspection features from osrf fork ros2-java/ros2_java#221
Merged

		QoSProfile qos = new QoSProfile();
		qos.nativeFromRCL(qos.nativeGetHandleFromName("default"));

	jmethodID qos_from_rcl_mid = env->GetMethodID(qos_clazz, "nativeFromRCL", "(J)V");
	env->CallObjectMethod(jqos, qos_from_rcl_mid, &p->qos_profile);
	env->SetObjectField(self, qos_fid, jqos);

Uh oh!

Conversation

ivanpauno commented Sep 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanpauno commented Sep 8, 2020

Uh oh!

jacobperronSep 8, 2020

Choose a reason for hiding this comment

Uh oh!

ivanpaunoSep 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacobperronSep 8, 2020• 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!

ivanpaunoSep 9, 2020

Choose a reason for hiding this comment

Uh oh!

jacobperronSep 9, 2020• 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!

ivanpaunoSep 9, 2020• 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobperronSep 8, 2020

Choose a reason for hiding this comment

Uh oh!

ivanpaunoSep 9, 2020

Choose a reason for hiding this comment

Uh oh!

ivanpauno commented Sep 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ivanpauno commented Sep 1, 2020•
edited
Loading

jacobperronSep 8, 2020•
edited
Loading

jacobperronSep 9, 2020•
edited
Loading

ivanpaunoSep 9, 2020•
edited
Loading