Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question about handling Micro-CDR #7

Open
takasehideki opened this issue Oct 16, 2021 · 5 comments
Open

Question about handling Micro-CDR #7

takasehideki opened this issue Oct 16, 2021 · 5 comments

Comments

@takasehideki
Copy link
Contributor

takasehideki commented Oct 16, 2021

This is not an issue but question to developer team.

embeddedRTPS uses eProsima Micro-CDR as the third party library. In this repository, the file for Micro-CDR was copied directly and managed as it is, not the submodule feature by git (.gitmodules).
https://github.com/embedded-software-laboratory/embeddedRTPS/tree/master/thirdparty/Micro-CDR

I understand why you did so is that you need to tune some code in Micro-CDR for the embeddedRTPS.
https://github.com/embedded-software-laboratory/embeddedRTPS/commits/master/thirdparty/Micro-CDR

However, many meaningful improvements have been committed to the Micro-CDR original repository. They may be also useful for embeddedRTPS. I think making Micro-CDR a git submodule becomes useful to follow the update about it.


So I have two questions.

Q1: which is the version you copied and located to this repository from the original repository of Micro-CDR?

Q2: Can we consider making Micro-CDR a git submodule and following the latest commits in it?
If you agree with this idea, your action item is just forking Micro-CDR repository to your Organization (embedded-software-laboratory). Then, I will try to tune the forked repository to fit the use of embeddedRTPS.
If no, I want to send PR that includes the change of file name to Micro-CDR/include/ucdr/types/string.h. We are working on mbed support for embeddedRTPS along with mROS2, but we are facing an issue due to this filename. Note that this file is no longer appeared in the latest version of Micro-CDR.
mROS-base#6 (comment)

@akampmann
Copy link
Contributor

akampmann commented Oct 19, 2021

Edit: We should definitely track the latest Micro-CDR if possible, we just have to figure out how to do this best.

Q1: I should be somewhere around this commit: eProsima/Micro-CDR@62d95c8

Q2: I absolutely don't mind adding Micro-CDR as a submodule. embeddedRTPS is currently used for a non-public project, where Micro-CDR is also used. Currently, the versions between embeddedRTPS and that other project match. I will have to double check what the necessary changes to the latest Micro-CDR project are. Does mROS itself also require changes to the latest Micro-CDR?

@pablogs9
Copy link

pablogs9 commented Oct 19, 2021

In micro-ROS we are using embeddedRTPS with mainline Micro-CDR. In this case, just using them as a CMake dependency managed with colcon. But I found a couple of problems when using latest version of our library. Maybe this commit can clarify something: pablogs9@7a8b9a4

@takasehideki
Copy link
Contributor Author

@akampmann Thank you so much for answering my questions!
In my opinion, it can be tracked to the latest version in the following ways.

  1. fork eProsima/Micro-CDR into your organization embedded-software-laboratory
  2. remove the current embeddedRTPS/thirdparty/Micro-CDR directory
  3. add 1. as the submodule
  4. and then, checkout eProsima/Micro-CDR@62d95c8 (commit ID you copied)
  5. track some commits you added for embeddedRTPS https://github.com/embedded-software-laboratory/embeddedRTPS/commits/master/thirdparty/Micro-CDR

I think the report from @pablogs9 is very useful. Thanks!
We can use the latest version of Micro-CDR in embeddedRTPS as it is just by importing pablogs9@7a8b9a4


FYI: We decided to fix the issue of name conflict about #include <ucdr/types/string.h> that happened to support Mbed Web Compiler by changing the filename. Please check mROS-base#6
But we would be happy if you could track the latest version of Micro-CDR.

@akampmann
Copy link
Contributor

Thank you both for your suggestions. I will have a look and come back about this.

@akampmann
Copy link
Contributor

This branch contains the latest Micro-CDR release as a submodule and the changes suggested by @pablogs9

https://github.com/embedded-software-laboratory/embeddedRTPS/tree/feature/microcdr-submodule

Could you check if this works for you? Its compiling on my side, but will take a few days to run it on target platform for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants