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

Implemented parameter handling #127

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Chootin
Copy link
Contributor

@Chootin Chootin commented Jan 17, 2024

This PR adds parameter handling to nodes including parameter overrides and support for use_sim_time on the node clock.

The PR also addresses the TODO regarding reading and using RMW definitions for quality of service profiles over the interop.
-> Was extracted to #131

Finally it updates the rcldotnet_examples/RCLDotnetTalker.cs example to demonstrate using a parameter.

@Chootin
Copy link
Contributor Author

Chootin commented Jan 17, 2024

Happy new year @hoffmann-stefan!

I have pulled together the commits from my fork relevant to parameter handling and QoS improvements when you have some time to have a look. Cheers!

@renemoll
Copy link
Contributor

@Chootin, first of all thanks for this patch, I am already using it.

I did have to make a fix on the QoS header, see: renemoll@ea1993f

@Chootin
Copy link
Contributor Author

Chootin commented Feb 13, 2024

@Chootin, first of all thanks for this patch, I am already using it.

I did have to make a fix on the QoS header, see: renemoll@ea1993f

Thanks for catching that! I've merged it in.

@hoffmann-stefan
Copy link
Member

Part of this was merged in #131

@Chootin: Could you force push my branch onto this? This was updated at the same time I split the Commits for #131, so you don't have to rebase everything with merge conflicts again. You can view the diff locally between your current branch and mine, there are only other changes that got merged in the meantime to main.

Something like that if you haven't done this before:

# add my remote
$ git remote add origin-hoffmann-stefan https://github.com/hoffmann-stefan/ros2_dotnet.git
$ git fetch origin-hoffmann-stefan
# Diff to check that nothing got changed
$ git diff tutina/upstream_pr2 origin-hoffmann-stefan/upstream_pr2_params_rebased
# Reset your branch to the last commit of my branch
$ git reset --hard origin-hoffmann-stefan/upstream_pr2_params_rebased
# Force push to update this PR
$ git push -f

@hoffmann-stefan hoffmann-stefan changed the title Implemented parameter handling and using RMW Quality of Service profile definitions Implemented parameter handling Jul 29, 2024
@Chootin Chootin force-pushed the tutina/upstream_pr2 branch from ea1993f to 1ba1c52 Compare July 30, 2024 03:42
@Chootin
Copy link
Contributor Author

Chootin commented Jul 30, 2024

I have completed the requested force push - looks like it can now be merged. @hoffmann-stefan

@hoffmann-stefan
Copy link
Member

@Chootin

Hi, took some time to get started reviewing this ;)

The functionality looks good, great work 👍

I did rebase your branch and made some changes as I did review this and compare the implementation with both rclppp and rclpy. see https://github.com/Chootin/ros2_dotnet/compare/tutina/upstream_pr2...hoffmann-stefan:ros2_dotnet:upstream_pr2_rebased?expand=1

The most significant change was introducing the Parameter wrapper type, like rclpy and rclcpp does.

I'm not quite finished yet reviewing, but at least wanted you to know I started and made some progress :)
Not sure how I find time for this over the holidays, if not I should be able to continue next year.

Wish you Happy Holidays :)

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.

4 participants