Revert dynamic params

[ghost]

Description of contribution in a few bullet points

• Reverts the dynamic parameter changes. They broke the system tests.
• Something in Bringing back dynamic parameters: Costmap2D #1370 is the primary cause of the failures, but Bringing back dynamic parameters: Amcl #1378 depends on Bringing back dynamic parameters: Costmap2D #1370 so this backs it out as well.

[ghost]

Now that I recall, I believe @bpwilcox mentioned there were recent upstream changes that broke this. I believe commits as they stand are correct for eloquent, but not for master.

I'll make a eloquent-devel branch today or tomorrow and retry them there.

[bpwilcox]

That is correct, ros2/rclcpp#929 on master breaks the current approach in my PRs. @crdelsey I had planned to submit a PR for the dwb parameter callbacks today, so it should now go to the eloquent-devel branch?

[ghost]

@bpwilcox Yeah. Target it to eloquent-devel. I just created the eloquent-devel branch and based off the version before this PR went in, so it should merge with no issues.

I was thinking we'd have a bit of time to deal with the conflict with upstream when I suggested merging to master, but it's all good. It just forced me to make the eloquent-devel branch a day early.

When we're ready to make it work on master, we can just revert this PR and than add the modification on top if that make sense.

[bpwilcox]

You already created the eloquent-devel branch? I don't currently see it

[seanyen]

@SteveMacenski Just want to bring this to your attention, on eloquent branch, I hit the access violation on Windows when the ACML nodes receive the laser scan data (a dangling callback on a already de-allocated laser_scan_filter_ object) and after some digging, it turned out that after I remove the dynamic params (removing the registration of paramEventCallback), the issue is no longer repro. I am not sure if it is the similar issue rendered here. If it is, perhaps it is something to consider to be back-ported to eloquent?

[SteveMacenski]

@seanyen just to be clear- you're suggesting we cherry pick this commit into the Eloquent branch, correct?

That seems reasonable. Confirm that that is what you're suggesting and I'll do it.

[seanyen]

@SteveMacenski Yes! That’s what I am suggesting. And thanks for taking care of it.

[SteveMacenski]

Got it- thanks for the idea.

I don't use the devel branches while developing on master so some of the backporting stuff I don't run into. Thanks for addressing it.