Added new functions which can be used to get rmw_qos_profile_t from WriterQos and ReaderQos

[jaisontj]

• Added new functions to qos.hpp which can be used to get rmw_qos_profile_t from WriterQos and ReaderQos
• Refactored dds_qos_to_rmw to re-use these new functions
• Added tests for dds_qos_to_rmw

Signed-off-by: Jaison Titus jaisontj92@gmail.com

[jaisontj]

@ivanpauno as part of refactoring, as discussed here

[ivanpauno]

LGTM!
I left some minimal comments

[ivanpauno]

This is pre-existent, but there is no reason to forward-declare here.
The corresponding headers should be included.

[jaisontj]

Oops. I had an alphabetical sort done on these. I'll remove this change.
Maybe removing the forward-declaration all together can be in a different PR?

[ivanpauno]

Yes, it can be done in a different PR.

[ivanpauno]

Why a separated header?
It's not clear in which header the functions should live.

Also, I think that dds_qos_to_rmw_qos should be renamed to dds_attributes_to_rmw_qos, or something similar.
That would make a clearer difference between one function and the other.

[jaisontj]

I put it in a different header to make the function package-private. Adding this as a new function inside qos.hpp would've made it public. Let me know if you think otherwise.

I agree with the name dds_attributes_to_rmw_qos; makes more sense than what it is now. Will change it!

[ivanpauno]

I didn't realize it was a package private header, so it's ok.

I see you renamed this method to dds_attributes_to_rmw_qos:

rmw_fastrtps/rmw_fastrtps_shared_cpp/src/qos_converter.hpp

Lines 34 to 36 in 40f9523

	dds_attributes_to_rmw_qos(
	const DDSQoSPolicyT & dds_qos,
	rmw_qos_profile_t * qos)

Though that one is transforming a dds qos into a rmw qos (and not a dds publisher/subscriber attributes into a rmw qos). So it should be named dds_qos_to_rmw_qos as before.

My idea was to rename this other to dds_attribute_to_rmw_qos:

rmw_fastrtps/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp

Lines 47 to 51 in 40f9523

	template<typename AttributeT>
	void
	dds_qos_to_rmw_qos(
	const AttributeT & dds_qos,
	rmw_qos_profile_t * qos);

As this one in the one transforming the dds publisher/subscriber attributes into a rmw qos.

[jaisontj]

Ah! You're right; the excitement around getting this PR merged got the better of me! I will change it back to how it was before.

As for renaming dds_qos_to_rmw_qos would you suggest that be done as a part of this PR? I ask because it involves a change in quite a few files.

[ivanpauno]

I find having both dds_qos_to_rmw_qos and dds_qos_policies_to_rmw_qos quite confusing (if I remember correctly, that was the original proposal).
I think it would be good to do the change as part of this PR.
It shouldn't be more than doing find and replace 😉 (through rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp and rmw_fastrtps_shared_cpp).

[ivanpauno]

I see you have sent #330, which is ok.
Please, rename this function back to dds_qos_to_rmw_qos and we're done.

[jaisontj]

Yeah, I had the PR ready by the time your comment came in :P.

I've renamed the function back to dds_qos_to_rmw_qos and also made the necessary changes in the tests :)

[ivanpauno]

• Linux
• Linux-aarch64
• macOS
• Windows

[jaisontj]

Looks like it failed at Step 21/58 : RUN apt-get update && apt-get install --no-install-recommends -y python3-pip; a re-run might just fix it?

[ivanpauno]

Looks like it failed at Step 21/58 : RUN apt-get update && apt-get install --no-install-recommends -y python3-pip; a re-run might just fix it?

Probably, Linux CI again:

• Linux