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 Dataframe message #238

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Add Dataframe message #238

merged 3 commits into from
Apr 5, 2022

Conversation

caguero
Copy link
Collaborator

@caguero caguero commented Mar 28, 2022

🎉 New feature

Required by

Summary

This is a message to mimic a data frame used to send and receive data over the network.

Test it

You can test it via gazebosim/gz-sim#1416

Note: This is ready to review but do not merge it yet please. I'm working on a Gazebo system that will use this message and want to confirm that no extra fields are needed.

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 caguero requested a review from iche033 March 28, 2022 16:57
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Mar 28, 2022
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #238 (a2119ff) into ign-msgs8 (962ae76) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           ign-msgs8     #238   +/-   ##
==========================================
  Coverage      85.38%   85.38%           
==========================================
  Files              9        9           
  Lines            951      951           
==========================================
  Hits             812      812           
  Misses           139      139           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 962ae76...a2119ff. Read the comment docs.

@caguero caguero added the mbzirc label Mar 28, 2022
{
/// \brief Header data.
Header header = 1;

Copy link
Contributor

Choose a reason for hiding this comment

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

While a header with the correct time stamp and the source and destination addresses are sufficient for the MBZIRC use case, we should also log the position of where the message is starting from this is useful for the MBARI use case. The reason is that the message take in the order of a few seconds to propagate hence it would help if the original position were logged as the vehicle may have moved in this time frame.

Copy link
Collaborator Author

@caguero caguero Apr 5, 2022

Choose a reason for hiding this comment

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

Does it make sense to store that information in the specific comms model during the step that's supposed to process this message for the first time? In addition, the client might not know its own location.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's one possibility, but it would lead to very complicated code. We probably can revisit this afterwards.

@caguero caguero merged commit 0f07dc7 into ign-msgs8 Apr 5, 2022
@caguero caguero deleted the caguero/comms branch April 5, 2022 15:33
@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-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress mbzirc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants