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

Adding server_uri for dispatcher node #230

Merged
merged 2 commits into from
May 27, 2024
Merged

Adding server_uri for dispatcher node #230

merged 2 commits into from
May 27, 2024

Conversation

aaronchongth
Copy link
Member

New feature implementation

Implemented feature

Related to open-rmf/rmf_ros2#355.
Gives the dispatcher node websocket access to servers

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth requested a review from arjo129 May 17, 2024 10:05
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Copy link
Member

@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.

LGTM

@arjo129 arjo129 merged commit 05504d9 into main May 27, 2024
2 of 3 checks passed
@arjo129 arjo129 deleted the dispatcher-ws branch May 27, 2024 09:26
@mxgrey
Copy link
Collaborator

mxgrey commented Jun 5, 2024

By changing the default value for server_uri, this PR has had the unpleasant side effect of causing these error messages to be blasted out once per second if a user is running a simulation without using the api-server:

[fleet_adapter-16] [ERROR] [1717560956.734331412] [tinyRobot_fleet_adapter]: Connection not yet established
[fleet_adapter-16] [INFO] [1717560956.757289553] [tinyRobot_fleet_adapter]: Attempting to connect to ws://localhost:8000/_internal
[fleet_adapter-16] [ERROR] [1717560956.758046354] [tinyRobot_fleet_adapter]: Connection to ws://localhost:8000/_internal failed. Reason:
[fleet_adapter-16]  > URI: ws://localhost:8000/_internal
[fleet_adapter-16] > Status: Closed
[fleet_adapter-16] > Remote Server: None Specified
[fleet_adapter-16] > Error/close reason: Underlying Transport Error
[fleet_adapter-16] 
[fleet_adapter-16] [ERROR] [1717560956.758128734] [tinyRobot_fleet_adapter]: Connection lost
[fleet_adapter-16] > Reconnecting in 1s
[fleet_adapter-16] > Host: ws://localhost:8000/_internal
[rmf_task_dispatcher-13] [INFO] [1717560956.953390910] [rmf_dispatcher_node]: Attempting to connect to ws://localhost:8000/_internal
[rmf_task_dispatcher-13] [ERROR] [1717560956.954344271] [rmf_dispatcher_node]: Connection to ws://localhost:8000/_internal failed. Reason:
[rmf_task_dispatcher-13]  > URI: ws://localhost:8000/_internal
[rmf_task_dispatcher-13] > Status: Closed
[rmf_task_dispatcher-13] > Remote Server: None Specified
[rmf_task_dispatcher-13] > Error/close reason: Underlying Transport Error
[rmf_task_dispatcher-13] 
[rmf_task_dispatcher-13] [ERROR] [1717560956.954460261] [rmf_dispatcher_node]: Connection lost
[rmf_task_dispatcher-13] > Reconnecting in 1s
[rmf_task_dispatcher-13] > Host: ws://localhost:8000/_internal

I think we should be consistent and keep the default parameter value for server_uri as an empty string. Since this PR was merged, the only way for a user to indicate that they don't need a web connection is to launch like this:

$ ros2 launch rmf_demos_gz office.launch.xml server_uri:=\"\"

The user needs to figure out that

  1. server_uri must be explicitly set to blank to stop the error messages
  2. Escaped-quotes need to be used in order for the terminal command to express an empty string

I expect this to cause significant confusion for users.

aaronchongth added a commit that referenced this pull request Jun 19, 2024
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
(cherry picked from commit 05504d9)
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
aaronchongth added a commit that referenced this pull request Jun 19, 2024
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
(cherry picked from commit 05504d9)
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
xiyuoh pushed a commit that referenced this pull request Jun 25, 2024
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
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.

3 participants