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

Update connectivity.md #1870

Merged
merged 5 commits into from
Sep 25, 2023
Merged

Update connectivity.md #1870

merged 5 commits into from
Sep 25, 2023

Conversation

npentrel
Copy link
Collaborator

@npentrel npentrel commented Sep 20, 2023

No description provided.

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Sep 20, 2023
@viambot
Copy link
Member

viambot commented Sep 20, 2023

Overall readability score: 55.73 (🔴 -0.04)

File Readability
sessions.md 53.03 (🟢 +0.05)
connectivity.md 46.06 (🔴 -4.67)
run.md 42.25 (🔴 -7.05)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
sessions.md 53.03 42 11.99 13.7 11.89 7.27
  🟢 +0.05 🟢 +0.3 🟢 +0.01 🟢 +0.1 🔴 -0.05 🔴 -0.03
connectivity.md 46.06 35.41 13.98 15.5 11.26 7.5
  🔴 -4.67 🔴 -5.28 🔴 -1.32 🔴 -2.1 🟢 +0.23 🟢 +0.37
run.md 42.25 42.75 15.06 17.4 11.26 7.63
  🔴 -7.05 🟢 +1.56 🔴 -3.08 🔴 -1.8 🟢 +0.87 🔴 -0.2

Averages:

  Readability FRE GF ARI CLI DCRS
Average 55.73 46.84 10.59 13.1 11.85 7.72
  🔴 -0.04 🔴 -0.01 🔴 -0.01 🔴 -0.01 🟢 +0 🟢 +0
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

@npentrel
Copy link
Collaborator Author

@dannenberg - could you read through the changes to the connectivity page and ensure that that answers all remaining questions

and @cheukt - apologies - could you confirm this is all still correct?

@sguequierre - please read over it for correctness as well

Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

looks accurate

Copy link
Contributor

@dannenberg dannenberg left a comment

Choose a reason for hiding this comment

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

the only question i'm not seeing an answer to here is "can a client establish a connection to the viam-server over LAN without an internet connection?" and possibly implicit in that "if we were connected via a route using the internet, but that gets severed, will the client be able to reconnect over their shared LAN?" (could this happen. that is, would webrtc ever choose a route out of LAN when LAN is available?)

otherwise this is looking great. thank you for your efforts

docs/program/apis/sessions.md Outdated Show resolved Hide resolved
docs/program/connectivity.md Outdated Show resolved Hide resolved
docs/program/connectivity.md Outdated Show resolved Hide resolved

When a robot loses its connection over LAN or WAN, it can no longer communicate with clients through [the Viam app](https://app.viam.com).
When no heartbeat has been received over a timeout period, `viam-server` will end any current client [*sessions*](/program/apis/sessions/) on this robot.
- Client sessions connected through the internet will timeout and end.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(opt) For clarity I would suggest moving this down and specifying "different LAN or WAN" or "through the cloud" vs just "through the internet", but idk I think this is technically correct anyways?

@npentrel
Copy link
Collaborator Author

@cheukt please confirm the answers:

  1. "can a client establish a connection to the viam-server over LAN without an internet connection?"

I assume yes and you'd do that the same way as https://docs-test.viam.dev/d4b541ef8633aa931fd9723fc58b82f3d0fd8915/public/program/run/#run-code-on-robot ?

  1. "if we were connected via a route using the internet, but that gets severed, will the client be able to reconnect over their shared LAN?" (could this happen. that is, would webrtc ever choose a route out of LAN when LAN is available?)

I believe it would automatially figure out there's a better connection and use LAN, so no reconnection needed?

@cheukt
Copy link
Member

cheukt commented Sep 21, 2023

for 1, yes, and at this point you should be able to connect just using the code sample for all the supported SDKs
for 2, if the client was connected over internet, it will disconnect and reconnect again, but that should get handled automatically. it should favor a connection over LAN though, at which point connection will not get dropped

@dannenberg
Copy link
Contributor

for clarity, the steps naomi linked in #1 are no longer necessary. correct cheuk?

@cheukt
Copy link
Member

cheukt commented Sep 21, 2023

yes, it should definitely work with go and python sdk. possibly not for the typescript sdk

@npentrel npentrel force-pushed the npentrel-patch-1 branch 6 times, most recently from 59ac856 to 16c1723 Compare September 23, 2023 19:54
@npentrel
Copy link
Collaborator Author

@dannenberg and @cheukt could you please review the changes in the last commit ("Address feedback") to ensure the changes based on your comments are factually accurate and cover what we want to cover?

We will need to understand the Typescript limitations as well but we can do that separately.

@viambot
Copy link
Member

viambot commented Sep 25, 2023

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/39fc5ee56de4407039031aa93adcaa790735dd02/public

@npentrel npentrel merged commit 625e0b8 into main Sep 25, 2023
9 of 10 checks passed
@npentrel npentrel deleted the npentrel-patch-1 branch September 25, 2023 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants