-
Notifications
You must be signed in to change notification settings - Fork 56
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
base: main
Are you sure you want to change the base?
feat(autoware_sensing_msgs): implemented the proposed universal radar messages #120
Conversation
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
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? |
uint32 ANIMAL = 8 | ||
uint32 HAZARD = 9 | ||
uint32 OVER_DRIVABLE = 10 | ||
uint32 UNDER_DRIVABLE = 11 |
There was a problem hiding this comment.
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?
I am very surprised someone actually used 0->100 to represent probabilities. At least, any new implementation should have them normalized 0->1. |
There was a problem hiding this 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
- ensures that RadarInfo is received before any RadarObjects are handled
- 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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.