Change unit of amplitude #17
Conversation
msg/RadarReturn.msg
Outdated
| @@ -7,4 +7,4 @@ float32 azimuth # Angle (in radians) in the azimuth pla | |||
| float32 elevation # Angle (in radians) in the elevation plane between the sensor and the detected return. | |||
| # Negative angles are below the sensor. For 2D radar, this will be 0. | |||
| float32 doppler_velocity # The doppler speed (m/s) of the return. | |||
| float32 amplitude # The amplitude of the return (dB). | |||
| float32 amplitude # The radar cross section of the return (RCS linear/m^2). | |||
There was a problem hiding this comment.
I would generally reject a unilateral change in units of a ROS message without much further discussion from other users of the message. Please motivate this change in detail and "@" copy the other contributors to this repository for review
There was a problem hiding this comment.
This are 2 changes that we can also separate in the discussion.
- The current definition is missing what the value of
amplituderelates to.
dB only specified the scale, not the unit.
Hence it is under-specified and would result in a sensor specific scale.
It would be better to describe the scene, and not the sensor-specific amplitude value. - The second change is, that we would propose to change the scale from logarithmic to linear.
Since the sensor would measure it linear and most (or all?) calculations would also require a linear value.
To calculate RCS a reference measurement of an object with a known RCS is required.
For example a metal ball, where the RCS is equal to the geometric cross section.
(Hence it can also be done if the manufacturer doesn't provide this value.)
It can then be calculated by
RCS = (intensity[measurement] / intensity[referenceObject]) * (distance[referenceObject]^2 / distance[measurement]^2) * CrossSectionalArea[referenceObject]
There was a problem hiding this comment.
@aaronfultonnz, @fred-apex-ai, @icolwell-as and @radarAaron please feel free to contribute to this discussion.
There was a problem hiding this comment.
@tobiasneumann I'm not an expert here, so I don't provide a conclusion and invite What is @radarAaron @icolwell-as @aaronfultonnz device-specific comments
The current definition is missing what the value of amplitude relates to.
dB only specified the scale, not the unit.
What @tobiasneumann suggests would also require renaming the field from amplitude to something like rcs. This would be a breaking change. An inconvenience to any consumer of this package, but useful to signal that something significant has changed. If you want to change the meaning of a field through a comment, the compiler will not flag the change for legacy uses and you risk having a legacy publisher and a new subscriber disagreeing on the meaning of a field, which we should avoid.
The second change is, that we would propose to change the scale from logarithmic to linear.
Since the sensor would measure it linear and most (or all?) calculations would also require a linear value.
I just want to add that for the one radar sensor I worked with, the device returns a cluster RCS with unit dBm². Given that, the logarithmic scale seems more natural to describe this sensor's output.
I personally don't know if it's more useful to have a value on the log or linear for consumers of this message
There was a problem hiding this comment.
I don't have too much to add, as I haven't directly worked with radars for a while now, and I'm definitely not up-to-date on the latest radar hardware.
That being said, I can provide some info based on my automotive radar experience:
- Delphi/Aptiv MRR & ESR: These radars are quite old now, but still fairly common in R&D and consumer vehicles. They provide the simple dB field, which was the original inspiration for the unit of this
amplitudefield. These radars do not provide RCS. - Continental ARS 408: This radar is the default forward-facing radar for use with Baidu's Apollo. According to this, RCS is available from the radar.
- We've also used SmartMicro radars in the past couple years, and RCS does seem available across most, if not all, of their radars. They have an open-source ROS driver: https://github.com/smartmicro/smartmicro_ros2_radars/tree/master
Aside from the API change pains, I do understand the need for a better field with clearer units. Would introducing a new field and leaving the existing amplitude field only for legacy use be an option?
There was a problem hiding this comment.
@tobiasneumann Thanks for giving this so much thought. I agree with your reasoning, and option one seems best but we need to clarify what happens if we the sender and receiver have a mismatch on the message definition. Would they notice with a clear message? I guess there wouldn't be a segfault as the message size remains the same.
There was a problem hiding this comment.
2 Points regarding this.
- ROS has some mechanism to warn about different Versions of messages.
But I only know about that it exists, not what exactly is been checked. - This is just 1 out of 4 changes, that are been requested.
I would think it would be best to discuss all of them, before a new version of this package is been released (but there I don't know the workflow for ROS)
And the other changes are less complicated/dangerous, but change the fields more extensively.
There was a problem hiding this comment.
@tobiasneumann You are right, if we introduce a breaking change, we better look at all changes holistically. @SteveMacenski has asked for a new maintainer on https://discourse.ros.org/t/radar-messages-call-for-maintenance-help/34560. If your PRs can wait a bit, I suggest we wait a bit more and see who wants to take over maintenance, then roll out a new version with the new maintenance team while @SteveMacenski is still involved to have a proper handover.
There was a problem hiding this comment.
Hello everyone,
we would support the following proposal from @tobiasneumann
Taking all this in consideration I would propose a different solution.
- Changing the field name from
amplitudetorcs.- Using a logarithmic scale (
dBm^2), instead of the currently proposed linear one.
There was a problem hiding this comment.
I've adapted the MR to ☝️ change.
Does this resolve this discussion?
tobiasneumann
force-pushed
the
change-unit-for-amplitude-to-RCS
branch
from
66cb4ba to
ed437dc
Compare
The description for the field
amplitudeis missing the relative information of the measurement.Proposed change
(Please note that this also changes the logarithmic value to a linear one.)