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

Add add_analyzer functionality #329

Merged
merged 13 commits into from
Jun 27, 2024

Conversation

MartinCornelis2
Copy link
Contributor

@MartinCornelis2 MartinCornelis2 commented Jan 22, 2024

Applied the following changes to allow adding analyzers at runtime similar to the ROS1 functionality.

  • Move the creation of analyzer_group_ and other_analyzer_ from the constructor to a new function initAnalyzers().

  • Added a subscriber to parameter events param_sub_ that triggers a callback parameterCallback.

  • Check in the callback if this node got any new paramters -> if true, lock the mutex and call initAnalyzers() again.

  • Created an add_analyzer node that forwards its own parameters to the diagnostic_aggregator by sending a request to /diagnostics_agg/set_parameters_atomically service

  • (EDIT) [The add_analyzer looks for all parameters starting with the prefix analyzers.,] has been replaced with [The add_analyzer looks for all parameters for which a similar named parameter exists with the suffix ".path"] converts them to a parameter_msg and sends them together as one SetParametersAtomically::Request

Example usage:

<node name="add_my_analyzer" pkg="diagnostic_aggregator" exec="add_analyzer" >
        <param from="$(find-pkg-share <pkg_name>)/param/<param_file_path>.yaml" allow_substs="true"/>
        <remap from="/diagnostics_agg/set_parameters_atomically" to="/diagnostics_autonomy/set_parameters_atomically" />
</node>

Copy link
Collaborator

@ct2034 ct2034 left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature.
Please add

  • information on the usage in diagnostic_aggregator/README.md
  • a test that initializes a new analyzer at runtime

@ct2034 ct2034 added enhancement This tackles a new feature of the code (and not a bug) needs more work Someone has worked on this but more work is needed ros2 PR tackling a ROS2 branch labels Jan 25, 2024
@Timple
Copy link
Contributor

Timple commented Mar 27, 2024

Looks like all comments where addressed. Any chance for a re-review?

@Timple
Copy link
Contributor

Timple commented Jun 27, 2024

If I may be so bold... Since there's a lot of maintenance effort going on: can this get a re-review? 🙂

@ct2034
Copy link
Collaborator

ct2034 commented Jun 27, 2024

Of course. Let's use the momentum while it lasts ;-)

@ct2034
Copy link
Collaborator

ct2034 commented Jun 27, 2024

@ct2034 ct2034 merged commit 5e1415c into ros:ros2 Jun 27, 2024
5 of 6 checks passed
ct2034 pushed a commit to ct2034/diagnostics that referenced this pull request Jun 27, 2024
* Add add_analyzer functionality
* Add copyright notice and license, remove unused includes, re-order includes correctly
* Increase clarity of prefix_ by renaming it to analyzers_ns_
* Add add_analyzer functionality
* Fix bug where base_path is not reset correctly
* Make the parameter forwarding condition more generic, fix the default service namespace from diagnostics_agg to analyzers
* Add an add_analyzer example to the diagnostic_aggregator
* Update the relevant READMEs
* Fix linter errors
* Add test for add_analyzer at runtime, remove unnecessary ros info logger, remove unnecessary hardcoded namespace from yaml files
* Remove the now redundant analyzers_ns_
* Change the copyright of add_analyzer, forgot to update it to Nobleo after copying the notice

(cherry picked from commit 5e1415c)
ct2034 pushed a commit to ct2034/diagnostics that referenced this pull request Jun 27, 2024
* Add add_analyzer functionality
* Add copyright notice and license, remove unused includes, re-order includes correctly
* Increase clarity of prefix_ by renaming it to analyzers_ns_
* Add add_analyzer functionality
* Fix bug where base_path is not reset correctly
* Make the parameter forwarding condition more generic, fix the default service namespace from diagnostics_agg to analyzers
* Add an add_analyzer example to the diagnostic_aggregator
* Update the relevant READMEs
* Fix linter errors
* Add test for add_analyzer at runtime, remove unnecessary ros info logger, remove unnecessary hardcoded namespace from yaml files
* Remove the now redundant analyzers_ns_
* Change the copyright of add_analyzer, forgot to update it to Nobleo after copying the notice

(cherry picked from commit 5e1415c)
ct2034 pushed a commit to ct2034/diagnostics that referenced this pull request Jun 27, 2024
* Add add_analyzer functionality
* Add copyright notice and license, remove unused includes, re-order includes correctly
* Increase clarity of prefix_ by renaming it to analyzers_ns_
* Add add_analyzer functionality
* Fix bug where base_path is not reset correctly
* Make the parameter forwarding condition more generic, fix the default service namespace from diagnostics_agg to analyzers
* Add an add_analyzer example to the diagnostic_aggregator
* Update the relevant READMEs
* Fix linter errors
* Add test for add_analyzer at runtime, remove unnecessary ros info logger, remove unnecessary hardcoded namespace from yaml files
* Remove the now redundant analyzers_ns_
* Change the copyright of add_analyzer, forgot to update it to Nobleo after copying the notice

(cherry picked from commit 5e1415c)
@ct2034
Copy link
Collaborator

ct2034 commented Jun 27, 2024

💚 All backports created successfully

Status Branch Result
ros2-humble
ros2-iron
ros2-jazzy

Questions ?

Please refer to the Backport tool documentation

ct2034 added a commit that referenced this pull request Jun 27, 2024
* Add add_analyzer functionality
* Add copyright notice and license, remove unused includes, re-order includes correctly
* Increase clarity of prefix_ by renaming it to analyzers_ns_
* Add add_analyzer functionality
* Fix bug where base_path is not reset correctly
* Make the parameter forwarding condition more generic, fix the default service namespace from diagnostics_agg to analyzers
* Add an add_analyzer example to the diagnostic_aggregator
* Update the relevant READMEs
* Fix linter errors
* Add test for add_analyzer at runtime, remove unnecessary ros info logger, remove unnecessary hardcoded namespace from yaml files
* Remove the now redundant analyzers_ns_
* Change the copyright of add_analyzer, forgot to update it to Nobleo after copying the notice

(cherry picked from commit 5e1415c)

Co-authored-by: MartinCornelis2 <[email protected]>
ct2034 added a commit that referenced this pull request Jun 27, 2024
* Add add_analyzer functionality
* Add copyright notice and license, remove unused includes, re-order includes correctly
* Increase clarity of prefix_ by renaming it to analyzers_ns_
* Add add_analyzer functionality
* Fix bug where base_path is not reset correctly
* Make the parameter forwarding condition more generic, fix the default service namespace from diagnostics_agg to analyzers
* Add an add_analyzer example to the diagnostic_aggregator
* Update the relevant READMEs
* Fix linter errors
* Add test for add_analyzer at runtime, remove unnecessary ros info logger, remove unnecessary hardcoded namespace from yaml files
* Remove the now redundant analyzers_ns_
* Change the copyright of add_analyzer, forgot to update it to Nobleo after copying the notice

(cherry picked from commit 5e1415c)

Co-authored-by: MartinCornelis2 <[email protected]>
ct2034 added a commit that referenced this pull request Jun 27, 2024
* Add add_analyzer functionality
* Add copyright notice and license, remove unused includes, re-order includes correctly
* Increase clarity of prefix_ by renaming it to analyzers_ns_
* Add add_analyzer functionality
* Fix bug where base_path is not reset correctly
* Make the parameter forwarding condition more generic, fix the default service namespace from diagnostics_agg to analyzers
* Add an add_analyzer example to the diagnostic_aggregator
* Update the relevant READMEs
* Fix linter errors
* Add test for add_analyzer at runtime, remove unnecessary ros info logger, remove unnecessary hardcoded namespace from yaml files
* Remove the now redundant analyzers_ns_
* Change the copyright of add_analyzer, forgot to update it to Nobleo after copying the notice

(cherry picked from commit 5e1415c)

Co-authored-by: MartinCornelis2 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This tackles a new feature of the code (and not a bug) needs more work Someone has worked on this but more work is needed ros2 PR tackling a ROS2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants