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

SNIClient: fix /snes command if tree #791

Merged
merged 4 commits into from
Sep 10, 2023
Merged

SNIClient: fix /snes command if tree #791

merged 4 commits into from
Sep 10, 2023

Conversation

Berserker66
Copy link
Member

No description provided.

@black-sliver
Copy link
Member

Looks like when using /snes addr:port 1 now, it will still try to connect to the default address:port (in addition to the new one), because ctx.snes_reconnect_address is set to the old one from the initial connect attempt

@black-sliver black-sliver added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. meta: help wanted Additional review/assistance is requested for these issues or pull requests. labels Jul 28, 2022
Copy link
Contributor

@Ijwu Ijwu left a comment

Choose a reason for hiding this comment

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

Whoops. See @black-sliver's comment.

@black-sliver
Copy link
Member

maybe to show what i mean:

/snes localhost:8080 1
Connecting to SNI at ws://localhost:8080 ...
Connecting to SNI at ws://localhost:23074 ...
Attaching to fxpakpro://./dev/ttyACM1
Attached to fxpakpro://./dev/ttyACM1

I believe the old "reconnect loop" remembers what it initially used and does not get cancelled.

@Berserker66
Copy link
Member Author

is this still the case after the /main merge?

@black-sliver
Copy link
Member

yes, unless i screwed up the merging, that's what i did for the output above

@black-sliver
Copy link
Member

/snes localhost:8080 1
Already connected to SNES. Disconnecting first.
Connecting to SNI at ws://localhost:8080 ...
Error connecting to SNI (Multiple exceptions: [Errno 111] Connect call failed ('::1', 8080, 0, 0), [Errno 111] Connect call failed ('127.0.0.1', 8080))
Attempt to start SNI was aborted as path SNI was not found, please start it yourself if it is not running
[starting SNI here manually]
Connecting to SNI at ws://localhost:23074 ...

ant then another

/snes localhost:8080 1
Already connected to SNES. Disconnecting first.
Already connected to SNI, likely awaiting a device.

But it never connects to 8080 afaict, and is effectively stuck at the "wrong" SNI

@Berserker66
Copy link
Member Author

/snes localhost:8080 1
Already connected to SNES. Disconnecting first.
Connecting to SNI at ws://localhost:8080 ...
Error connecting to SNI (Multiple exceptions: [Errno 111] Connect call failed ('::1', 8080, 0, 0), [Errno 111] Connect call failed ('127.0.0.1', 8080))
Attempt to start SNI was aborted as path SNI was not found, please start it yourself if it is not running
[starting SNI here manually]
Connecting to SNI at ws://localhost:23074 ...

ant then another

/snes localhost:8080 1
Already connected to SNES. Disconnecting first.
Already connected to SNI, likely awaiting a device.

But it never connects to 8080 afaict, and is effectively stuck at the "wrong" SNI

Assuming this is still an issue, it might be time to move this to an Issue, as it doesn't seem related to the PRs change

@@ -68,12 +68,11 @@ def connect_to_snes(self, snes_options: str = "") -> bool:
options = snes_options.split()
num_options = len(options)

if num_options > 0:
snes_device_number = int(options[0])

if num_options > 1:
snes_address = options[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trying to fix the problem reported by black-sliver

Suggested change
snes_address = options[0]
snes_address = self.ctx.snes_address = options[0]

Copy link
Collaborator

@el-u el-u left a comment

Choose a reason for hiding this comment

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

Independently of the additional fix suggestion above, I can confirm that with this PR the two-argument version of /snes no longer causes an exception due to trying to parse an URI as a number.

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

I wasted another hour on the connect code. I think I will write it here and stop caring now.

Basically,

  1. snes_device_number gets thrown away for reconnect (i.e. user has to start SNI first)
  2. connect address gets thrown away for reconnect (fixed by el-u's change)
  3. i believe the use of snes_reconnect_address in combination with /snes is unexpected.
  4. connect will get stuck if there are no snes devices on the connected SNI - so /snes to switch hosts requires the user to exit the SNI that SNIClient is already connected to
  5. duplicate logger line (because we have two connections to SNI)

Here is the text on Discord for reference:
https://discord.com/channels/731205301247803413/731214280439103580/1131490109703852062

--

(I don't think there is value in enabling this broken command by fixing the arg parsing, but since el-u and berserker think differently, I'll approve in the sense that this PR fixes the arg parsing and nothing else.)

@Berserker66 Berserker66 merged commit 72b44be into main Sep 10, 2023
23 checks passed
@Berserker66 Berserker66 deleted the sniclient_snes_fix branch September 10, 2023 05:19
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. meta: help wanted Additional review/assistance is requested for these issues or pull requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants