Use weak_ptr to avoid circular dependency in panther_battery package #310

KmakD · 2024-05-20T08:56:52Z

Summary by CodeRabbit

New Features
- Improved logging functionality for battery status.
- Enhanced clock retrieval capabilities for better time management.
Refactor
- Optimized internal method calls for better performance and maintainability.
Bug Fixes
- Addressed potential issues with battery timeout handling.

…os2-fix-circular-dependency

coderabbitai · 2024-05-27T10:11:30Z

Walkthrough

The recent updates to the BatteryPublisher class involve refactoring method signatures to remove the const qualifier, changing the type of node_ from SharedPtr to WeakPtr, and adding new methods for retrieving the logger and clock. These changes streamline the code and enhance flexibility in method calls and resource management.

Changes

File	Change Summary
`panther_battery/include/.../battery_publisher.hpp`	Removed `const` qualifier from `TimeoutReached` and `BatteryStatusLogger` methods; added `GetLogger` and `GetClock` methods; changed `node_` type to `WeakPtr`.
`panther_battery/src/battery_publisher.cpp`	Updated method calls to use `node.` instead of `node_->`; modified `TimeoutReached`; added `GetLogger` and `GetClock` methods; adjusted parameter declarations and subscription creation.

Poem

In code, a shift, a subtle tweak,
Methods freed, no longer meek.
A logger found, a clock in hand,
With WeakPtr now, the node will stand.
Battery's tale, in bytes conveyed,
Refactored paths, in code arrayed.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to full the review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5e3ccee and 03c0a78.

Files selected for processing (4)

panther_battery/include/panther_battery/battery_publisher.hpp (1 hunks)
panther_battery/src/battery_publisher.cpp (5 hunks)
panther_battery/src/dual_battery_publisher.cpp (2 hunks)
panther_battery/src/single_battery_publisher.cpp (2 hunks)

Additional comments not posted (13)

panther_battery/include/panther_battery/battery_publisher.hpp (3)

52-53: Removed const qualifiers from TimeoutReached and BatteryStatusLogger.

This change aligns with the PR's objective to modify method signatures for better flexibility in method usage.

57-59: Introduced GetLogger and GetClock methods.

These methods provide centralized access to logging and clock functionalities, improving code maintainability and reducing redundancy.

60-60: Changed node_ from SharedPtr to WeakPtr.

This change helps avoid circular dependencies and potential memory leaks, aligning with modern C++ best practices for smart pointer usage.

panther_battery/src/single_battery_publisher.cpp (3)

38-39: Updated publisher initialization to use node-> instead of node_->.

This change reflects the updated pointer type (WeakPtr) and ensures that the object is accessed correctly.

44-44: Updated to use GetClock() for obtaining the current time.

Using GetClock() centralizes clock access, enhancing code consistency and maintainability.

Also applies to: 50-50

66-66: Utilized GetLogger() and GetClock() in logging function.

This usage promotes consistency and reduces direct dependency on the node object, aligning with the changes made in the class definition.

panther_battery/src/battery_publisher.cpp (4)

32-33: Updated parameter declaration and subscription creation to use node->.

Reflects the change to WeakPtr, ensuring that the node is accessed correctly.

Also applies to: 38-38

50-53: Updated to use GetClock() and GetLogger() in the Publish method.

Enhances consistency and maintainability by centralizing access to logging and clock functionalities.

68-70: Updated TimeoutReached and BatteryStatusLogger to use GetClock() and GetLogger().

These changes are consistent with the objective of reducing direct dependencies on the node object and improving code maintainability.

Also applies to: 74-97

125-139: Implemented GetLogger() and GetClock() methods.

These methods provide a safe way to access the node's logger and clock, handling the possibility of the node being expired.

panther_battery/src/dual_battery_publisher.cpp (3)

41-43: Updated publisher initialization to use node-> instead of node_->.

This change reflects the updated pointer type (WeakPtr) and ensures that the object is accessed correctly.

48-48: Updated to use GetClock() for obtaining the current time.

Using GetClock() centralizes clock access, enhancing code consistency and maintainability.

Also applies to: 55-55

74-78: Utilized GetLogger() and GetClock() in logging function.

This usage promotes consistency and reduces direct dependency on the node object, aligning with the changes made in the class definition.

rafal-gorecki · 2024-05-27T10:42:35Z

panther_battery/include/panther_battery/battery_publisher.hpp

+  rclcpp::Logger GetLogger();
+  rclcpp::Clock::SharedPtr GetClock();
+
+  rclcpp::Node::WeakPtr node_;


Maybe add info to do not forget why is that?

rafal-gorecki

Small suggestion rest is fine.

coderabbitai

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 03c0a78 and 990d6c0.

Files selected for processing (2)

panther_battery/include/panther_battery/battery_publisher.hpp (1 hunks)
panther_battery/src/battery_publisher.cpp (5 hunks)

Files skipped from review as they are similar to previous changes (2)

panther_battery/include/panther_battery/battery_publisher.hpp
panther_battery/src/battery_publisher.cpp

use weak_ptr to avoid circular dependency

e941ea1

JanBrzyk requested a review from rafal-gorecki May 21, 2024 08:30

Merge branch 'ros2' of https://github.com/husarion/panther_ros into r…

03c0a78

…os2-fix-circular-dependency

coderabbitai bot reviewed May 27, 2024

View reviewed changes

rafal-gorecki reviewed May 27, 2024

View reviewed changes

rafal-gorecki approved these changes May 27, 2024

View reviewed changes

add comment

5278d27

KmakD requested a review from rafal-gorecki May 27, 2024 11:37

rafal-gorecki approved these changes May 27, 2024

View reviewed changes

Merge branch 'ros2' into ros2-fix-circular-dependency

990d6c0

rafal-gorecki merged commit 0be2f54 into ros2 May 27, 2024

rafal-gorecki deleted the ros2-fix-circular-dependency branch May 27, 2024 14:36

coderabbitai bot reviewed May 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use weak_ptr to avoid circular dependency in panther_battery package #310

Use weak_ptr to avoid circular dependency in panther_battery package #310

KmakD commented May 20, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented May 27, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

rafal-gorecki May 27, 2024

rafal-gorecki left a comment

coderabbitai bot left a comment

Use weak_ptr to avoid circular dependency in panther_battery package #310

Use weak_ptr to avoid circular dependency in panther_battery package #310

Conversation

KmakD commented May 20, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot commented May 27, 2024 • edited Loading

Walkthrough

Changes

Poem

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

rafal-gorecki May 27, 2024

Choose a reason for hiding this comment

rafal-gorecki left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

KmakD commented May 20, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented May 27, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)