Solo 200 resubmit G - Handle differences in encoding between launcher versions #337
Conversation
|
@PropGit - If this is fixed in the Launcher, do we still need to fiddle with the stream on our side? This seems like we're trying to compensate for an issue that we can address in the Launcher. |
|
No - you can't address this only on the launcher side. There is some code
on the client side that can come back out once we are confident that every
version of the launcher fully encodes it's streams in both directions, but
until then, we need a couple branches to act accordingly.
…On Wed, Mar 4, 2020 at 10:30 AM Jim Ewald ***@***.***> wrote:
@PropGit <https://github.com/PropGit> - If this is fixed in the Launcher,
do we still need to fiddle with the stream on our side? This seems like
we're trying to compensate for an issue that we can address in the Launcher.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#337?email_source=notifications&email_token=AEWSVOGF3WU6XO34GGKHBI3RF2M5FA5CNFSM4LBPTIF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENZOL4A#issuecomment-594732528>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEWSVOFKFSBR7YGMOAFGRG3RF2M5FANCNFSM4LBPTIFQ>
.
|
|
So you're saying that we are applying a workaround to an error in the client/launcher that we won't fix for some reason? I don't see the logic in that at all. |
|
Not at all what I am saying. There is a PR in place to fix it on the BPL side, too, but we will need to support both the fixed and broken versions for a while, until we set the minimum allowed version ahead of the version of the BPL that's fixed. |
|
I highly disagree with saying the BP Launcher is "broken" or there's an "error in the client/launcher" that caused this. That is not the case. What happened is, we didn't enforce the encoding in all communications, we only encoded in places where we had noted a problem in getting data from the Launcher to the web side. The Launcher code has been the way it was prior to this issue since 2017 with no problems regarding this particular communication that I know of. It's been in use from Chromebooks since then. The issue here is that the inconsistency was noticed and the code was changed on the web side to treat it as encoded, plus a PR was submitted to Launcher repo to encode it, but apparently no protections were put in place to accept both unencoded or encoded in that place (transitional code) to support the existing Launchers in the wild plus the first Launcher that will feature the encoding in that place. I'm not a fan of encoding everything because it further slows down communication; we already have a constrained max speed exhibiting itself in the the Terminal. At this point, I will indeed absorb that PR into Launcher and move forward. |
|
Thanks, I agree. Nothing was show-stoppingly broken, there were just a
couple of inconsistencies that were fairly harmless, and this is meant to
shore them up.
FWIW - base64 coding creates an overhead of roughly 35%-40% (every 3 bytes
becomes 4 after encoding, plus the time need to do the encoding itself).
Websockets are blazingly fast, so while there is a small amount of visible
latency, without encoding non-printable characters break the comms. I
agree - it would be nice if we didn't need them. Unfortunately cursor
positioning is done with ASCII 0-16, and sending one of those raw through a
websocket causes it to choke.
Comms from the BPL to the browser have been encoded for some time. This
encodes the comms from the browser to the BPL. Given that the only way to
currently "send" to the BPL is to type into the browser, the additional
overhead won't be noticed, because I don't think anyone can physically type
at that high of a baudrate :)
What was somewhat inconsistent, and this gets addressed in these PRs, is
that one status/error message from the BPL to the browser was NOT encoded,
when all others were. It did create an error in the background if you
tried to open the terminal with no devices connected, but it didn't lock or
break the browser, so unless you were looking for that bug, you wouldn't
notice it as a problem...
…On Thu, Mar 5, 2020 at 9:08 AM Parallax Git Administrator < ***@***.***> wrote:
I highly disagree with saying the BP Launcher is "broken" or there's an
"error in the client/launcher" that caused this. That is not the case. What
happened is, we didn't enforce the encoding in all communications, we only
encoded in places where we had noted a problem in getting data from the
Launcher to the web side. The Launcher code has been the way it was prior
to this issue since 2017 with no problems regarding this particular
communication that I know of. It's been in use from Chromebooks since then.
The issue here is that the inconsistency was noticed and the code was
changed on the web side to treat it as encoded, plus a PR was submitted to
Launcher repo to encode it, but apparently no protections were put in place
to accept both unencoded or encoded in that place (transitional code) to
support the existing Launchers in the wild plus the first Launcher that
will feature the encoding in that place.
I'm not a fan of encoding everything because it further slows down
communication; we already have a constrained max speed exhibiting itself in
the the Terminal. At this point, I will indeed absorb that PR into Launcher
and move forward.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#337?email_source=notifications&email_token=AEWSVOH4BONHQGC3BHRUCYTRF7MA7A5CNFSM4LBPTIF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN6DADQ#issuecomment-595341326>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEWSVOEAONWLRJENXPF2J5DRF7MA7ANCNFSM4LBPTIFQ>
.
|
|
Okay, thanks for the clear details. Notes on current BPL work: |
Continues addressing #279
The launcher was not encoding one direction of its serial comms stream. There is a PR in the lanucher's repo holding on this change:
BlocklyPropLanucher-96
This adds code to compensate for that issue across all versions of the launcher.