ros-perception / radar_msgs

A set of standard messages for RADARs in ROS
Apache License 2.0
25 stars 5 forks source link

Message format for RadarPoint #1

Closed ghost closed 4 years ago

ghost commented 4 years ago

The proposed message type for a RadarPoint.

Prior to proposing this format, research was done into the radar data message formats that exist out in the wild. The details of this analysis can be found here: https://github.com/radarAaron/radar_msgs/blob/master/ROS%20Message%20format%20comparison.xlsx

Summary

Only two drivers appear to output a 'raw' point format.

Position/Coordinate system One driver outputs in polar coordinates while the other outputs both polar and Cartesian format

Speed Both drivers output speed. One is called "velocity" and the other "doppler_alias" (both are float 32)

Intensity Both drivers output intensity. One driver calls this "intensity" and the other "sensor_nr" (noise ratio)

Additional fields Drivers also include the following fields: usage, target_format_type, msg_counter

Conclusion The proposed format appears to be sufficiently in line with the existing drivers.

Questions

SteveMacenski commented 4 years ago

@radarAaron can you add the analysis for each specific message in the PR description (# packages looked at, what they had in common or what they did otherwise, the stuff you posted in slack)? And our rationale about why "this" over "that"?

That will let me then post these 3 PRs in a grouping on Discourse to get community feedback / buy-in on these messages. We should explain why we did "this" over "that" and how our message is suitable for the vast majority, if not all, radar messages to be the new standard.

SteveMacenski commented 4 years ago

Because this is the "raw data" output from the sensor, i think we should follow the sensor conventions that are also smaller messages which can be beneficial. Folks working with this level of radar data would probably want to have it in this format so you can have some more direct awareness of where in the antenna the return came from, in case that's helpful in filtering/structured algorithms

SteveMacenski commented 4 years ago

RadarReturn

So we at one point were doing RadarRaw, and Aaron rightfully corrected me that this isn't really "raw" for people doing signal processing. I would be OK with RadarReturn but I'm going to yield to Aaron for best practices.

RadarScan to parallel our LaserScan

I really like that, any objections?

frankeverdij commented 4 years ago

The ROS driver for the UMRR-11 and UMRR-96 automotive radars from SmartMicro is not included in the overview. It is CAN based, but there is a JSON file which list 'Range', 'Azimuth', 'Speed_Radial', RCS' and 'Elevation' as fields. I couldn't find any further information on these fields, but the driver is available at smartmicro.de as well as data sheets for the individual radar types.

As for the RadarPoint message proposal, i am in favor of the polar coordinate system for representing a single radar reflection.

The intensity in radar systems we came across, mostly Continental ARS ones but also this UMRR, is given as an RCS (radar cross section) value in dB which is a float32, not uint8. The naming of this field seems to vary between 'intensity', 'snr' and 'RCS'. Since it depends on the specific radar used, its interpretation may vary, but i would recommend it not to be called 'intensity' to avoid mixup with LaserScan sensor measurements. 'snr' or 'RCS' would be more suitable, in my opinion.

I also am in favor of the 'azimuth' naming over 'angle' for the horizontal angle between the radar point vector and the normal vector of the radar.

SteveMacenski commented 4 years ago

@frankeverdij please provide a link to this ROS driver.

Sounds like we're all in agreement on azimuth and RadarReturn changes. More discussion required on the intensity / similar parameter naming and type.

SteveMacenski commented 4 years ago

@tfoote @mjcarroll can you take another sweep through? I think this resolved all your concerns and mine.

The only ? is the amplitude and RCS thing which I think is resolved now, but @frankeverdij will need to chime in.

If we get a consensus, then we can merge this and move onto the Detection message

frankeverdij commented 4 years ago

The only ? is the amplitude and RCS thing which I think is resolved now, but @frankeverdij will need to chime in.

The name 'Amplitude' and its type of float32 is fine with me. It is certainly more descriptive than 'RCS' , for it being an acronym. An issue might be that radars outputting intensity values between 0 and 255 for each detection either requires the interface driver to convert these values in dB, or simply put the values in the 'Amplitude' field as float32. This could be indicated by an extra flag, but i don't know if this is a big issue which merits such an addition.

SteveMacenski commented 4 years ago

@frankeverdij so do you approve of this as is? If so, can you hit the approve check mark so we can get consensus? I'd like at minimum 3-4 approvals from different groups to make sure we have a good cross section of the community.

We appreciate your time in helping work through these issues!

frankeverdij commented 4 years ago

You're welcome. I have submitted my approval for this thread. Do i need to approve the change request below as well? If so, a quick help on how to do this would be most welcome.

SteveMacenski commented 4 years ago

I'm not entirely sure what you mean by approve change request below

frankeverdij commented 4 years ago

I'm not entirely sure what you mean by approve change request below

Disregard my question. I am not too familiar with github's discussion interface so i wasn't sure if i missed a checkmark somewhere. Everything looks good from my side.

ghost commented 4 years ago

I think that is all the suggested changes made. Last call for changes before we commit this.

shrijitsingh99 commented 4 years ago

Looks good to me 👍

SteveMacenski commented 4 years ago

That's now 5 independent approvals, merging