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

Acoustic msg devel #20

Merged

Conversation

CaptKrasno
Copy link
Collaborator

@CaptKrasno CaptKrasno commented Feb 22, 2022

Hi all,

I implemented some of the changes to Multibeam Related messages we discussed at our last meeting.

In addition, I have created a new repository for Acoustic Message tools. To get things started I added a water column viewer. you can find the repo here: Acoutic Messages Tools
wc_gui2-2022-02-22_15 21 30

I have converted some logged water column data to the new format. You can find it here if you want to look at it or try the viewer. You can download that here: https://next.krasno.cloud/s/RnaGswoDbrZWkqQ

After this pull request is accepted I will fully support the new message types with my Norbit MBES driver, open source it and make it available for use.

It's worth mentioning how the data is packed in the Watercolumn message as it may not be obvious.

  • the actual pixel data is in row-major (i.e beam_no major) order.
  • the pixel data is stored as an array of bytes (uint8). It should be cast as the type specified in dtype flag. They are enumerated in the message
  • to find the range for each cell: (sample_number + sample0) * sound_speed / (2.0 * sample_rate)

Hopefully the image below clarifies how things are stored.

wcl_def

@CaptKrasno
Copy link
Collaborator Author

@valschmidt @lauralindzey do you guys have any feedback on this? I would like to get it changed or merged as necessary ASAP and it is required for upcoming projects.

# especially in deep water.
std_msgs/Header header

float32 sound_speed # m/s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment that this is sound speed typically measured at the head, not to be confused with an average watercolumn sound speed for example.

#
# header.stamp: TX cycle starts
# Each beam's TX time: header.stamp + txDelay[i]
# Each beam's RX time: header.stamp + txDelay[i] + twowayTravelTime[i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace txDelay[i] and twowayTravelTime[i] in comments with message member names? transmit_delay[i] and i/sample_rate?

@rolker
Copy link
Collaborator

rolker commented Apr 21, 2022

Seems like the only ways I can see to determine beam count are to look at the size of rx_angles or the size of image.data and account for data type and sampes_per_beam.

Should we add a beam_count member or at document the prefered method of determining the number of beams?

@CaptKrasno
Copy link
Collaborator Author

I think we had one in there initially and decided to take it out during our phone conversation. I wouldn't be opposed to adding it back, it is convenient. Maybe add it to the SonarImageData sub message? That way a SonarImageData could be parsed without the wrapper Datatype.

-Kris

@rolker
Copy link
Collaborator

rolker commented Apr 21, 2022

Yeah, adding it to SonarImageData does make sense. As is, it can't be processed independently.

@lauralindzey
Copy link
Collaborator

@CaptKrasno -- any chance that you could make the minor revisions that are still outstanding on this PR, and then we can merge it?

I'm currently installing your Norbit driver on NUI, and am eager to use these messages for our cruise =)

# Sonar reported -3db beamwidths
# May be empty if not reported
# reported in radians
float32[] tx_beamwidths
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CaptKrasno -- are you willing to add a note describing these in a bit more detail? e.g. a roboticist wanting to get started using these may not know how transmit/receive beamforming is performed, and thus won't know which axes these correspond to.

@@ -15,31 +15,7 @@ std_msgs/Header header
# than one thing to flag beams as bad. To check if a beam is good, simply
Copy link
Collaborator

Choose a reason for hiding this comment

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

(comment for line 5; it wouldn't let me select it ...)

Do you want to add attributions to the other institutions who have worked on this, or just delete this line?
(I'm ambivalent, but as it stands now, it's inaccurate)


# Truly raw multibeam data uses travel times rather than ranges.
float32[] two_way_travel_time
DetectionFlag[] flags
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, this comment is for a different line that Github wouldn't let me select. GRRR.

Did you intentionally leave PingInfo out of this message, or was that just an oversight?

@lauralindzey
Copy link
Collaborator

Also, I just want to say, that after not thinking about these messages for a while and then coming back, I REALLY like how you've laid them out. It makes sense to me, and works for both of the multibeam projects I have right now. (Imaging sonar reconstruction with an Oculus, and mapping with a Norbit.)

@rolker
Copy link
Collaborator

rolker commented May 5, 2022

I second Laura's friendly nudge to have this merged in before this cruise gets underway. I'll be using the format to collect EK80 data from Drix and hopefully from Drix's other sonars as well, so now's a great time to freeze this message format.

@lauralindzey
Copy link
Collaborator

Also, just as a FYI, I'm working on a PR to your norbit driver using these message formats.
I'm trying to get it all done tonight so we can test it during tomorrow's NUI dunk test.

(Yes, I'm kicking myself for not pushing to get this merged earlier and giving you the chance to do that work, but oh well ...)

@rolker
Copy link
Collaborator

rolker commented May 5, 2022

Another clarification we could make in the comments is about the tx_delays and tx_angles arrays. Can they be empty if all values are zero? Even the rx_angles could potentially be empty in the case of a single beam sonar :)

@CaptKrasno
Copy link
Collaborator Author

@lindzey I have added the number of beams field to the ping info message. I will do some documentation soon.

@CaptKrasno
Copy link
Collaborator Author

@lauralindzey @rolker

I've sent you an email discussing some issues with coordinate convention and catching you up on the driver. Let me know your thoughts ASAP and we can get this resolved.

@lauralindzey
Copy link
Collaborator

Merging!

We know of some documentation improvements that we should make, but I'm going to go ahead and merge this so we can start using it. The documentation can come in a separate PR.

@lauralindzey lauralindzey merged commit b2ef4a4 into apl-ocean-engineering:main May 5, 2022
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

4 participants