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

Add comms infrastructure #1416

Merged
merged 44 commits into from
Apr 13, 2022
Merged

Add comms infrastructure #1416

merged 44 commits into from
Apr 13, 2022

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Mar 28, 2022

🎉 New feature

Requires

Summary

This PR adds some infrastructure for being able to create comms systems. A comms system lets you send and receive data under the constrains of a given comms model. The comms model specifies when and how messages should be delivered to its destinations.

Subscribers create a regular callback via Ignition Transport and send a request to a centralized broker to bind an address, the robot attached to the address and the topic name used as a callback. Publishers fill a Dataframe message specifying the addresses of the sender and destination and set the payload. Then, publishers need to send via Ignition Transport the message to the centralized broker.

Besides the general infrastructure, this PR contains two systems: PerfectComms and CommsEndpoint.

PerfectComms is an example of a comms system. As required, it implements the ICommsModel interface and it always delivers the messages to their destinations.

CommsEndpoint is a helper system that can be optionally attached to a model (see examples/worlds/comms.sdf) and lets you assign an address to a robot and the subscription topic. The system automatically connects with the broker and bind this address/robot for you. You could do this process manually if needed but I think it's convenient for most of the users.

Test it

  • Run the tests.
  • Run the example:
  1. Launch Gazebo:
ign gazebo -v 4 <your_ws>/src/ign-gazebo/examples/worlds/perfect_comms.sdf
  1. Launch a subscriber:
ign topic -e -t addr2/rx
  1. Launch a publisher:
cd <your_ws>/src/ign-gazebo/examples/standalone/comms
mkdir build && cd build
cmake ..
make
./publisher addr2

You should observe messages on the subscriber terminal.

CC @arjo129

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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.

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

caguero and others added 13 commits March 2, 2022 20:21
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero caguero requested a review from iche033 March 28, 2022 16:57
@caguero caguero requested a review from chapulina as a code owner March 28, 2022 16:57
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Mar 28, 2022
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Mar 28, 2022
@caguero caguero added the mbzirc Sponsored by MBZIRC: https://github.com/osrf/mbzirc/ label Mar 28, 2022
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

I've done a cursory review of this. Overall its looking pretty good. I spotted some places where we used std::map instead of std::unordered_map. Since this is targeting fortress and I believe we are using C++17 we should prefer unordered_map unless we need the data sorted. One issue I have is with the Registry type. Currently its an alias of an std::map however as I've listed this leads to two problems (1) fragile API/ABI since this is something end users implementing comms plugins will use making it difficult for us to change underlying implementation and (2) the user has to access and internal data type to figure out how to add a message to the outbound. I can make a PR targeting this PR with the relevant changes.

I think as we discussed before the comms model for acoustics will need the ability to be stepped at a different pace. That can be addressed in a future PR.

include/ignition/gazebo/comms/MsgManager.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/comms/MsgManager.hh Outdated Show resolved Hide resolved
src/systems/perfect_comms/PerfectComms.cc Outdated Show resolved Hide resolved
include/ignition/gazebo/comms/MsgManager.hh Show resolved Hide resolved
include/ignition/gazebo/comms/MsgManager.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/comms/Broker.hh Show resolved Hide resolved
test/integration/comms.cc Outdated Show resolved Hide resolved
@caguero
Copy link
Contributor Author

caguero commented Mar 29, 2022

I've done a cursory review of this. Overall its looking pretty good. I spotted some places where we used std::map instead of std::unordered_map. Since this is targeting fortress and I believe we are using C++17 we should prefer unordered_map unless we need the data sorted. One issue I have is with the Registry type. Currently its an alias of an std::map however as I've listed this leads to two problems (1) fragile API/ABI since this is something end users implementing comms plugins will use making it difficult for us to change underlying implementation and (2) the user has to access and internal data type to figure out how to add a message to the outbound. I can make a PR targeting this PR with the relevant changes.

I think as we discussed before the comms model for acoustics will need the ability to be stepped at a different pace. That can be addressed in a future PR.

Thanks for the suggestions and the offer to help! They all sound pretty good. If you're doing the Registry transformation from an alias to its own class I can work in parallel in the changes to step at different frequency than physics.

@mjcarroll
Copy link
Contributor

One note would be that it appears that we could move the stuff that is in include (part of the public API) to the src directory and continue to iterate before we commit to putting it in the public API. Is that a correct assumption?

caguero and others added 3 commits April 12, 2022 20:21
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero
Copy link
Contributor Author

caguero commented Apr 12, 2022

One note would be that it appears that we could move the stuff that is in include (part of the public API) to the src directory and continue to iterate before we commit to putting it in the public API. Is that a correct assumption?

We talked about this offline. The derived plugins will need to include the ICommsModel.hh, so I don't think the headers can be installed in src.

caguero and others added 7 commits April 13, 2022 09:10
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero
Copy link
Contributor Author

caguero commented Apr 13, 2022

All right, I think Windows CI is happy, which it was the last outstanding bit as far as I know.

https://build.osrfoundation.org/job/ign_gazebo-pr-win/4164/

*
*/

// Example: ./publisher addr1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be addr2?

ign topic -e -t addr1/rx

Try launching a comms publisher:
./publisher addr1
Copy link
Contributor

Choose a reason for hiding this comment

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

addr1 -> addr2

@iche033
Copy link
Contributor

iche033 commented Apr 13, 2022

just have very minor comments. I think you can probably just do that in #1428 so we don't trigger another round of CI

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me

@iche033 iche033 dismissed stale reviews from ahcorde and arjo129 April 13, 2022 19:56

comments addressed

@iche033 iche033 merged commit 7c88a8e into ign-gazebo6 Apr 13, 2022
@iche033 iche033 deleted the comms branch April 13, 2022 19:59
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Apr 13, 2022
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-releases-2022-04-27-fortress-citadel/1389/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress mbzirc Sponsored by MBZIRC: https://github.com/osrf/mbzirc/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants