Skip to content

Allow editing values_ in status_item and small refactor#441

Open
MahmoudAlmasri wants to merge 4 commits into
ros:ros2from
MahmoudAlmasri:feat/add_values_to_status_item
Open

Allow editing values_ in status_item and small refactor#441
MahmoudAlmasri wants to merge 4 commits into
ros:ros2from
MahmoudAlmasri:feat/add_values_to_status_item

Conversation

@MahmoudAlmasri

@MahmoudAlmasri MahmoudAlmasri commented Feb 19, 2025

Copy link
Copy Markdown

This PR allows editing values- within status_item in a convenient way:
* Add new constructor that takes an extra vector of KeyValue
* Introduce the addValue() method to StatusItem.
Finally, a small refactor is performed to reuse some code.

@MahmoudAlmasri MahmoudAlmasri force-pushed the feat/add_values_to_status_item branch 3 times, most recently from e506d27 to 75f2ed2 Compare February 19, 2025 14:35
@MahmoudAlmasri MahmoudAlmasri marked this pull request as draft February 19, 2025 18:54
@MahmoudAlmasri MahmoudAlmasri force-pushed the feat/add_values_to_status_item branch from 75f2ed2 to 654cdba Compare February 19, 2025 19:49
@MahmoudAlmasri MahmoudAlmasri marked this pull request as ready for review February 19, 2025 19:57
@MahmoudAlmasri MahmoudAlmasri changed the title Add status_item::addValue and refactor Allow editing values_ in status_item and small refactor Feb 21, 2025
@ct2034 ct2034 added enhancement This tackles a new feature of the code (and not a bug) ros2 PR tackling a ROS2 branch labels Feb 24, 2025
@ct2034 ct2034 self-assigned this Feb 24, 2025

@ct2034 ct2034 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for your contribution. Could you please add a unit test for these functions.

@ct2034 ct2034 added the needs more work Someone has worked on this but more work is needed label May 21, 2026
* Add new constructor that takes an extra vector of KeyValue

* Introduce the addValue() method to StatusItem.

* Refactor addValue and hasKey and move them to the cpp

Signed-off-by: Mahmoud Almasri <[email protected]>
Signed-off-by: Mahmoud Almasri <[email protected]>
@MahmoudAlmasri MahmoudAlmasri force-pushed the feat/add_values_to_status_item branch from 654cdba to 3f98d9e Compare May 24, 2026 13:54
@MahmoudAlmasri MahmoudAlmasri requested a review from ct2034 May 24, 2026 13:57
@ct2034 ct2034 requested a review from Copilot May 28, 2026 22:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a small extension to diagnostic_aggregator::StatusItem so callers can initialize and edit the internal values_ (KeyValue pairs) more conveniently, and introduces a focused unit test for this behavior.

Changes:

  • Added a new StatusItem constructor that accepts an initial std::vector<diagnostic_msgs::msg::KeyValue>.
  • Added StatusItem::addValue() (insert-or-update) and refactored hasKey() to reuse shared lookup logic.
  • Added a new gtest target to validate constructor/value-editing behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
diagnostic_aggregator/test/test_status_item.cpp New gtest coverage for the values-initializing constructor, addValue(), and hasKey().
diagnostic_aggregator/src/status_item.cpp Implements the new constructor, addValue(), hasKey(), and internal findKey().
diagnostic_aggregator/include/diagnostic_aggregator/status_item.hpp Declares the new constructor and methods; moves hasKey() out-of-line and adds findKey() declaration.
diagnostic_aggregator/CMakeLists.txt Registers the new gtest target and links it against the library.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread diagnostic_aggregator/src/status_item.cpp

@ct2034 ct2034 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just some small refinements

Comment on lines +62 to +63
EXPECT_EQ(item.getValue("a"), "1");
EXPECT_EQ(item.getValue("b"), "2");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
EXPECT_EQ(item.getValue("a"), "1");
EXPECT_EQ(item.getValue("b"), "2");
EXPECT_EQ(item.getValue("a"), "1");
EXPECT_EQ(item.getValue("b"), "2");
EXPECT_TRUE(item.hasKey("a"));
EXPECT_TRUE(item.hasKey("b"));
EXPECT_FALSE(item.hasKey("c"));

Comment on lines +81 to +84

item.addValue("k", "new");

EXPECT_EQ(item.getValue("k"), "new");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
item.addValue("k", "new");
EXPECT_EQ(item.getValue("k"), "new");
EXPECT_EQ(item.getValue("k"), "old");
item.addValue("k", "new");
EXPECT_EQ(item.getValue("k"), "new");

*/
std::size_t findKey(const std::string & key) const;

rclcpp::Time update_time_;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just a though: I think a size() method would also be helpful

@ct2034

ct2034 commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

@MahmoudAlmasri Could you please explain the rationale for this feature?
Why do you not edit the values in diagnostic_msgs::msg::DiagnosticStatus directly?

@MahmoudAlmasri

Copy link
Copy Markdown
Author

@ct2034 Conceptually, a StatusItem allows reading values and emitting a path-prefixed, staleness-aware message, but until this PR, there was no way to populate values except by copying them in from a wire DiagnosticStatus. This PR closes that asymmetry.
The existing alternative is DiagnosticStatusWrapper. But using it would demand to depend on diagnostic_updater package where it lives.
Thanks to the introduced feature, the user would gain more freedom on safely editing the status item without this extra dependency.

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