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 comms plugin #1608

Merged
merged 19 commits into from
Aug 24, 2022
Merged

Acoustic comms plugin #1608

merged 19 commits into from
Aug 24, 2022

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Jul 22, 2022

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Summary

This plugin adds a simple acoustic comms plugin to gz-sim.

Test it

Added a small test case which contains 2 pairs of boxes which communicate with each other. One set in inside the max range of comms, and the other is outside it. The one outside the range should not receive any messages.

TODO

  • The current calculation for distanceToTransmitter considers the current distance between the 2 bodies, when actually it should be the distance between current receiver position and position of the sender at the time at which the packet was sent, as the sender might be moving. This is not a big problem if the 2 bodies are at rest or moving slowly compared to speed of sound.

Since the packet does not contain a field related to position of sender at the time the packet was sent, I;m not sure how to implement this.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Aditya <[email protected]>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review July 22, 2022 21:08
src/systems/acoustic_comms/AcousticComms.cc Outdated Show resolved Hide resolved
src/systems/acoustic_comms/AcousticComms.cc Outdated Show resolved Hide resolved
test/integration/acoustic_comms.cc Show resolved Hide resolved
src/systems/acoustic_comms/AcousticComms.cc Outdated Show resolved Hide resolved
@chapulina chapulina added enhancement New feature or request 🌱 garden Ignition Garden MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv labels Jul 23, 2022
Signed-off-by: Aditya <[email protected]>
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #1608 (96e9bea) into main (9d24893) will increase coverage by 0.06%.
The diff coverage is 94.64%.

❗ Current head 96e9bea differs from pull request most recent head 6aceed4. Consider uploading reports for the commit 6aceed4 to get more accurate results

@@            Coverage Diff             @@
##             main    #1608      +/-   ##
==========================================
+ Coverage   63.57%   63.63%   +0.06%     
==========================================
  Files         330      331       +1     
  Lines       25926    25981      +55     
==========================================
+ Hits        16482    16534      +52     
- Misses       9444     9447       +3     
Impacted Files Coverage Δ
src/systems/acoustic_comms/AcousticComms.cc 94.64% <94.64%> (ø)
src/EntityComponentManager.cc 90.86% <0.00%> (-0.02%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

At a high level, the code looks good to me and I like the strategy for deciding when to keep the message, drop it or deliver it.

My only suggestion is to try to remove the need for the transmitter to populate the message with its own pose. I think it would be way more elegant if that's something that the plugin can do automatically for us. We have access to the ECM and the entity ID of the sender, so it should be possible.

@adityapande-1995
Copy link
Contributor Author

As per offline discussion with @caguero , we decided that making the users specify the position of the sender in the header file is cumbersome. Instead, what I have now is : The plugin will keep track the position of sender when a msg is first processed, and use that for distance calculations throughout the life of the message.

This means the difference between actual position of the sender (when the msg was sent) and the one we use will only differ by one spin cycle of the plugin, which is still good.

src/systems/acoustic_comms/AcousticComms.cc Show resolved Hide resolved
}
}
else
{
Copy link
Contributor

@caguero caguero Jul 29, 2022

Choose a reason for hiding this comment

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

I think there's a situation that isn't covered here: If source and destination are initially at a distance > maxRange but the destination is moving closer to the sender (in the direction of the message). If I'm not wrong, you'll discard the message instantly but you're not accounting that potentially the destination might be within range eventually while the message is traveling.

Copy link
Contributor Author

@adityapande-1995 adityapande-1995 Jul 29, 2022

Choose a reason for hiding this comment

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

I am actually, distanceToTransmitter is the distance between :

  • Position of sender when the message was sent, (this is constant throughout the lifetime of the message)
  • Current position of the destination. This is when the plugin is spinning, so always contains the current position of destination. So even if the destination has just moved towards the source, that distance should be reflected accurately.

I could add a test for this situation if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, the key is to use distanceCoveredByMessage to decide when to drop the message and not distanceToTransmitter as I originally thought you were using.

@chapulina
Copy link
Contributor

Removed the Garden label because we're past feature freeze ❄️ .

This PR can either be merged into main and backported to gz-sim7 after the stable release, or retargeted to gz-sim7 and merged after the stable release (i.e. October).

@adityapande-1995
Copy link
Contributor Author

@osrf-jenkins retest this please

@adityapande-1995
Copy link
Contributor Author

I don't think the failures in CI are related to the changes made here.

src/systems/acoustic_comms/AcousticComms.cc Show resolved Hide resolved
src/systems/acoustic_comms/AcousticComms.cc Outdated Show resolved Hide resolved
src/systems/acoustic_comms/AcousticComms.cc Outdated Show resolved Hide resolved
src/systems/acoustic_comms/AcousticComms.cc Outdated Show resolved Hide resolved
src/systems/acoustic_comms/AcousticComms.cc Outdated Show resolved Hide resolved
test/integration/acoustic_comms.cc Outdated Show resolved Hide resolved
test/integration/acoustic_comms.cc Outdated Show resolved Hide resolved
test/integration/acoustic_comms.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM barring minor comments

test/integration/acoustic_comms.cc Outdated Show resolved Hide resolved
test/integration/acoustic_comms.cc Outdated Show resolved Hide resolved
@adityapande-1995 adityapande-1995 merged commit a7071ec into main Aug 24, 2022
@adityapande-1995 adityapande-1995 deleted the aditya/acoustic_comms branch August 24, 2022 21:21
adityapande-1995 added a commit that referenced this pull request Aug 25, 2022
* Added acoustic comms plugin

Signed-off-by: Aditya <[email protected]>
hidmic pushed a commit that referenced this pull request Aug 31, 2022
* Added acoustic comms plugin

Signed-off-by: Aditya <[email protected]>
adityapande-1995 added a commit that referenced this pull request Sep 14, 2022
* Added acoustic comms plugin

Signed-off-by: Aditya <[email protected]>
azeey pushed a commit to azeey/gz-sim that referenced this pull request Sep 22, 2022
* Added acoustic comms plugin

Signed-off-by: Aditya <[email protected]>
adityapande-1995 added a commit that referenced this pull request Oct 7, 2022
* Acoustic comms plugin (#1608)
* Acoustic comms example world (#1693)

Signed-off-by: Aditya <[email protected]>
arjo129 pushed a commit that referenced this pull request Nov 1, 2022
* Added acoustic comms plugin

Signed-off-by: Aditya <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants