Skip to content

Custom IP/Port fixes for xsbug on Windows #1376

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

Open
wants to merge 8 commits into
base: public
Choose a base branch
from

Conversation

cmidgley
Copy link
Contributor

Fixes the following issues to support custom IP/ports with xsbug on Windows:

  1. Adds XSBUG_PORT and XSBUG_HOST environment variable support for Windows make (nmake.win.mk)
  2. Restarts xsbug in nmake.esp32.mk during pre-build so xsbug can adopt the then-current host/port

    This may be an issue on other platforms as well. It's only a problem when changing the environment variables during a session, such as with my automation test framework.

  3. Add support for XSBUG_PORT environment variable in xsbug-log

@phoddie
Copy link
Collaborator

phoddie commented Jul 22, 2024

I don't think this change is correct (though it probably does exactly what you want). We have to be careful because the Node-RED MCU editor plug-in really depends on this working in a specific way (and I broke that once!).

XSBUG_PORT and XSBUG_HOST are used by simulator hosts and serial2xsbug to connect to xsbug. They are not used by xsbug to set its listening port. The port and host can be set on the command line to mcconfig using the -x option (docs note: "overrides the default host and port (localhost:5002) debug builds use to connect to xsbug.")

xsbug has a preference in the UI to set the port that it listens on. xsbug-log (currently) does not have a way to set the listening port. If we define a way to set the listening port, we can use that in both xsbug-log and xsbug. But, we will need to be explicit about what happens in xsbug if there is both a user preference set and the environment variable to avoid inconsistent behaviors.

FWIW –  It may be useful to understand the way Node-RED MCU editor works. It sets the XSBUG_PORT to something other than 5002. It listens on that port with an xsbug proxy server. It monitors for certain messages and injects others. If xsbug is running, it is on 5002 and the proxy connects to that, relaying all traffic.

cc: @ralphwetzel

@cmidgley
Copy link
Contributor Author

Thanks for the background. Since we can't set the command line options for xsbug-log when launched by mcconfig, how about a different environment variable, like XSBUG_LOG_LISTEN_PORT? I guess we could add some preferences file that it loads, or even have it examine the manifest for settings, but the environment variable seems easiest. Of course it would need to be set outside of running mcconfig but that's ok.

I'm pretty sure the Windows build issue still needs to be addressed. The XSBUG_HOST and XSBUG_PORT environment variables are never set currently, which means that serial2xsbug isn't using it (independent of any xsbug-log changes).

So as a proposal:

  1. Change xsbug-log to use XSBUG_LOG_LISTEN_PORT as the environment variable
  2. Back out the xsbug kill/restart
  3. Leave the Windows build change in place as is

Thoughts?

@phoddie
Copy link
Collaborator

phoddie commented Jul 22, 2024

how about a different environment variable, like XSBUG_LOG_LISTEN_PORT?

That seems like a good first step. And it could be all that is needed.

I'm pretty sure the Windows build issue still needs to be addressed. The XSBUG_HOST and XSBUG_PORT environment variables are never set currently, which means that serial2xsbug isn't using it (independent of any xsbug-log changes).

It looks like the embedded nmake files do set XSBUG_HOST and XSBUG_PORT. It is just the Windows simulator build nmake files that doesn't (that's the one your PR changes). If we are going to fix that, we should do it for both the xsbug-log case (your changes does this) and the normal xsbug bug case (your change doesn't do that) to be consistent with Mac and Linux.

@cmidgley
Copy link
Contributor Author

I'm back to looking at this, and I'm finding the current approach a little odd. Currently, if you set XSBUG_HOST it still launches xsbug (or xsbug-log) even though it's intent is to connect to an xsbug running on a different host. It makes for a confusing experience to launch the debugger locally when the intent was to allow connecting a remote one. Additionally, it doesn't really make sense to locally launch with XSBUG_PORT changed and not change xsbug to listen on that port, as it won't connect if you don't manage that carefully.

My thinking is that the makefiles should check if "$(XSBUG_HOST)" == "localhost" (which is defaulted this way in mcmanifest.js), and use XSBUG_PORT consistently for setting the port for locally launched instances (xsbug and xsbug-log). If you launch with XSBUG_HOST set to anything else (-x myhost:1234), it would not launch any xsbug and connect to the specified host/port. On your other host (or different shell on same host) you can use XSBUG_PORT for xsbug-log or preferences settings for xsbug.

For xsbug to know which port to use, it can just use XSBUG_PORT to override the preferences port, since it would only be set when launched via mcconfig. The UI could show in preferences a help message that it was launched by mcconfig and the port is temporarily overridden. This all has the benefit that when you hand-launch xsbug it comes up in "remote" mode, listening on your preferences port., but when mcconfig launches it locally, it "just works" with the override port.

I took a stab at this with the Windows simulator makefile (gist) and it's a really clean experience. By default it "just works" (except for port and xsbug due to properties setting), but if I change the host, it easily connects to another host. The "gotcha" here is that if you want xsbug running on the same host, but in a different shell - a hacky solution is to use -x 127.0.0.1:5002 and since it doesn't match localhost it connects remotely. This is a change that would need to be made to all the makefiles, plus a bit of work on xsbug itself.

I think this makes it much cleaner for remote, since you don't get an extra instance of xsbug launched, and unifies the environment variable use across xsbug and xsbug-log. Thoughts?

@cmidgley
Copy link
Contributor Author

cmidgley commented May 3, 2025

Tested with latest Moddable and continues to work well for me. Any possibility to merge this?

@phoddie
Copy link
Collaborator

phoddie commented May 5, 2025

I understand you have a scenario you'd like to work and I support that, but it cannot be a breaking change,.

xsbug does not take the port to listen on from the environment. It defaults the listening port to 5002 and allows it to be overridden by preferences which are set in the user interface. xsbug-log always listens on port 5002, with no option to override. If I've overlooked something, please let me know.

The XSBUG_PORT environment variable tells hosts (mcsim and serial2xsbug, which is an xsbug proxy for a host) the port to connect to on the JavaScript debugger. Overloading it to also indicate the port for the debugger to listen on will be confusing and, importantly, will cause failures with the Node-RED MCU plug-in which runs a localhost xsbug proxy. It is also not parallel with XSBUG_HOST which cannot tell the debugger which host to listen on – it will always be localhost.
New environment variables are free, eliminate any ambiguity in this situation, and preserve the consistent use XSBUG_HOST and XSBUG_PORT together.

Providing a port from the environment to xsbug-log is straightforward. As you note, doing so for xsbug raises the question about whether how the environment variable interacts with the stored preference including how it appears in the user interface.

Currently, if you set XSBUG_HOST it still launches xsbug (or xsbug-log) even though it's intent is to connect to an xsbug running on a different host.

Agreed – there's no need to launch the debugger locally if it won't be used.


To understand the concern with the Node-RED MCU Edition, perhaps some details will help. There are three pieces of software all running on the same computer:

  1. Host (either mcsim or serial2xsbug) – this connects to the debugger at XS_HOST and XS_PORT (or localhost:5002 if not specified). The Node-RED MCU Plugin sets XS_PORT to 5004
  2. Node-RED MCU Plugin proxy. This listens on port 5004 and connects to xsbug on port 5002
  3. xsbug – This listens on port 5002. (We don't want to change this port from the default because it can only lead to confusion with devs also using xsbug outside the Node-RED MCU plugin)

In this scenario, xsbug listens on a different port than the one the host uses to connect to it. That works, thanks to the proxy, but it means that a single environment variable cannot be used to specify the port.

@cmidgley
Copy link
Contributor Author

cmidgley commented May 6, 2025

Thank you for the detailed explanation of Node-RED and how it acts as a proxy. It was very helpful in understanding the port usage strategy.

Given this, I now have XSBUG_LOG_PORT used only for the listening port of xsbug-log. XSBUG_PORT and XSBUG_HOST remain unchanged with their original use. For this to work it required changes to many makefiles, but all generally doing the same thing:

  1. Default XSBUG_LOG_PORT to 5002 (when XSBUG_PORT is defaulted in the makefile)
  2. Expand all executions of node xsbug-log to include XSBUG_LOG_PORT alongside the existing XSBUG_PORT and XSBUG_HOST.

How is this attempt looking? Thanks!

@phoddie
Copy link
Collaborator

phoddie commented May 7, 2025

I think this is reasonable. It is focused, so there won't be surprise side effects.

It shouldn't be necessary to set XSBUG_LOG_PORT except when launching xsbug-log, so the changes are a little over complete. But, there's need to unwind that.

Thanks for sticking with this. The support for Node-RED MCU Edition is subtle, but it enables really amazing workflows in that environment (bi-directional communication between the Nodes on the device and Node-RED running in the browser) so I don't want to repress on that hard-won capability.

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.

2 participants