Add packet transfer protocol

[rikkuness]

Will keep this open as a WIP and add to it as I work things out to make sure I don't end up too far down a wrong path.

Fixes #187

[gfwilliams]

Looks good so far! I think given EventEmitter isn't on browser it might be worth adding .on/etc to Espruino.Core.Serial in the basic way done at https://github.com/espruino/EspruinoWebTools/blob/master/uart.js#L401-L409 - then add the packet parsing stuff (https://github.com/espruino/EspruinoWebTools/blob/master/uart.js#L437-L471) in where we do XON/XOFF https://github.com/espruino/EspruinoTools/blob/master/core/serial.js#L222 ?

I guess the input data handler would ideally be its own function rather than buried inside openSerialInternal?

Right now we don't remove XON/XOFF chars from the input stream, but I think we'd probably want to do that now (as we need to be able to remove the packet ACK/NAK anyway) - and to reliably receive binary data I think we need to ignore XON/XOFF when in the middle of receiving a binary packet.

[rikkuness]

Yeah missing the events stuff was catching me out, and by first stab at writing the packet parsing I did using Buffer before realising that's also not in browser 😂

The ACK/NACK does that come right after the write, so there's no control chars or anything as a preamble? Just that window of within a second after a write expect a 6 or 21 char?

Another thing I wasn't sure on after reading more about the protocol (and I've not dug too much into the C code yet), with something like a FILE_RECV you expect to get back n packets and then completing on getting back a packet with 0 in the length? How about for an EVAL? If I for example ran an eval that listed the contents of the filesystem which wouldn't fit in an 8191 byte frame, do you get multiple RESPONSE packets, or do you just have the limitation of that size of response?

[gfwilliams]

Yeah, it's a pain not being able to rely on certain things being built in (atob/Buffer/etc) - EspruinoTools/WebIDE feels a bit hacky but really I've just been trying to get something as simple as possible with as little dependencies as I can (and not relying on transpiling so you can self-host/debug easily).

Just that window of within a second after a write expect a 6 or 21 char?

Correct, yes.

...wouldn't fit in an 8191 byte frame, do you get multiple RESPONSE packets, or do you just have the limitation of that size of response?

There's just a limitiation of response size (it's just one packet). Not ideal, but it's simple.

One thing to note is the response is produced by E.toJS so you get stuff like {a:1,b:2} which is more compact, but which JSON.parse won't touch - it's why we have parseRJSON

[gfwilliams]

This is looking really good so far - is the 'eval' you added working at the moment?

[rikkuness]

I've been having mixed results 😄 I was going to have a chat to you about it for a bit of a direction steer lest I wander down a wrong path.

I'll push my latest code, but so far I've been testing with:

esp.init(() => {
  Espruino.Config.set("SERIAL_FLOW_CONTROL", false)
  Espruino.Core.Serial.open("COM17", (status) => {
    if (status === undefined) throw new Error("Failed to connect")

    Espruino.Core.Utils.executeExpressionV2('process.env', (data) => {
      console.log("process.env", data)
      
      Espruino.Core.Utils.downloadFileV2('FW.js', true, (data) => {
        console.log("FW.js", data)
      })
    })
  })
})

and I'm getting back the EVAL response okay, the download works okay if I disable flow control, but it's still not quite there as I'm not properly collecting the 0 length packet to close out the frame.

I'm also not sure how to best handle concurrency and how that goes at the Espruino end, I've seen there's code that does queuing of transmit stuff, and then some of the evals was matching the request to responses maybe? Not sure how that works here, I know most things should be synchronous but wasn't sure the best way to enforce that in the code so that if a sent another EVAL while one was still ongoing, how that would maybe queue and still tie up the expected responses etc.

I feel like a lot of these problems are already solved in some way in the code so I didn't want to just make v2 versions of every method and have both methods overlap and duplicate functionality if that makes sense. So happy for any feedback/input here 👍🏻

Edit:
Also I think I can improve the way I buffered the input data, when I do parsePacketsFromBuffer I think there's potential for error in the current state as I take out complete packets from the input stream but don't handle all cases where there may be incomplete packets. I think in the latest push I do buffer.fill(undefined, 0, dle + packet.length) so that I snip out a complete packet that's been emit but also all proceeding bytes, just so I don't end up with like half packets at the start of the data stream and keep invalid data out of order.

[gfwilliams]

Thanks - this is looking good!

the download works okay if I disable flow control

I imagine flow control will be 'eating' the XON/XOFF - what I'd do is what we do in uart.js - handle the packets in the same code that handles ack/nack (ideally also xon/xoff but uart.js doesn't do that yet) and then emit a 'packet' event. Then you can ignore xon/xoff when in the middle of a packet (Espruino doesn't escape them as ideally it shouldn't be emitting XOFF during a packet as we're not sending stuff to it).

The other benefit is the IDE could listen for packets later on (I'd planned on having the ability to maybe send debug info via packets on a separate channel)

I'll have a play with this now and let you know how I get on.

I'm also not sure how to best handle concurrency and how that goes at the Espruino end

Well, it's not handled super well at the moment, but I think with the code you have you're okish. Espruino.Core.Serial.write will only call back after the data has been written, so in sendPacket you're only going to start listening for packets after you've sent the request.

Perhaps what we could do is make the callback you supply to Espruino.Core.Serial.write optionally return a promise - and if it returns a promise then Espruino.Core.Serial.write will wait until that completes before writing the next data that's in the queue?

We'd probably have to tweak the old 'eval' too but that would have suffered issues too

[gfwilliams]

I just made some changes at master...rikkuness-packetsend (aea870e) if that looks ok to you? I think file receive now works ok (with the zero packet) and it works alongside flow control.

Looking at this, one thing that I can't help thinking is that the Connection class in uart.js (https://github.com/espruino/EspruinoWebTools/blob/master/uart.js#L398) kind of does most of what we want already - and while your current code works, part of me is wondering whether I shouldn't pull the flow control code into the Connection class (as I'd hoped to at https://github.com/espruino/EspruinoWebTools/blob/master/uart.js#L432) and then maybe we could just copy the whole Connection class inside serial.js (or even have it as a separate file)?

It's not perfect, but there's a lot to be said for using the exact same code in the IDE and EspruinoWebTools (I could even copy it into the Puck.js library) so we can make maintenance easier later on.

[rikkuness]

Thanks for taking a look over it! Yeah sounds good to me, you're definitely much more familiar with the codebase so happy to take your lead on it 😄 I was a bit worried that some of what I had was diverging and I was duplicating anyway, makes sense to roll some of the WebTools stuff over here.

I guess once this has those elements this almost becomes the goto package and supersedes WebTools to some degree? May be a bit more of a sledgehammer to crack a nut in some cases but it'd be nice to have that lower maintainability

[gfwilliams]

Thanks! I've just pushed some changes to https://github.com/espruino/EspruinoWebTools

I'll see how well it works (it's now used for the Bangle.js App Loader) and will have a go at getting it into EspruinoTools

I guess once this has those elements this almost becomes the goto package and supersedes WebTools to some degree?

Yes, there's definitely an argument that the App Loader should be using it (as it uses the espruinotools package anyway).

UART.js/Puck.js libs started off as just a super-simple way to get access from the PC, and there seem to be a bunch of places that use them as-is - so I think they probably still have their place, but anything that's actually writing JS code to an Espruino device should probably end up using EspruinoTools I guess. The one thing it'll be missing is the menus for selecting which connection method to use (as they're currently in the IDE)

[gfwilliams]

Ok, I've just pushed some more changes to master...rikkuness-packetsend. I've tested with:

esp = require("./index.js");

esp.init(() => {
  console.log("Espruino.Core.Serial=",Espruino.Core.Serial);
  Espruino.Config.set("BAUD_RATE", 9600);
  Espruino.Core.Serial.open("/dev/ttyACM0", (status) => {
    if (status === undefined) throw new Error("Failed to connect")
    var connection = Espruino.Core.Serial.connection;

    connection.espruinoEval('process.env').then((data) => {
      console.log("process.env", data)

      connection.espruinoReceiveFile('testfile').then((data) => {
        console.log("testfile", data)
      })
    })
  })
})

How does this work for you?

So now we're using the same Connection class as UART.js, which feels like it tidies things up a little - it's not great but as far as I can tell the CLI and IDE still works which is good!

[rikkuness]

Hey, thanks for getting this over the line, sorry I wasn't around to test, day job has been hectic.

I'll have more of a play now everything is merged up and see how I get on 🙏🏻