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(autoware_sensing_msgs): implemented the proposed universal radar messages #120

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

knzo25
Copy link

@knzo25 knzo25 commented Mar 3, 2025

Description

This PR implemented the universal radar messages discussed in
https://github.com/orgs/autowarefoundation/discussions/5264

How was this PR tested?

Used together with tier4/nebula#284
to check the field contents

Notes for reviewers

None.

Effects on system behavior

None.

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Copy link

github-actions bot commented Mar 3, 2025

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@knzo25 knzo25 requested review from YoshiRi, drwnz and technolojin March 3, 2025 05:18
@knzo25 knzo25 self-assigned this Mar 3, 2025
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Copy link
Author

knzo25 commented Mar 3, 2025

Reference: https://github.com/autowarefoundation/autoware_msgs/blob/main/autoware_perception_msgs/msg/DetectedObject.msg

The one think I am over the fence on is about whether we should normalize the probabilities for [0.0, 1.0] or keep them [0.0, 100.0]. How do you guys think?

@technolojin technolojin requested a review from xmfcx March 4, 2025 01:08
Comment on lines +12 to +15
uint32 ANIMAL = 8
uint32 HAZARD = 9
uint32 OVER_DRIVABLE = 10
uint32 UNDER_DRIVABLE = 11
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about updating https://github.com/autowarefoundation/autoware_msgs/blob/main/autoware_perception_msgs/msg/ObjectClassification.msg
list with these items? We could avoid creating a new classification message and duplicating information.

Also, what are hazard, over drivable, under drivable? And why are they radar specific?

@xmfcx
Copy link
Collaborator

xmfcx commented Mar 5, 2025

The one think I am over the fence on is about whether we should normalize the probabilities for [0.0, 1.0] or keep them [0.0, 100.0]. How do you guys think?

I am very surprised someone actually used 0->100 to represent probabilities. At least, any new implementation should have them normalized 0->1.

Copy link

@mojomex mojomex left a comment

Choose a reason for hiding this comment

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

It's probably worth it to write have a small support library that

  1. ensures that RadarInfo is received before any RadarObjects are handled
  2. provides accessors for RadarObject fields that auto-convert/validate using the received RadarInfo

It seems quite easy to screw up as a user of these messages otherwise - e.g. orientation, is it deg, rad, something else?

@@ -0,0 +1,35 @@
float32 INVALID_COV_VALUE = 100000000.0
Copy link

Choose a reason for hiding this comment

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

Instead of a dummy value we should most probably use NaN

uint8 MEASUREMENT_STATUS_PREDICTED = 1
uint8 MEASUREMENT_STATUS_NEW = 2
uint8 MEASUREMENT_STATUS_UNKNOWN = 3
uint8 MEASUREMENT_STATUS_INVALID = 255
Copy link

Choose a reason for hiding this comment

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

Not sure if these values are chosen for backward compatibility, but e.g. Protobuf recommends to make 0 the invalid option such that any zero-initialized message is invalid by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants