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

Comms bridge #756

Merged
merged 35 commits into from
Jan 19, 2024
Merged

Comms bridge #756

merged 35 commits into from
Jan 19, 2024

Conversation

kbrowne15
Copy link
Contributor

This is the comms bridge that allows any ros message to be sent between robots without having to specifically add ros to dds and vice versa code.

@kbrowne15 kbrowne15 changed the base branch from master to develop November 20, 2023 04:36
Copy link
Contributor

@trey0 trey0 left a comment

Choose a reason for hiding this comment

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

Thanks for sharing this! Great to see how far it's coming along.

Comments mostly trivial.

I had a hard time understanding how to estimate the memory usage. The 128K per content message buffer allocation could potentially get bad quickly if we aren't careful. But maybe it doesn't matter for ISAAC in the near term if we don't subscribe to high-rate messages.

astrobee/config/communications/comms_bridge.config Outdated Show resolved Hide resolved
astrobee/config/communications/comms_bridge.config Outdated Show resolved Hide resolved
astrobee/config/communications/comms_bridge.config Outdated Show resolved Hide resolved
astrobee/config/communications/comms_bridge.config Outdated Show resolved Hide resolved
communications/comms_bridge/README.md Outdated Show resolved Hide resolved
Added an out topic to link entries so one can specify an out topic name if
the8y want. Also added code so that all topics don't go to all robots.
The comms bridge was changed to start the dds subscribers and
publishers either on start up if specified in the comms bridge
configuration file or when the executive calls the enable comms
service. This matches how the current astrobee to astrobee bridge
works.
@kbrowne15 kbrowne15 force-pushed the comms_bridge branch 3 times, most recently from aa51b37 to 0d1c82b Compare January 11, 2024 21:23
@kbrowne15 kbrowne15 marked this pull request as ready for review January 13, 2024 01:11
Copy link
Member

@marinagmoreira marinagmoreira left a comment

Choose a reason for hiding this comment

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

Tested latest version in the lab

Copy link
Contributor

@rgarciaruiz rgarciaruiz left a comment

Choose a reason for hiding this comment

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

The current configuration for the bridge is not exactly what other users would need. Usually, they need /gnc/ekf both ways. This is not a big deal though since it can be configured.

@kbrowne15 kbrowne15 requested a review from bcoltin January 17, 2024 23:55
Copy link
Contributor

@trey0 trey0 left a comment

Choose a reason for hiding this comment

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

Thanks! I'm really excited about this coming together and hope other Astrobee users will get some benefit. My comments are minor and mostly about docs.

astrobee/config/communications/comms_bridge.config Outdated Show resolved Hide resolved
communications/comms_bridge/readme.md Outdated Show resolved Hide resolved
communications/comms_bridge/readme.md Outdated Show resolved Hide resolved
@@ -71,11 +71,11 @@ echo " Analysing python code style with 'isort'."
# could happen for example if the .isort.cfg src_paths list gets out
# of date.

if $(isort . --extend-skip cmake --profile black --diff --check-only --quiet >/dev/null); then
if $(isort . --extend-skip cmake,submodules --profile black --diff --check-only --quiet >/dev/null); then
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Should we propagate this change to the isaac repo? Not sure why the relevant commands are different.

https://github.com/nasa/isaac/blob/develop/scripts/git/pre-commit.linter_python#L75-L79

Copy link
Contributor

Choose a reason for hiding this comment

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

(To answer my own question: "Yes." nasa/isaac#123 )

@kbrowne15 kbrowne15 merged commit 2862998 into nasa:develop Jan 19, 2024
5 checks passed
@kbrowne15 kbrowne15 deleted the comms_bridge branch January 19, 2024 21:45
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.

5 participants