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

fix interactions between missing/deleted params and param subscription (get_param_cached) #9

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

Conversation

jobafr
Copy link
Contributor

@jobafr jobafr commented Jan 12, 2025

Fixes:

  • subscribing to non-existent params (maybe they've not yet been set?) now notifies the subscriber
  • deleting a param also notifies the subscriber

Because ROS seems to use the empty struct as the placeholder value for non-existing values in the param tree, for this to work we need a version of dxr that supports creating that value. I have opened a PR there as well.

That would need to be merged first (and a new release made on crates.io) for us to be able to use it in ros-core-rs.

Edit: failing CI is expected at the moment, due to the required change in dxr's API.

Edit 2: @decathorpe pointed out a way to create an empty struct without changing dxr's API, so I changed the code accordingly.

The remaining build failure is unrelated and seems to be caused by some version of xml-rs having been yanked. This affects rosrust as well, and is unrelated to this change. You can still build with an old Cargo.lock file in place.

Edit 3: The build with rust nightly is broken as well (for the same reason), I don't know why this shows up as green though.

@PatWie
Copy link
Owner

PatWie commented Jan 13, 2025

Thanks a ton for the pull request and for all your effort in fixing these bugs—it’s really appreciated! 🙌 I’m still on vacation for another week, so I can’t dig into the build errors just yet. Once I’m back, I’ll give it a proper review and also bump the Cargo crate version.

@jobafr
Copy link
Contributor Author

jobafr commented Jan 13, 2025

ok have fun :)

if you want to build it, it works fine for me using this Cargo.lock file:
https://gist.github.com/jobafr/505aeed796b8785b8f9a27bfb1afa69a

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.

2 participants