refactor(lidar_apollo_segmentation_tvm/_nodes): remove autoware_auto_common dependency

[takayuki5168]

Signed-off-by: Takayuki Murooka takayuki5168@gmail.com

Description

related to #2229
remove autoware_auto_common from lidar_apollo_segmentation_tvm/_nodes

Pre-review checklist for the PR author

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

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

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.

[takayuki5168]

@yukke42 @yukkysaito
I'm not familiar with these packages, but I wanna remove autoware_auto_common which is autoware.auto's common package.
For this reason, I created this PR. Could you check this PR please.

[yukkysaito]

@takayuki5168 I agree with the policy. There must be a reason why it was introduced and I would like to know.
@esteve Do you know the background and benefits of the introduction of these types?

[codecov]

Codecov Report

Base: 12.29% // Head: 10.88% // Decreases project coverage by -1.41% ⚠️

Coverage data is based on head (3f8bc4a) compared to base (a7e322d).
Patch has no changes to coverable lines.

❗ Current head 3f8bc4a differs from pull request most recent head d7f9531. Consider uploading reports for the commit d7f9531 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2234      +/-   ##
==========================================
- Coverage   12.29%   10.88%   -1.42%     
==========================================
  Files        1171     1179       +8     
  Lines       82644    84935    +2291     
  Branches    23314    20043    -3271     
==========================================
- Hits        10162     9242     -920     
- Misses      61711    65931    +4220     
+ Partials    10771     9762    -1009     

Flag	Coverage Δ		*Carryforward flag
differential	∅ <ø> (?)
total	10.85% <ø> (-1.44%)	⬇️	Carriedforward from 4c0f75f

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

Impacted Files	Coverage Δ
...vehicle_model/vehicle_model_bicycle_kinematics.hpp	0.00% <0.00%> (-100.00%)	⬇️
vehicle/raw_vehicle_cmd_converter/src/pid.cpp	0.00% <0.00%> (-85.72%)	⬇️
...clude/obstacle_avoidance_planner/mpt_optimizer.hpp	0.00% <0.00%> (-80.00%)	⬇️
...nner/src/vehicle_model/vehicle_model_interface.cpp	0.00% <0.00%> (-75.00%)	⬇️
...onverter/include/raw_vehicle_cmd_converter/pid.hpp	0.00% <0.00%> (-66.67%)	⬇️
...vehicle_model/vehicle_model_bicycle_kinematics.cpp	0.00% <0.00%> (-52.18%)	⬇️
...g/obstacle_avoidance_planner/src/mpt_optimizer.cpp	0.00% <0.00%> (-43.55%)	⬇️
...include/obstacle_avoidance_planner/utils/utils.hpp	0.00% <0.00%> (-36.48%)	⬇️
...planner/src/lanelet2_plugins/utility_functions.cpp	0.00% <0.00%> (-29.04%)	⬇️
...n_planner/src/lanelet2_plugins/default_planner.cpp	0.00% <0.00%> (-24.60%)	⬇️
... and 920 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[ambroise-arm]

It comes from https://gitlab.com/autowarefoundation/autoware.auto/AutowareAuto/-/issues/99

[kenji-miyake]

Regarding this kind of code, let's refer to https://www.autosar.org/fileadmin/user_upload/standards/adaptive/21-11/AUTOSAR_RS_CPP14Guidelines.pdf and follow the latest recommendation.

https://autowarefoundation.github.io/autoware-documentation/main/contributing/coding-guidelines/languages/cpp/

[takayuki5168]

@kenji-miyake Could you give me more hint of how to refer to the guideline. (-> Explained in a chat.)

FYI: There is no behavior change with this PR since bool8_t, float32_t, float64_t is defined as bool, float, double respectively here.

autoware.universe/common/autoware_auto_common/include/common/types.hpp

Lines 39 to 47 in 7829c07

	using bool8_t = bool;
	using char8_t = char;
	using uchar8_t = unsigned char;
	// If we ever compile on a platform where this is not true, float32_t and float64_t definitions
	// need to be adjusted.
	static_assert(sizeof(float) == 4, "float is assumed to be 32-bit");
	using float32_t = float;
	static_assert(sizeof(double) == 8, "double is assumed to be 64-bit");
	using float64_t = double;

test

[xmfcx]

(Please ignore my review, it was a dummy test)

@kenji-miyake Could you give me more hint of how to refer to the guideline. (-> Explained in a chat.)

@takayuki5168 I've opened up the following discussion to talk about it: https://github.com/orgs/autowarefoundation/discussions/3016

[ambroise-arm]

I feel like this PR is a step backwards if long term we want to indeed have fixed-width types.
Also, I don't understand the rational of removing the common package just because it is an autoware.auto package. It could simply be renamed.
And as for the "annoying dependencies" it creates, isn't it the case for any common package that aims at being used in several other packages in order to de-duplicate code?

In any case, those are not strongly held opinions and I don't oppose going ahead with the change.

[xmfcx]

@ambroise-arm

I don't understand the rational of removing the common package just because it is an autoware.auto package. It could simply be renamed.

It's not because it is an Autoware.Auto package, it's because it's a dependency. In https://github.com/orgs/autowarefoundation/discussions/3016 we've discussed if we should create a similar package for types in Autoware Core.

I feel like this PR is a step backwards if long term we want to indeed have fixed-width types.

As discussed in https://github.com/orgs/autowarefoundation/discussions/3016 , here are some conclusions:

• Autoware doesn't require to be MISRA/AUTOSAR compliant in near future.
• We will encourage devs to use <cstdint> for integer types. And will try to add automatic checking features to CI to fix them automatically.
• When C++23 comes out and a ROS version supporting it comes out, we will use <stdfloat>

As for typedef bool bool8_t and typedef char char8_t I think we can discuss it further.

And as for the "annoying dependencies" it creates, isn't it the case for any common package that aims at being used in several other packages in order to de-duplicate code?

Since types are the most essential things, it needs to be included in every package. We will of course have these kind of things if they are necessary. But what we are discussing is about whether they are necessary or not for the current state of the Autoware, the users etc.

Please add your inputs to the discussion as well.

[ambroise-arm]

It's not because it is an Autoware.Auto package, it's because it's a dependency.

I may have misunderstood the meaning of the initial comment #2234 (comment). Alright then.

We will encourage devs to use <cstdint> for integer types.

I had missed that the PR only replaced floatXX_t and bool8_t. Although it should also replace char8_t by char. It probably won't compile in the current state (not picked up in CI as the package doesn't get compiled by default).

[xmfcx]

Should be removed too as pointed out by @ambroise-arm in #2234 (comment)

[takayuki5168]

d7f9531
DONE

[ambroise-arm]

using autoware::common::types::char8_t; is the issue here, not the white line, as it is a autoware_auto_common dependency
After removing this line, the char8_t in the file should then be replaced by char as mentioned in #2234 (comment)

[yukkysaito]

@takayuki5168 Can you resolve some conflicts?

[takayuki5168]

@yukkysaito @xmfcx I resolved the conflicts.

[yukkysaito]

LGTM

[ambroise-arm]

using autoware::common::types::char8_t; is the issue here, not the white line, as it is a autoware_auto_common dependency
After removing this line, the char8_t in the file should then be replaced by char as mentioned in #2234 (comment)

[esteve]

@esteve Do you know the background and benefits of the introduction of these types?

Sorry for the slow response, my notifications were not working properly. The reason for these types is mostly MISRA, because the built-in types don't have a defined size across all platforms, so by defining these aliases we can see quickly what size they have.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity.

[xmfcx]

@takayuki5168 I've moved this branch to AWF organization to collaborate easier. And made some fixes.

New PR: #3136

In order to build it, you need to run colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DDOWNLOAD_ARTIFACTS=1 --packages-select lidar_apollo_segmentation_tvm lidar_apollo_segmentation_tvm_nodes tvm_utility

Without the --cmake-args -DDOWNLOAD_ARTIFACTS=1 argument, it will skip building the package.

[takayuki5168]

@xmfcx Sorry about not handling this PR. I really appreciate your help.