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.

Sign up for GitHub

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

Jump to bottom

docs(design/autoware-interfaces): add sensing interface #328

Merged
merged 19 commits into from
Aug 2, 2023
Merged

docs(design/autoware-interfaces): add sensing interface #328

merged 19 commits into from
Aug 2, 2023

Conversation

beginningfan
Copy link
Contributor

Description

Add sensing interface tutorial in Autoware interfaces/Components page.

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 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.

beginningfan and others added 2 commits February 21, 2023 16:58
@mitsudome-r mitsudome-r added type:documentation Creating or refining documentation. component:sensing Data acquisition from sensors, drivers, preprocessing. labels Mar 7, 2023
@beginningfan
fix(design/autoware-interfaces): fix typo(date)
8d3d54f 
Signed-off-by: beginningfan <beginning.fan@autocore.ai>
@mitsudome-r mitsudome-r requested a review from kaancolak March 14, 2023 08:24
@beginningfan
docs(design/autoware-interfaces): delete input topics
c11c905 
Signed-off-by: beginningfan <beginning.fan@autocore.ai>
@kminoda kminoda added the tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) label Mar 27, 2023
@github-actions
Copy link

Documentation URL: https://autowarefoundation.github.io/autoware-documentation/pr-328/
Modified URLs:

kminoda
kminoda reviewed Mar 27, 2023
View reviewed changes
Copy link
Contributor

@kminoda kminoda left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Mostly seems good 👍
I left some minor comments.

docs/design/autoware-interfaces/components/sensing.md Outdated Show resolved Hide resolved
docs/design/autoware-interfaces/components/sensing.md Outdated Show resolved Hide resolved
docs/design/autoware-interfaces/components/sensing.md Outdated Show resolved Hide resolved
@beginningfan
docs(design/autoware-interfaces): Fix typos found by spell-check
ef0f252 
Signed-off-by: beginningfan <beginning.fan@autocore.ai>
@beginningfan
docs(design/autoware-interfaces): fix lidar output
8fbf1a3 
Signed-off-by: beginningfan <beginning.fan@autocore.ai>
kminoda
kminoda reviewed Apr 3, 2023
View reviewed changes
Copy link
Contributor

@kminoda kminoda left a comment

Choose a reason for hiding this comment

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

LGTM for the localization part (please get approval from perception point of view as well)

@beginningfan
docs(design/autoware-interfaces): add radar tracks
10e2db2 
Signed-off-by: beginningfan <beginning.fan@autocore.ai>
@liuXinGangChina
Copy link

LGTM for the localization part (please get approval from perception point of view as well)

Hi,mits-san can you help us assign a reviewer who know well about perception module? @mitsudome-r

mitsudome-r
mitsudome-r reviewed May 11, 2023
View reviewed changes
docs/design/autoware-interfaces/components/sensing.md Outdated

Tracks from radar. Used by the Perception.

- [radar_msgs/RadarTracks](https://docs.ros.org/en/noetic/api/radar_msgs/html/msg/RadarTracks.html)
Copy link
Member

Choose a reason for hiding this comment

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

I think we are still open with the message type. Let' make it TBD for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related discussion: https://github.com/orgs/autowarefoundation/discussions/2531

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [radar_msgs/RadarTracks](https://docs.ros.org/en/noetic/api/radar_msgs/html/msg/RadarTracks.html)
- [radar_msgs/RadarTracks](https://github.com/ros-perception/radar_msgs/blob/ros2/msg/RadarTracks.msg)

I will try to end the discussion, I think it's OK to use https://github.com/ros-perception/radar_msgs/tree/ros2/msg in the pipeline.

@xmfcx xmfcx self-requested a review May 17, 2023 13:23
@mitsudome-r
Copy link
Member

@yukkysaito Fatih suggested to make "Driver" a separate component from "Sensing". Do you have any comments on this change?

@xmfcx
Copy link
Contributor

xmfcx commented May 23, 2023 •
edited
Loading

@yukkysaito we can consider 3 options:

  • Option A:
    • Sensing and Driver components are separated.
    • Namespaces have following conventions:
      • /driver/velodyne_driver
      • /sensing/gnss_poser
  • Option B:
    • Sensing component has 2 subsections: Driver and Preprocessor
    • Namespaces have following conventions:
      • /sensing/driver/velodyne_driver
      • /sensing/preprocessor/gnss_poser
  • Option C:
    • Keep things as they are.
    • Namespaces have following conventions:
      • /sensing/velodyne_driver
      • /sensing/gnss_poser

What do you think?

@xmfcx
Copy link
Contributor

xmfcx commented May 23, 2023

I've done some simple analysis with ChatGPT:

Proposal Analysis: Autoware Component Organization

In the proposal, three options are presented for organizing the Autoware components related to sensing and driver functionality. Let's analyze each option and provide reasoning behind them:

Option A: Separated Sensing and Driver Components

  • Sensing and Driver components are separated.
  • Namespaces follow the conventions: /driver/velodyne_driver and /sensing/gnss_poser.

Reasoning:
This option provides a clear separation between the sensing and driver components, allowing for independent development and maintenance. It can promote modularity and reusability since different sensor drivers can be developed separately and easily integrated into the sensing component. The namespaces provide a clear distinction between the two functionalities.

Option B: Sensing Component with Driver and Preprocessor Subsections

  • Sensing component has two subsections: Driver and Preprocessor.
  • Namespaces follow the conventions: /sensing/driver/velodyne_driver and /sensing/preprocessor/gnss_poser.

Reasoning:
This option introduces a hierarchical structure within the sensing component by dividing it into driver and preprocessor subsections. The driver subsection focuses on interfacing with sensors, while the preprocessor subsection handles initial data processing before passing it to perception, prediction, and planning components. This approach can provide a more organized and modular design, enabling better encapsulation of sensor-specific functionalities.

Option C: Keeping the Current Organization

  • Sensing and Driver components are combined.
  • Namespaces follow the conventions: /sensing/velodyne_driver and /sensing/gnss_poser.

Reasoning:
This option maintains the current organization where the sensing and driver functionalities are combined within the same component. It follows the convention of using namespaces to differentiate between different sensor functionalities. However, it may lead to a less modular design since the sensing component becomes responsible for both interfacing with sensors and initial data processing.

Considering the proposals, both Option A and Option B offer advantages in terms of modularity and separation of concerns. Option A provides a cleaner separation between the sensing and driver functionalities, while Option B introduces a hierarchical structure within the sensing component. The choice between these options depends on the specific requirements and goals of the Autoware project.

If modularity and independent development of sensor drivers are important, Option A would be a suitable choice. On the other hand, if a more organized structure within the sensing component is desired, Option B can provide better encapsulation of driver and preprocessor functionalities.

Option C, maintaining the current organization, may be appropriate if there are no immediate concerns regarding modularity and hierarchical structuring. However, it might limit flexibility and make it harder to evolve the system in the future.

@xmfcx
Copy link
Contributor

xmfcx commented May 23, 2023

From analysis, I would go with the

  • Option A:
    • Sensing and Driver components are separated.
    • Namespaces have following conventions:
      • /driver/velodyne_driver
      • /sensing/gnss_poser

xmfcx
xmfcx reviewed May 23, 2023
View reviewed changes
docs/design/autoware-interfaces/components/sensing.md Outdated

Image data from camera. Used by the Perception.

- [sensor_msgs/Image](https://docs.ros.org/en/noetic/api/sensor_msgs/html/msg/Image.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [sensor_msgs/Image](https://docs.ros.org/en/noetic/api/sensor_msgs/html/msg/Image.html)
- [sensor_msgs/Image](https://github.com/ros2/common_interfaces/blob/rolling/sensor_msgs/msg/Image.msg)

Can you update this and following messages to use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote this because other documents use noetic message type links, such as control

I will update all messages in sensing document soon.

@yukkysaito
Copy link
Contributor

@yukkysaito we can consider 3 options:

  • Option A:

    • Sensing and Driver components are separated.

    • Namespaces have following conventions:

      • /driver/velodyne_driver
      • /sensing/gnss_poser

  • Option B:

    • Sensing component has 2 subsections: Driver and Preprocessor

    • Namespaces have following conventions:

      • /sensing/driver/velodyne_driver
      • /sensing/preprocessor/gnss_poser

  • Option C:

    • Keep things as they are.

    • Namespaces have following conventions:

      • /sensing/velodyne_driver
      • /sensing/gnss_poser

What do you think?

@xmfcx @mitsudome-r
There are several reasons, but I prefer B or C.
Please wait a moment as I put together my reasons.

If we choose B, I want to add prefix camera or lidar namespace before driver or preprocess.
example:

/sensing/lidar/driver/velodyne_driver
/sensing/gnss/preprocessor/gnss_poser

@yukkysaito
Copy link
Contributor

@xmfcx @mitsudome-r
There are several reasons, but I prefer B or C.
Please wait a moment as I put together my reasons.

The reasons for B and C are as follows.

  • Currently, the sensing component also includes a driver node.
  • Separating the driver and the sensing component is too small compared to other components.
  • Separating the driver and the sensing component will make the actual sensing component almost nonexistent. It will be as small as lidar and gnss preprocess component.
  • Even if the driver and the sensing component are separated, the configuration may become tightly coupled.

@liuXinGangChina
Copy link

Hi, Mits-san @mitsudome-r ,our engineer has revise this pr accroding to above discussion result. Please help us review this one.

@beginningfan
Copy link
Contributor Author

@mitsudome-r I have checked the result of CI. The dead links and nonsense string are not in the file I submitted or in the .svg file. If there are no other problems, please merge it.

@mitsudome-r
Copy link
Member

@beginningfan thanks for the update.
Could you check #328 (comment)? Once the thread is resolved, I think we can merge the PR.

@mitsudome-r mitsudome-r mentioned this pull request Jun 21, 2023
Merged
4 tasks
@mitsudome-r mitsudome-r merged commit 2b4da3f into autowarefoundation:main Aug 2, 2023
alanmengg pushed a commit to alanmengg/autoware-documentation that referenced this pull request Aug 2, 2023
@beginningfan @pre-commit-ci @alanmengg
docs(design/autoware-interfaces): add sensing interface (autowarefoun…
fa99850 
…dation#328)

* docs(design/autoware-interfaces): add sensing interface

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* style(pre-commit): autofix

* fix(design/autoware-interfaces): fix typo(date)

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces): delete input topics

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces): Fix typos found by spell-check

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces):fix description of vehicle velocity and pose

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces): fix lidar output

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces): add radar tracks

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces): Modify the repo of msg type

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces): add driver related content

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* style(pre-commit): autofix

* docs(design/autoware-interfaces): add radar description

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* docs(design/autoware-interfaces): replace svg image with mermaid code

Signed-off-by: beginningfan <beginning.fan@autocore.ai>

* style(pre-commit): autofix

---------

Signed-off-by: beginningfan <beginning.fan@autocore.ai>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: guiping meng <alan.meng@autocore.ai>
@tier4-autoware-public-bot tier4-autoware-public-bot bot mentioned this pull request Aug 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kminoda kminoda kminoda left review comments

@scepter914 scepter914 scepter914 left review comments

@YamatoAndo YamatoAndo YamatoAndo left review comments

@xmfcx xmfcx xmfcx left review comments

@yukkysaito yukkysaito yukkysaito left review comments

@aohsato aohsato aohsato left review comments

@mitsudome-r mitsudome-r mitsudome-r approved these changes

@miursh miursh Awaiting requested review from miursh

@kaancolak kaancolak Awaiting requested review from kaancolak

Assignees
No one assigned
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) type:documentation Creating or refining documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@beginningfan @liuXinGangChina @mitsudome-r @xmfcx @yukkysaito @aohsato @kaancolak @YamatoAndo @scepter914 @kminoda