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

feat(tier4_autoware_utils): add published time debug class into utils #6440

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

brkay54
Copy link
Member

@brkay54 brkay54 commented Feb 16, 2024

Description

Part of:

Required for:

Depends on:

This interface would be enabled only if the use_published_time = true.

Tests performed

Tested on PSim and e2e_simulator, worked well.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the component:common Common packages from the autoware-common repository. (auto-assigned) label Feb 16, 2024
@brkay54 brkay54 force-pushed the feat/publish-time-info branch 2 times, most recently from f9f6613 to 4f24256 Compare February 25, 2024 16:41
@github-actions github-actions bot added component:perception Advanced sensor data processing and environment understanding. (auto-assigned) component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) component:planning Route planning, decision-making, and navigation. (auto-assigned) component:control Vehicle control algorithms and mechanisms. (auto-assigned) labels Feb 25, 2024
@brkay54 brkay54 force-pushed the feat/publish-time-info branch 2 times, most recently from c84d1b0 to 19ddcab Compare February 26, 2024 06:41
@github-actions github-actions bot removed component:perception Advanced sensor data processing and environment understanding. (auto-assigned) component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) component:planning Route planning, decision-making, and navigation. (auto-assigned) component:control Vehicle control algorithms and mechanisms. (auto-assigned) labels Feb 26, 2024
@brkay54 brkay54 requested a review from mitsudome-r February 26, 2024 06:52
@brkay54 brkay54 marked this pull request as ready for review February 26, 2024 06:53
@xmfcx
Copy link
Contributor

xmfcx commented Mar 11, 2024

Could you move struct GidCompare to the class, as a private member, closer to its usage?

Reasons:

  1. Encapsulation is improved by moving GidCompare inside PublishedTimePublisher, making it clear that this comparison logic is specific to the class's internal workings.
  2. Cohesion is enhanced by keeping related functionalities together, making the class more self-contained and understandable.

Example:

private:
  rclcpp::Node * node_;
  std::string end_name_;
  rclcpp::QoS qos_;

  struct GidCompare
  {
    bool operator()(const rmw_gid_t & lhs, const rmw_gid_t & rhs) const
    {
      return std::memcmp(lhs.data, rhs.data, RMW_GID_STORAGE_SIZE) < 0;
    }
  };

  std::map<rmw_gid_t, rclcpp::Publisher<PublishedTime>::SharedPtr, GidCompare> publishers_;

  void ensure_publisher_exists(const rmw_gid_t & gid_key, const std::string & topic_name);

@xmfcx
Copy link
Contributor

xmfcx commented Mar 11, 2024

Placing the using directive for PublishedTime outside the class at the namespace level makes it globally accessible within the namespace, which could lead to naming conflicts with other entities. This broader scope unnecessarily exposes the alias, potentially complicating the codebase and affecting readability for developers not directly working with the PublishedTimePublisher class.

Moving the using directive for PublishedTime inside the PublishedTimePublisher class confines its scope to where it is actually used, enhancing code readability and maintainability. This approach avoids potential naming conflicts and makes the class definition more self-contained and easier to understand.

private:
  rclcpp::Node * node_;
  std::string end_name_;
  rclcpp::QoS qos_;

  using PublishedTime = autoware_internal_msgs::msg::PublishedTime;

@brkay54 brkay54 force-pushed the feat/publish-time-info branch 2 times, most recently from a5da243 to 550b9b0 Compare March 11, 2024 11:43
@brkay54 brkay54 requested a review from xmfcx March 11, 2024 11:46
@brkay54
Copy link
Member Author

brkay54 commented Mar 11, 2024

@xmfcx Thank you for the detailed review. 🙏🏼

@brkay54 brkay54 force-pushed the feat/publish-time-info branch from 550b9b0 to 7626af7 Compare March 12, 2024 07:53
@brkay54 brkay54 force-pushed the feat/publish-time-info branch from 7626af7 to b42b772 Compare March 12, 2024 08:34
Copy link
Contributor

@xmfcx xmfcx left a comment

Choose a reason for hiding this comment

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

thanks!

@xmfcx
Copy link
Contributor

xmfcx commented Mar 12, 2024

https://github.com/autowarefoundation/autoware.universe/actions/runs/8245809015/job/22550518856?pr=6440#step:7:99
fails

@xmfcx
Copy link
Contributor

xmfcx commented Mar 12, 2024

a1a595c should fix it.

@brkay54 brkay54 enabled auto-merge (squash) March 12, 2024 12:04
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 14.78%. Comparing base (b7fdccc) to head (a1a595c).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6440      +/-   ##
==========================================
- Coverage   14.79%   14.78%   -0.02%     
==========================================
  Files        1915     1915              
  Lines      132352   132547     +195     
  Branches    39333    39496     +163     
==========================================
+ Hits        19580    19593      +13     
- Misses      90939    91086     +147     
- Partials    21833    21868      +35     
Flag Coverage Δ *Carryforward flag
differential 16.78% <ø> (?)
total 14.79% <ø> (ø) Carriedforward from b7fdccc

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brkay54
Copy link
Member Author

brkay54 commented Mar 12, 2024

@soblin @TakaHoribe @takayuki5168 Hi, we need a code owner approvement. Could you check the PR?

Copy link
Contributor

@takayuki5168 takayuki5168 left a comment

Choose a reason for hiding this comment

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

@brkay54
Thank you for the contribution. Can you create a unit test for the feature? Generally, we create a test for functions in the tier4_autoware_utils package.

@xmfcx xmfcx disabled auto-merge March 12, 2024 19:59
@xmfcx
Copy link
Contributor

xmfcx commented Mar 12, 2024

@brkay54 could you open up a new PR for the tests of the code added here? I will merge it, it is blocking some other PRs.

@xmfcx xmfcx merged commit 69572f1 into autowarefoundation:main Mar 12, 2024
31 of 32 checks passed
@brkay54 brkay54 deleted the feat/publish-time-info branch March 13, 2024 11:03
kaigohirao pushed a commit to kaigohirao/autoware.universe that referenced this pull request Mar 22, 2024
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common Common packages from the autoware-common repository. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to detect publishing times of the messages in the pipelines
5 participants