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.
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?
There was a problem hiding this comment.
@xmfcx
Thank you for the comments.
I agree with duplicating information, but using perception msgs in the sensing package seems unnatural. Would you prefer moving the ObjectClassification message to sensing ? using perception msgs as a dependency in sensing ? moving it outside to a common one?
The radar interfaces that I saw define them, but there is nothing that explains them. We could omit them, and update the universal interface. AFAIK:
- hazard: unknown but that is dangerous for AD
- overdrivable: leaves, plstic bags, etc
- underdrivable: overpasses (?)
There was a problem hiding this comment.
This makes me consider even merging sensing and perception messages under perception as a general category.
Because sensing layer is doing some of the perception tasks here.
@mitsudome-r what do you think?
We don't have many sensing messages too, do you think these could be merged?
Also I see that https://github.com/tier4/tier4_autoware_msgs there is no tier4_sensing_msgs too. So I'd suggest merging.
There was a problem hiding this comment.
Even for perception point-of-view, I think those new classes are necessary. Current perception do not distinguishes obstacles can be ignored or not ignored for driving, but just classified as "Unknown".
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.
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.
Instead of a dummy value we should most probably use NaN
There was a problem hiding this comment.
We discussed this internally, and the architects decision was not to use NaNs or infs
There was a problem hiding this comment.
So, if the user of these messages makes a mistake, the decision is to produce values that are valid numbers in a floating-point sense (not NaN) but invalid in a semantic sense (nonsensical value). This rids us of any chance to detect faulty assumptions about the content of the messages.
What were the reasons for this choice?
There was a problem hiding this comment.
| float32 INVALID_COV_VALUE = 100000000.0 | |
| float32 INVALID_COV_VALUE = -1.0f; |
https://en.wikipedia.org/wiki/Covariance_matrix
Any covariance matrix is symmetric and positive semi-definite and its main diagonal contains variances (i.e., the covariance of each element with itself).
And since the message contains the diagonal of this matrix in the arrays below, we could make the invalid value -1.0f.
There was a problem hiding this comment.
@xmfcx That's at least a value that is not semantically valid. I still don't get the aversion to using NaN as one of the many reasons for its existence is exactly this use case: to mark invalid entries.
There was a problem hiding this comment.
I'd guess it's because of varying definitions of inf and nans across multiple systems.
| 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.
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.
| geometry_msgs/Point position | ||
| geometry_msgs/Vector3 velocity | ||
| geometry_msgs/Vector3 acceleration | ||
| geometry_msgs/Vector3 shape |
There was a problem hiding this comment.
Is shape means dimension of 3d bounding box?
There was a problem hiding this comment.
Yes. That being said, the perception package has has one that is more general https://github.com/autowarefoundation/autoware_msgs/blob/main/autoware_perception_msgs/msg/Shape.msg
In the past I though that is was more inefficient (more bloat), but it seems that is not the case.
However, Shape is in the perception package. Assuming that radars can really only detect bboxes, would you prefer to keep it as is (with a better name like box_shape) or reuse Shape by redefining the message in the sensing package?
There was a problem hiding this comment.
If the message only support bbox, just rename the vector3 "shape" to "size" or "dimensions" or something else would be enough.
There was a problem hiding this comment.
Nice!
This has changed a bit from the proposal, with the generalization of the field info being the biggest difference I can see. While this is clean, I wonder if it is good to explicitly define the info fields in the radar info message.
Another comment - whether to not this ends up being the catalyst to merge sensing and perception messages, these are quite complex message definitions so adding an explanation for the new types in the README would be very much appreciated (once the review of the contents themselves is complete).
While we don't use the radar detections currently, would it be worth defining it to make the info fields easier to understand/use in driver implementation?
| @@ -0,0 +1,8 @@ | |||
| std_msgs/String field_name | |||
| bool min_value_available | |||
There was a problem hiding this comment.
For the availability bools, I remember there was a discussion about using NaN (I guess the above suggestion by @xmfcx , using -1.0f may work for resolution, but it's a bit dangerous as requires the user to check an apparently irrelevant field for availability). I'm guessing having the bools as proposed is cleaner?
There was a problem hiding this comment.
Another way of doing this would be to use a field like
float32[<=1] min_value
which is functionally equivalent to an std::optional<float32>.
This would remove the need for the boolean field, and the user is forced to check if the value is present. Probably a bit ugly to use since it shows up as a dynamic bounded array, not an optional 🥲
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.