Skip to content

Conversation

@adamdbrw
Copy link
Contributor

@adamdbrw adamdbrw commented Jul 8, 2019

Work in progress: this is not a master-ready material, but please create a working branch so we can coordinate work between 3 people easily and make other pull requests to it.
This is a natural step towards Dashing port, which should follow once we get this to a satisfying level.

Current status:

  • Examples confirmed to run. Tested with fresh build, no other testing done.

How to run:

  • Following prerequisites are required: existing Crystal install, colcon.
  • Use ros2_dotnet_crystal.repo with vcs, this is a minimal set of repos.
  • Source your Crystal env.
  • colcon build

Required work:

  • Update README with option of build with existing Crystal install, colcon etc.
  • Produce and test .repo file for standalone build (aka no Crystal distro installed). Remember to add colcon dependency, remove unnecessary ones.
  • Test with UWP
  • Update CI to test for new minimal build
  • Update NOTICE files so they reflect contributions properly

Commit message:
Integrated improvements from unity_ros2 project:

  • nested types
  • unit tests
  • rclcs overhaul to crystal version and structure changes

Also added my own fixes to build and remaining code

  • extended interfaces to provide something useful
  • replaced obsolete rcl_ok() call
  • adapted examples to new library semantics
  • introduced ros2_dotnet_crystal.repo, which supports a smaller build with existing Crystal install

- nested types
- unit tests
- rclcs overhaul to crystal version and structure changes

Also added my own fixes to build and remaining code
- extended interfaces to provide something useful
- replaced obsolete rcl_ok() call
- adapted examples to new library semantics
- introduced ros2_dotnet_crystal.repo, which supports a smaller build with existing Crystal install
@adamdbrw
Copy link
Contributor Author

adamdbrw commented Jul 8, 2019

@esteve

@esteve
Copy link
Member

esteve commented Jul 8, 2019

@adamdbrw thanks, I really appreciate the effort, but this is impossible to review. Can it be broken into smaller pull requests? Also, I'm not comfortable with including work from someone else's, it should be Samuel who submits the pull request, to ensure that all is ok, copyright-wise.

@adamdbrw
Copy link
Contributor Author

adamdbrw commented Jul 8, 2019

@esteve
I would really like to make it easier for you.

  • I am cooperating with Samuel on this and I emailed him the pull request so hopefully he can comment himself. Please note that the integration was not straightforward and took some significant effort since you don't have the same root and there were tasks of porting and fixing, and he has limited time while I have some hours for this in my project. I kept all the NOTICE and authorship stuff intact, didn't even add my name anywhere.
  • Lots of stuff is interdependent here and hard to break into smaller pull request without plenty of creative effort to do so (and I can't be convinced to do it). If I could make it easier for you without much work on my side, I would (ideas?). However I am working in a frame where I can't afford to do much "extra" work, unfortunately.

@adamdbrw
Copy link
Contributor Author

adamdbrw commented Jul 8, 2019

@esteve I believe these are not normal circumstances, since we are creatively integrating two bodies of work with plenty of differences. There are methods to handle this kind of case, i.e. a dedicated code-base review after we pass Unit Tests and CI. I also understand there's anxiety when your child mutates into something alien.

I like small pull requests just like any other guy, and I seek no glory.

It is of course a question whether the benefit is worth the effort for you and other ros2_dotnet users.

@theseankelly
Copy link
Member

theseankelly commented Jul 9, 2019

FWIW my current thinking is to focus on getting a 'minimal' standalone build of dashing running in a UWP container, using the C++/WinRT native layer (no C# required). This ought to serve as a foundation for deploying ROS2 within UWP (including the various patches to rcl and others by @esteve and anything else that pops up).

My understanding is the the rcldotnet/rclcs stuff should ride on top of that rather independently, right?

By the way @esteve is there any significant need to fully support crystal now that dashing is out? I'd rather skip straight to the latest.

@esteve
Copy link
Member

esteve commented Jul 9, 2019

@esteve I believe these are not normal circumstances, since we are creatively integrating two bodies of work with plenty of differences.

Except that one of the bodies of work started two years ago, whereas the other is a fork that could have been integrated as patches from the beginning. This pull request wasn't created in a vacuum completely separate from the rest of the world.

There are methods to handle this kind of case, i.e. a dedicated code-base review after we pass Unit Tests and CI.

Why not just contribute the test suite? That's what I told you by email.

I also understand there's anxiety when your child mutates into something alien.

There's nothing of that, look at https://github.com/ros2-rust/ros2_rust for example. A user saw that ros2-rust already existed and instead of forking it and evolve it on their own without contributing back, he decided to submit a pull request (ros2-rust/ros2_rust#3) that included a lot of improvements, but in a way that was humanly reviewable. The outcome? I added that user to the team and he and I are now collaborating, he's actually taking the lead on the project.

Moreover, what's the point of replacing one project with another? I just don't understand. If you like the other project, seriously, use it, I will not force anyone to use any of the projects I've written. I'm doing this in my spare time for the fun of it.

It is of course a question whether the benefit is worth the effort for you and other ros2_dotnet users.

However, why should I make the effort of integrating a third person's work (Samuel)? Why couldn't that person submit this pull request? Your effort is laudable, but I just don't understand any of this process. Why isn't Samuel doing this work instead of you?

@esteve
Copy link
Member

esteve commented Jul 9, 2019

@theseankelly that makes the most sense and it's much easier to iterate that way. Ensuring that ROS 2 Dashing as is (up to rcl) can be built for UWP would pave the way for any .NET implementation to support the HoloLens.

I'll set up the CI to use colcon and then we can update the repos files to build the rcl-based subset for UWP.

@adamdbrw
Copy link
Contributor Author

adamdbrw commented Jul 9, 2019

@esteve
Should we close the pull request then?
If you have an idea how I can help you otherwise I would be happy to.

@adamdbrw adamdbrw closed this Jul 15, 2019
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

Successfully merging this pull request may close these issues.

3 participants