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

[BACKPORT - #227] Check if parameter is already declared to avoid re-declaring it #228

Merged
merged 2 commits into from
Nov 12, 2022

Conversation

bergercookie
Copy link
Contributor

@bergercookie bergercookie commented May 30, 2022

This is a backport of PR #227 to be used in galactic


A parameter can only be declared once. When implementing a LicecycleNode
one would normally do all the initialization in on_configure() and the
cleanup, deinitialization in the on_cleanup method. Furthermore, these
transitions could potentially be called multiple times, depending on how
the node is t ransitioned from the lifecycles are triggered. However,
this approach doesn't currently work well with Diagnostics since, in the
constructor, of diagnostic_updater::Updater it's declaring a static
parameter without checking whether that parameter has already been
declared. In subsequent calls to on_configure (and thus,
if Diagnostics are initialized there, in subsequent calls to the
Updater constructor, this results in the following message:

Original error: parameter 'diagnostic_updater.period' has already been declared

To avoid this, until the Foxy version, the wrapper code could explicitly
call undeclare_parameter on the parameter that the Updater is
declaring, but as of Galactic, undeclaring a static parameter is not
allowed anymore. See e.g., ros2/rclcpp#1850.

In this commit, I'm checking explicitly if the parameter has already
been registered and if so, I'm just reading its value before attempting
to re-declare it.

A parameter can only be declared once. When implementing a LicecycleNode
one would normally do all the initialization in `on_configure()` and the
cleanup, deinitialization in the `on_cleanup` method. Furthermore, these
transitions could potentially be called multiple times, depending on how
the node is t ransitioned from the lifecycles are triggered. However,
this approach doesn't currently work well with Diagnostics since, in the
constructor, of `diagnostic_updater::Updater` it's declaring a static
parameter without checking whether that parameter has already been
declared. In subsequent calls to `on_configure` (and thus,
if Diagnostics are initialized there, in subsequent calls to the
`Updater` constructor, this results in the following message:

```
Original error: parameter 'diagnostic_updater.period' has already been declared
```

To avoid this, until the Foxy version, the wrapper code could explicitly
call `undeclare_parameter` on the parameter that the `Updater` is
declaring, but as of Galactic, undeclaring a static parameter is not
allowed anymore. See e.g., ros2/rclcpp#1850.

In this commit, I'm checking explicitly if the parameter has already
been registered and if so, I'm just reading its value before attempting
to re-declare it.
Copy link

@com2rng com2rng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as in #227

@ralph-lange ralph-lange merged commit 76ec379 into ros:galactic Nov 12, 2022
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