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

Updated comms model #579

Merged
merged 7 commits into from
Sep 23, 2020
Merged

Updated comms model #579

merged 7 commits into from
Sep 23, 2020

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Aug 26, 2020

A pass of improvements over the communication model when breadcrumbs are deployed.

The main change is a modification on how distance is computed in the model. Before this change, the Euclidean distance between source and destination was used even in the presence of breadcrumbs. This created a radial effect in the probability of message success as shown in the picture. This is inconsistent with the fact that breadcrumbs should extend the communication range. In the following example the source robot is in the staging area.

comms_v1

The new approach still takes into account distance but in a different way. Instead, we compute the greatest single hop of the route from source to destination. A hop is the distance from source to the first breadcrumb, between a pair of breadcrumbs, or between the last breadcrumb and the destination. In the following example with the updated model the source robot is in the staging area.

comms_v2

The updated model also adds a small penalty (in distance) every time that a breadcrumb is added to the route.

This should close issue #488 .

Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero caguero requested review from nkoenig and acschang August 26, 2020 14:26
@acschang
Copy link
Contributor

acschang commented Sep 1, 2020

For tracking sake:
Example breadcrumb placement in PW01 is as follows:
pw01_v2_test1

There are three issues from this breadcrumb placement with this PR:
pw01_option_c_issues

  1. Comms dead zone in green area
  2. Comms dead zone between drop-off in the Y tile and the adjacent tile
  3. Low comms escalating to high comms in further adjacent tile

These issues are not present with the current comms model (A) or visualization-only comms model (B):
pw01_legacy_bc_exp

Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
auto firstBreadcrumb = _relaySequence.at(0);
auto secondBreadcrumb = _relaySequence.at(1);

// We only consider the first breadcrumb stored in the tile.
Copy link
Contributor

@acschang acschang Sep 17, 2020

Choose a reason for hiding this comment

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

Is it accurate that additional breadcrumbs in a tile have no bearing on a max distance single hop from that tile?

e.g.
Placing a breadcrumb at the beginning and end of a long tile would have an adverse effect on communication beyond the max hop distance would be unable to factor in the breadcrumb at the end of the long tile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it accurate that additional breadcrumbs in a tile have no bearing on a max distance single hop from that tile?

e.g.
Placing a breadcrumb at the beginning and end of a long tile would have an adverse effect on communication beyond the max hop distance would be unable to factor in the breadcrumb at the end of the long tile.

That's correct, we're not taking into account additional breadcrumbs within the same tile.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be related to #587

Copy link
Contributor

@acschang acschang left a comment

Choose a reason for hiding this comment

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

Results look good on test cases with the resolution of all but the first issue (dead zone) which is not related to a source modification. If you can clarify the comment I left on VisibilityTable.cc then I think we can be good to go on a merge here.

@angelacmaio
Copy link
Contributor

Please update the documentation with more detail on how the comms model works once this is merged.

Copy link
Contributor

@acschang acschang left a comment

Choose a reason for hiding this comment

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

This is ready as issues 2 and 3 have been addressed. A subsequent PR to address overlapping tiles blocking communication can be used to resolve issue 1 as that issue is not related to these changes and is present in the current communication model and the visibility-only communication model.

caguero and others added 4 commits September 23, 2020 00:36
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
* Visibility table enhancements

* Use threads to accelerate computation (distributing the space equally
across the threads)
* Split visiblity functionality into library to deduplicate compilation
* New validate_visibility_table cc file for main function
* Use FCL's distance function to disambiguate overlapped tile segments

Signed-off-by: Michael Carroll <michael@openrobotics.org>

* Cleanups

Signed-off-by: Michael Carroll <michael@openrobotics.org>

* Adjust to Ubuntu libfcl (0.5.0)

Signed-off-by: Michael Carroll <michael@openrobotics.org>

* Address reviewer feedback

Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero caguero merged commit 6fb6235 into master Sep 23, 2020
@nkoenig nkoenig deleted the comms_v2 branch December 10, 2020 22:30
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