8000 RFC: Webrepl feature parity with mpremote · Issue #13540 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

RFC: Webrepl feature parity with mpremote #13540

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.

Sign up for GitHub

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

Jump to bottom
Open
felixdoerre opened this issue Jan 28, 2024 · 17 comments
Open

RFC: Webrepl feature parity with mpremote #13540

felixdoerre opened this issue Jan 28, 2024 · 17 comments
Labels
enhancement Feature requests, new feature implementations

Comments

@felixdoerre
Copy link
Contributor

The current webrepl-code seems to be a bit outdated lack behind the feature progress of mpremote. Additionally, the current file-transfer logic that aims to work while the REPL is blocked, however this can lead to filesystem corruption if the blocking code accesses the filesystem at the same time.

I propose the following:

  • move the password-checking logic from _webrepl to webrepl.py
  • remove the current file-transfer protocol, and with it the _webrepl module.
  • move the repl interaction from websocket "text" packages to "binary" packages, as the output from the repl is not required to be valid UTF-8, but just a byte sequence.
  • implement the same logic that mpremote uses for file transfers in the webrepl client.

Removing _webrepl and moving the password logic to webrepl.py even seems to make the firmware smaller:

   text    data     bss     dec     hex filename                                                                       
 606100       0  381308  987408   f1110 /home/jenkins/tmp/micropython/ports/rp2/build-RPI_PICO_W/firmware.elf
 605668       0  381292  986960   f0f50 /home/jenkins/tmp/micropython/ports/rp2/build-RPI_PICO_W/firmware.elf

I've implemented a proof-of-concept here: https://github.com/felixdoerre/webreplv2 together with a new webrepl client.

The benefits over the current webrepl are:

  • Allow binary data transfers over the websocket (this also helps other usages of the webrepl, like Thonny, or using the websocket as backend for mpremote instead of a serial device)
  • Implement a File-Browser in the webrepl-client, with file up- and download using the same primitives as mpremote
  • Implement mip to install packages directly from the webrepl-client
  • Implement the filesystem-mounting feature
  • Implement direct mounting of a local directory through the browser (without uploading). Due to webapi limitations this is only a readonly mount.
  • Allow browser-saving of the webrepl password
  • Auto-connect the webrepl, if the target (and password) is known.

Do you want to include this into micropython, replacing the current webrepl module?
@felixdoerre felixdoerre added the enhancement Feature requests, new feature implementations label Jan 28, 2024
@felixdoerre felixdoerre changed the title Webrepl feature parity with mpremote RFC: Webrepl feature parity with mpremote Feb 18, 2024
This was referenced Feb 29, 2024
Open
Open
@felixdoerre
Copy link
Contributor Author

Just to give an overview of the related PRs and their dependencies.

Regarding compatibility:

  • As implemented, the current webrepl-client is not compatible with webreplv2, as webreplv2 will send REPL output via websocket-binary packages, and the webrepl-client ignores them. This could be extended, so that the webrepl client will accept binary packages as well. File transfer with the old webrepl client will not work. Not that this is only a problem if a user navigates to http://micropython.org/webrepl manually and enters the IP. If a user navigates to the device directly, it will serve the correct webrepl client in any case
  • webrepl_cli (https://github.com/micropython/webrepl/blob/master/webrepl_cli.py) should work with webreplv2 for REPL, file transfer does not. I would suggest it to be superseeded by mpremote where (with tools/mpremote: Add ws:// and wss:// endpoints. #14141) file transfer and mounting works.

@andrewleech
Copy link
Contributor

This looks fantastic, thanks for your work!

I've been super keen to make mpremote work over network to a webrepl device but with 2 kids under 2 my project time has reduced pretty much to nil :-)

Just reading your description so far, I like how you've organised your charges. I'll review them in more detail when I can and hopefully poke the right people to get this merged (eventually, these things take time) because I think this is really important to have - network connected devices are more and more the norm now.

@cwalther
Copy link
Contributor

Sounds great, my own attempts at improving this have long stalled (I was trying to keep it backward compatible).

I would be a bit unhappy to lose the ability to transfer files while a program is running though. I never had any filesystem corruption with it. Maybe having all mpremote functionality will make up for it in practice, who knows.

I haven’t read the code carefully enough yet to tell: Are you putting just the stdio content into the binary websocket messages, or are the messages tagged with some kind of “opcode” field that would keep the protocol extensible, so that file transfer or other features that need a separate channel from stdio could be added back in the future? Lack of such extensibility was my main gripe with the original WebREPL protocol and I eventually gave up trying to think of convoluted ways to work around it. If we’re reimplementing things from scratch, it would be nice not to repeat that design flaw.

@felixdoerre
Copy link
Contributor Author

Are you putting just the stdio content into the binary websocket messages, or are the messages tagged with some kind of “opcode” field that would keep the protocol extensible, so that file transfer or other features that need a separate channel from stdio could be added back in the future

Previously the stdio content was in websocket "text" message (and "binary" message were used for that file transfer protocol), which does not work well, if stdio does not contain valid utf8 (as can be the case by the file transfer logic implemented in mpremote). Essentially, this change makes the websocket "binary" messages behave identical to the repl that you can get via uart or usb. In theory you could use the (now unused) text segments again to transfer some other data, but that feels more like a hassle.

I would be a bit unhappy to lose the ability to transfer files while a program is running though.

I haven't personally played with aiorepl, but I think that this might be the safer way to transfer files "in the background" (and would also work via usb/uart). That way, the transfer (blocks) only happen in between running application code and can not interrupt/interleave with another file system operation. I haven't tested this yet, but I might.

other features that need a separate channel from stdio could be added back in the future

Which other features for an additional channel would you want? In principle mpremote mount needs and establishes such a separate "channel", by putting an escape character before all mpremote mount messages. In principle nothing is preventing us from adding more such escape characters, or even nest them. However this technique only works will as long as the application does not output the escape character on stdout. One could start escaping the application's stdout, but I am not sure that this is wanted for the sake of keeping things simple.

Having another "channel" towards the device is probably more tricky, as the stdin buffer might get filled up and block other channels if it is not read. Using asyncio with aiorepl that could proably be avoided, as this logic would make sure, that stdin is checked regularly.

@cwalther
Copy link
Contributor

I haven't personally played with aiorepl, but I think that this might be the safer way to transfer files "in the background" (and would also work via usb/uart).

Good idea. I assume that only works in asyncio applications though. So that wouldn’t have helped me in the situations where I found the background file transfer handy. But maybe it’s not a big deal.

other features that need a separate channel from stdio could be added back in the future

Which other features for an additional channel would you want?

I don’t have any particular ones in mind (other than file transfer), I’m more thinking of future updates that we don’t know about yet. Like when I was trying to solve the same problem you did (inability to transfer non-UTF-8 data) and was hitting a brick wall because the protocol was not extensible to do that in a backward-compatible way without jumping through ugly hoops.

In principle mpremote mount needs and establishes such a separate "channel", by putting an escape character before all mpremote mount messages. In principle nothing is preventing us from adding more such escape characters, or even nest them. However this technique only works will as long as the application does not output the escape character on stdout. One could start escaping the application's stdout, but I am not sure that this is wanted for the sake of keeping things simple.

Hmm, mpremote mount does not do that already? (I have never studied how exactly it works.) So you could confuse it by outputting certain bytes today? That sounds like a design flaw (similar to the one in the original WebREPL that we’re trying to fix here).

@felixdoerre
Copy link
Contributor Author
felixdoerre commented Apr 19, 2024 •
edited
Loading

You can confuse mpremote mount by printing the mount escape char: sys.stdout.write("\x18"). So I am arguing that mpremote over uart/usb faces the same challenge.

I never had any filesystem corruption with it

I believe that one reason, that you didn't observe this, is that your applications only write to flash very rarely, and you only upload files very rarely, so the chance of those two operations (writes on both sides) interfere is pretty low.

@vshymanskyy
Copy link
Contributor
vshymanskyy commented Jun 6, 2024 •
edited
Loading

Webrepl improvements could definitely be leveraged by MicroPython Web IDE:
https://github.com/orgs/micropython/discussions/15219

@dpgeorge
Copy link
Member

@felixdoerre I like your overall proposal. It definitely makes sense to leverage mpremote at the front end for webrepl interaction, and then webrepl itself simply becomes a transport that mpremote can use.

Then, instead of having a new webreplv2 interface (that has a subset of mpremote within it), I think it would be more general to instead port mpremote to run in the browser. Then it could support WebSerial as well as webrepl/websockets to connect via a serial port. That way you could use mpremote in the browser as a complete replacement for mpremote on the command line, if you wanted to. It would also mean less duplication of code, no need to implement the mpremote protocol in JavaScript.

We can use pyscript to port mpremote to the browser. You would be able to just run the existing mpremote Python code directly in the browser. Then it's just a matter of adding a browser-based UI to it.

I think the main sticking point with all of this is the removal of the webrepl background file transfer protocol. Honestly I think we just need to remove it and then find new/different ways to do the same thing. Eg with aiorepl as mentioned above. Or if it's really such a useful feature then we should add it to the normal serial REPL as a special "background REPL" mode (but make it safe with respect to concurrency of filesystem access).

@vshymanskyy
Copy link
Contributor
vshymanskyy commented Jun 26, 2024 •
edited
Loading

@dpgeorge , the ViperIDE already is a port (more precisely, a rewrite) of mpremote to the browser.

@vshymanskyy
Copy link
Contributor

One of the downsides of using the mpremote for the file transfer, is that dupterm actually broadcasts the commands to all REPLs. It's only noticeable when you have both USB/Serial repl and WebREPL connected.

I'm not too worried about the interference between the two, but it probably slows down things a bit.

Maybe, we could introduce something like a virtual term in addition to dupterm...

@vshymanskyy
Copy link
Contributor
vshymanskyy commented Jun 26, 2024 •
edited
Loading

OR, maybe... treat RAW REPL mode in a special way, and skip duplicating the traffic?
Actually, RAW mode could act as critical section. This is what already happens on the ViperIDE level.

Are there any downsides to this?

@felixdoerre
Copy link
Contributor Author

We can use pyscript to port mpremote to the browser. You would be able to just run the existing mpremote Python code directly in the browser. Then it's just a matter of adding a browser-based UI to it.

I've spent some time to figure out the practicability of this, and have met two significant obstacles, to just running mpremote inside a pyscript shell in the browser:

  • Reading single bytes from stdin (or even characters) in pyscript seems not to be intended/implemented, and even if so, it might need to be async, which would require quite a lot or restructuring of mpremote.
  • The Terminal and Websocket class seem to be completely different and would probably need to be wrapped, and even then might require async, leading to more restructuring through mpremote.

I've raised mainly the first point pyscript/pyscript#2152 and I am not sure what to make of the response, as it mainly boils down to "this is too complicated the default pyscript usecase", and I should use the lower-level components and plug them into xterm myself. And at this point it seems far more complicated than implementing the basic websocket<->xterm bridge only, and ignoring other functions for the "official" webrepl web client.

At this point I am not sure how to continue.

@felixdoerre
Copy link
Contributor Author

Out of curiosity, I started patching around inside a local copy of pyscript to work around some of the issues and tried to start it inside the context, where we would expect the webrepl-client to run (loaded into a webpage served from the device), and I found another problem: pyscript uses, buried deep inside layers of abstraction, crypto.randomUUID, which is only available in https-contexts. And when we run the webrepl-web client, we usually run it in non-https-contexts, as the WebSocket is usually not served via https and opening http-Websockets from https-contexts is not allowed.

@vshymanskyy
Copy link
Contributor
vshymanskyy commented Sep 19, 2024 •
edited
Loading

This could be fixed on PyScript level, I guess?

Even when this is resolved, serving MPY VM from device just to make the website working will require storing additional ~180Kb just for the (pre-gzipped) wasm. WebREPL works it around by serving the editor from the web, and serving only a simple stub/redirector from the device. ViperIDE does the same thing.

I agree that using mpremote directly in web browser would be cool. I was able to reuse existing python infrastructure (python_minifier, mpy-tool) in the browser. At the same time, the device transports and REPL protocol are simple enough so it made sense to have them implemented in native JS, see:

@felixdoerre
Copy link
Contributor Author
felixdoerre commented Sep 19, 2024 •
edited
Loading

Even when this is resolved, serving MPY VM from device just to make the website working will require storing additional ~180Kb just for the (pre-gzipped) wasm.

I am not suggesting to store the resources on the device, only the stub that loads these resources, just like webrepl is doing right now. But no matter what, for the availability of crypto.randomUUID its not the protocol from which the script was served, but the protocol for the initial website, that counts, an that's via http for webrepl.

@vshymanskyy
Copy link
Contributor

Yes, I realise that. So, do you think we could get rid of secure context dependent stuff in PyScript?

@felixdoerre
Copy link
Contributor Author

I am more and more doubtful about that. Substituting the calls to crypto.randomUUID is probably possible, but to communicate with the Worker that runs the micropython-wasm module, pyscript wants to use SharedArrayBuffers (https://docs.pyscript.net/2024.9.1/faq/#sharedarraybuffer), and they also depend on a secure context (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#security_requirements)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Feature requests, new feature implementations
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@cwalther @vshymanskyy @andrewleech @dpgeorge @felixdoerre
0