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

Standard proposal for marine radar messages. #4

Closed
wants to merge 6 commits into from

Conversation

rolker
Copy link
Member

@rolker rolker commented Jan 20, 2023

Work on achieving #1

Copy link

@peci1 peci1 left a comment

Choose a reason for hiding this comment

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

I left some comments regarding the documentation, but structure-wise, this proposal seems good to me.

# to be used separately.

float32[] echoes # Regularly spaced series of intensities from closest to
# farthest. Each array represents data from the same
Copy link

Choose a reason for hiding this comment

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

The wording here is a bit cumbersome. It talks about "each array", implying there are multiple arrays, but this message only contains one array. I guess it should be interpreted relative to the RadarSector message, but that information is missing here.

I would also add an explanation to the regular spacing. Definition of the spacing is also information to be extracted from the "parent" message, but it is not mentioned here.

#
# in frame frame_id, angles are measured around
# the positive Z axis (counterclockwise, if Z is up)
# with zero angle being forward along the x axis
Copy link

Choose a reason for hiding this comment

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

Please unify the case of the axis names: Z vs x. Choose one case (I'd prefer uppercase).

# This message is a submessage of RadarSector and is not intended
# to be used separately.

float32[] echoes # Regularly spaced series of intensities from closest to
Copy link

Choose a reason for hiding this comment

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

Here, the note about device-specific units from RadarSector should be repeated.

float32 angle_start # start angle of this sector [rad]
float32 angle_increment # angular distance between measurements [rad]

duration time_increment # time between rays [seconds] - if your scanner
Copy link

Choose a reason for hiding this comment

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

In angle_increment, you use "measurements" and here "rays". It might be good to unify this terminology.

float32 angle_increment # angular distance between measurements [rad]

duration time_increment # time between rays [seconds] - if your scanner
# is moving, this will be used in interpolating
Copy link

Choose a reason for hiding this comment

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

Suggested change
# is moving, this will be used in interpolating
# is moving, this can be used in interpolating

duration scan_time # time between two consecutive complete scans
# or revolutions [seconds]

# Warning: On hardware that doesn't provide a timestamp
Copy link

Choose a reason for hiding this comment

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

Suggested change
# Warning: On hardware that doesn't provide a timestamp
# Warning: On hardware that doesn't provide a timestamp,

Comment on lines +24 to +25
float32 range_min # range of first sample in each ray [m]
float32 range_max # range of last sample in each ray [m]
Copy link

Choose a reason for hiding this comment

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

Suggested change
float32 range_min # range of first sample in each ray [m]
float32 range_max # range of last sample in each ray [m]
float32 range_min # range of the first sample in each ray [m]
float32 range_max # range of the last sample in each ray [m]

Copy link

Choose a reason for hiding this comment

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

Also, "ray" vs. "measurement" vs. "intensities".

float32 range_min # range of first sample in each ray [m]
float32 range_max # range of last sample in each ray [m]

RadarEcho[] intensities # intensity data [device-specific units].
Copy link

Choose a reason for hiding this comment

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

Is there really no physically-sensible unit these values could be converted to? What are the units called in datasheet? dB, W, something else? I can also imagine converting the values to percentages of the emitted ray (log) power or something similar... Lidars usually report intensity as integers 0-100 or 0-255 (although I don't think this is standardized anywhere).

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there really no physically-sensible unit these values could be converted to? What are the units called in datasheet? dB, W, something else? I can also imagine converting the values to percentages of the emitted ray (log) power or something similar... Lidars usually report intensity as integers 0-100 or 0-255 (although I don't think this is standardized anywhere).

In the case of the radar I am using, the protocol was reversed-engineered (mostly by the OpenCPN community) so it's not clear what units are used.

@rolker
Copy link
Member Author

rolker commented May 2, 2023

Replaced by: apl-ocean-engineering/marine_msgs#40

@rolker rolker closed this May 2, 2023
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.

None yet

2 participants