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: create autoware_auto_msgs_adapter node #3876

Merged
merged 28 commits into from
Jun 19, 2023

Conversation

xmfcx
Copy link
Contributor

@xmfcx xmfcx commented May 31, 2023

Description

Closes: #3845

Tests performed

Fill here.

Effects on system behavior

It will do:

  • subscribe to a message
  • convert the message
  • publish the converted message

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.

@xmfcx xmfcx added component:system System design and integration. (auto-assigned) core component:simulation Virtual environment setups and simulations. (auto-assigned) labels May 31, 2023
@xmfcx xmfcx added this to the 2023 May - Jun Milestone milestone May 31, 2023
@xmfcx xmfcx self-assigned this May 31, 2023
@github-actions github-actions bot removed the component:system System design and integration. (auto-assigned) label May 31, 2023
@xmfcx xmfcx marked this pull request as draft May 31, 2023 16:33
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: +0.03 🎉

Comparison is base (07f8de7) 14.45% compared to head (9e4e50d) 14.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3876      +/-   ##
==========================================
+ Coverage   14.45%   14.48%   +0.03%     
==========================================
  Files        1456     1461       +5     
  Lines      102537   102639     +102     
  Branches    29652    29704      +52     
==========================================
+ Hits        14819    14870      +51     
- Misses      71567    71568       +1     
- Partials    16151    16201      +50     
Flag Coverage Δ *Carryforward flag
differential 50.00% <50.00%> (?)
total 14.45% <ø> (ø) Carriedforward from 361dea5

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

Impacted Files Coverage Δ
...dapter/test/test_msg_ackermann_control_command.cpp 33.33% <33.33%> (ø)
...gs_adapter/src/autoware_auto_msgs_adapter_core.cpp 47.82% <47.82%> (ø)
...s_adapter/test/test_autoware_auto_msgs_adapter.cpp 66.66% <66.66%> (ø)
...are_auto_msgs_adapter/src/include/adapter_base.hpp 75.00% <75.00%> (ø)
..._auto_msgs_adapter/src/include/adapter_control.hpp 92.85% <92.85%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xmfcx xmfcx force-pushed the feat/autoware_auto_msgs_adapter branch from 50f0284 to fd9ec46 Compare May 31, 2023 16:43
@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label May 31, 2023
@xmfcx xmfcx force-pushed the feat/autoware_auto_msgs_adapter branch 3 times, most recently from 9c21566 to 7efe1ad Compare June 5, 2023 15:47
M. Fatih Cırıt added 4 commits June 12, 2023 20:12
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@xmfcx xmfcx force-pushed the feat/autoware_auto_msgs_adapter branch from 7efe1ad to 14ab27b Compare June 12, 2023 17:12
pre-commit-ci bot and others added 2 commits June 12, 2023 17:14
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@xmfcx xmfcx requested a review from isamu-takagi June 12, 2023 18:08
@xmfcx xmfcx marked this pull request as ready for review June 12, 2023 18:09
@xmfcx
Copy link
Contributor Author

xmfcx commented Jun 12, 2023

@isamu-takagi I've created the first functioning prototype for this node. I have also added an integration test for this too.

I'm open to any suggestions on this node.

Could you please review?

After your first review, I will also add documentation to show how it works too.

I've made it to be extensible like we discussed.

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
{
public:
using SharedPtrInterface = std::shared_ptr<AdapterBaseInterface>;
using ConstSharedPtrInterface = const SharedPtrInterface;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Interface is needed for a pointer name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2023-06-13 17-31-18

Screenshot from 2023-06-13 17-33-10

The reason was to be able to differentiate with the shared pointer from the class and the base class.

This functionality is not used, so it makes sense to remove it for this case. (Done in: a4e98e0 )

But if it was used, would it makes sense to either:

  • give it a different name like I did
  • OR somehow cast it to the base or derived class' pointer in the places it is used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't notice that. I'll look into type alias for derived classes.

M. Fatih Cırıt added 3 commits June 13, 2023 17:04
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@yukkysaito
Copy link
Contributor

Thanks for the very good PR.

When we move from autoware_auto_msgs to autoware_msgs, I think we will need this adaptor not only in simulation but also in real machine. So I think it would be better to put it in the control directory instead of the simulation directory.

M. Fatih Cırıt and others added 3 commits June 19, 2023 14:27
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@xmfcx xmfcx force-pushed the feat/autoware_auto_msgs_adapter branch from 0142948 to 66edca2 Compare June 19, 2023 11:36
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@github-actions github-actions bot added component:system System design and integration. (auto-assigned) and removed component:simulation Virtual environment setups and simulations. (auto-assigned) labels Jun 19, 2023
@xmfcx
Copy link
Contributor Author

xmfcx commented Jun 19, 2023

Thanks for the very good PR.

When we move from autoware_auto_msgs to autoware_msgs, I think we will need this adaptor not only in simulation but also in real machine. So I think it would be better to put it in the control directory instead of the simulation directory.

@yukkysaito thanks! I moved it to system directory instead: 4ba25b3

Reason is, this is not related to control only, it will probably adapt more message types from other components too.

I had 3 options:

  • common (they are mostly libraries)
  • system (default_ad_api is here too, picked this)
  • tools (felt like, this folder is not intended for tools that run in real time)

What do you think?

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@yukkysaito
Copy link
Contributor

@yukkysaito thanks! I moved it to system directory instead: 4ba25b3
Reason is, this is not related to control only, it will probably adapt more message types from other components too.

@xmfcx
Thank you! It looks good.

pre-commit-ci bot and others added 7 commits June 19, 2023 13:56
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Copy link
Contributor

@isamu-takagi isamu-takagi left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:system System design and integration. (auto-assigned) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a messages_adapter package
3 participants