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.

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

WebUSB: Always reset the serial terminal when closed #312

Merged
merged 4 commits into from
Dec 23, 2019
Merged

WebUSB: Always reset the serial terminal when closed #312

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5e792b
WebUSB: Refactor code to close serial terminal into own function.
microbit-carlos Dec 18, 2019
f2ff704
WebUSB: Focus REPL when activated, and remove all daplink serial list…
microbit-carlos Dec 18, 2019
fc5842a
WebUSB: return promise during disconnection.
microbit-carlos Dec 18, 2019
0efa8a8
WebUSB: Clear the dapwrapper only after disconnection.
microbit-carlos Dec 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 49 additions & 81 deletions python-main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,22 +1258,11 @@ function web_editor(config) {
webusbErrorHandler(error);
}

function clearDapWrapper(event) {
if(window.dapwrapper || window.previousDapWrapper) {
window.dapwrapper = null;
window.previousDapWrapper = null;
}
}

Comment on lines -1261 to -1267
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Function removed from here, but code still present in webusbErrorHandler():

https://github.com/bbcmicrobit/PythonEditor/blob/0efa8a8dadc09161b89be3ce607621a7827b01a0/python-main.js#L1332-LL1342

function doConnect(serial) {
// Change button to connecting
$("#command-connect").hide();
$("#command-connecting").show();
$("#command-disconnect").hide();

// Device disconnect listener
// Clears dapwrapper
navigator.usb.addEventListener('disconnect', clearDapWrapper);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed from here since the disconnect event already executes showDisconnectError().

showDisconnectError() -> webusbErrorHandler() -> clearDapWrapper()


// Show error on WebUSB Disconnect Events
navigator.usb.addEventListener('disconnect', showDisconnectError);
Expand Down Expand Up @@ -1340,19 +1329,16 @@ function web_editor(config) {
console.log(err);
console.trace();

// If there was an error and quick flash is in use, then clear dapwrapper
if(usePartialFlashing) {
if(window.dapwrapper) {
// Disconnect from the microbit
doDisconnect().then(function() {
// As there has been an error clear the partial flashing DAPWrapper
if (window.dapwrapper) {
window.dapwrapper = null;
}

if(window.previousDapWrapper) {
if (window.previousDapWrapper) {
window.previousDapWrapper = null;
}
}

// Disconnect from the microbit
doDisconnect();
Copy link
Collaborator Author

@microbit-carlos microbit-carlos Dec 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Before this PR, at this point of we had already set to null the window.dapwrapper and window.previousDapWrapper variables above before executing doDisconnect().

So doDisconnect() never attempted to execute disconnectAsync() since we had if (window.dapwrapper) window.dapwrapper.disconnectAsync(); and dapwrapper was already falsy.

});

var errorType;
var errorTitle;
Expand Down Expand Up @@ -1462,7 +1448,7 @@ function web_editor(config) {
$('#flashing-overlay').keydown(function(e) {
if (e.which == 27) {
flashErrorClose();
}
}
});

// Send event
Expand All @@ -1478,22 +1464,13 @@ function web_editor(config) {
}

function doDisconnect() {

// Remove disconnect listenr
// Remove disconnect listener
navigator.usb.removeEventListener('disconnect', showDisconnectError);

// Hide serial and disconnect if open
if ($("#repl").css('display') != 'none') {
$("#repl").hide();
$("#request-repl").hide();
$("#request-serial").hide();
$("#editor-container").show();
closeSerial();
}
$("#command-serial").attr("title", config["translate"]["static-strings"]["buttons"]["command-serial"]["title"]);
$("#command-serial > .roundlabel").text(config["translate"]["static-strings"]["buttons"]["command-serial"]["label"]);

$("#repl").empty();
REPL = null;

// Change button to connect
$("#command-disconnect").hide();
Expand All @@ -1506,22 +1483,23 @@ function web_editor(config) {

var p = Promise.resolve();

if (usePartialFlashing) {
if (window.dapwrapper) {
console.log("Disconnecting: Using Quick Flash");
p = p.then(function() { window.dapwrapper.daplink.stopSerialRead() } )
.then(function() { window.dapwrapper.disconnectAsync() } );
}
if (usePartialFlashing && window.dapwrapper) {
console.log('Disconnecting: Using Quick Flash');
p = p.then(function() { return window.dapwrapper.disconnectAsync() });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@microbit-sam I think these two were meant to return the disconnect promise?

I assume this was originally an arrow function .then(() => window.dapwrapper.disconnectAsync()); that returned the promise from disconnectAsync(), but then when converting to ES5 the return was accidentally left out?

On the other hand stopSerialRead() does not return a promise, so the original line removed in this PR (p.then(function() { window.dapwrapper.daplink.stopSerialRead() } )) is correct in not using return in the function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, looks right!

}
else {
if (window.daplink) {
console.log("Disconnecting: Using Full Flash");
p = p.then(function() { window.daplink.stopSerialRead() } )
.then(function() { window.daplink.disconnect() } );
}
else if (window.daplink) {
console.log('Disconnecting: Using Full Flash');
p = p.then(function() { return window.daplink.disconnect() });
}

p.finally(function() {
p = p.catch(function() {
console.log('Error during disconnection');
document.dispatchEvent(new CustomEvent('webusb', { 'detail': {
'flash-type': 'webusb',
'event-type': 'error',
'message': 'error-disconnecting'
}}));
}).finally(function() {
console.log('Disconnection Complete');
document.dispatchEvent(new CustomEvent('webusb', { 'detail': {
'flash-type': 'webusb',
Expand All @@ -1538,21 +1516,7 @@ function web_editor(config) {

// Hide serial and disconnect if open
if ($("#repl").css('display') != 'none') {
$("#repl").hide();
$("#request-repl").hide();
$("#request-serial").hide();
$("#editor-container").show();
$("#command-serial").attr("title", config["translate"]["static-strings"]["buttons"]["command-serial"]["title"]);
$("#command-serial > .roundlabel").text(config["translate"]["static-strings"]["buttons"]["command-serial"]["label"]);

if (usePartialFlashing) {
if (window.dapwrapper) {
window.dapwrapper.daplink.stopSerialRead();
}
}
else {
window.daplink.stopSerialRead();
}
closeSerial();
}

// Get the hex to flash in bytes format, exit if there is an error
Expand Down Expand Up @@ -1580,9 +1544,6 @@ function web_editor(config) {

var p = Promise.resolve();
if (usePartialFlashing) {
REPL = null;
$("#repl").empty();

p = window.dapwrapper.disconnectAsync()
.then(function() {
return PartialFlashing.connectDapAsync();
Expand Down Expand Up @@ -1640,28 +1601,35 @@ function web_editor(config) {
});
}

function closeSerial(keepSession) {
console.log("Closing Serial Terminal");
$('#repl').empty();
$('#repl').hide();
$('#request-repl').hide();
$('#request-serial').hide();
$('#editor-container').show();

var serialButton = config['translate']['static-strings']['buttons']['command-serial'];
$('#command-serial').attr('title', serialButton['title']);
$('#command-serial > .roundlabel').text(serialButton['label']);

var daplink = usePartialFlashing ? window.dapwrapper.daplink : window.daplink;
daplink.stopSerialRead();
daplink.removeAllListeners(DAPjs.DAPLink.EVENT_SERIAL_DATA);
Copy link
Collaborator Author

@microbit-carlos microbit-carlos Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the main reason sometimes we were seeing double characters on the terminal.
I don't quite have a way to replicate the original issue reported by some users, @microbit-mark do you have any additional info on the problem? I couldn't find an issue in this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's mentioned in #253 and originally from
https://support.microbit.org/a/tickets/24489 However the user says that they are unsure whether they accidentally typed the extra characters or not, so it maybe hard to reproduce.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right, this is not the same then.
This issue would have caused all typed characters to appear twice. Before this PR I only saw this type of double characters very sporadically, but this refactor somehow exuberated the issue and this line fixed it, I was hoping it was the same, but it isn't.

REPL.uninstallKeyboard();
REPL.io.pop();
REPL = null;
}

function doSerial() {
console.log("Setting Up Serial Terminal");
// Hide terminal
// Hide terminal if it is currently shown
var serialButton = config["translate"]["static-strings"]["buttons"]["command-serial"];
if ($("#repl").css('display') != 'none') {
$("#repl").hide();
$("#request-repl").hide();
$("#request-serial").hide();
$("#editor-container").show();
$("#command-serial").attr("title", serialButton["label"]);
$("#command-serial > .roundlabel").text(serialButton["label"]);
if (usePartialFlashing) {
if (window.dapwrapper) {
window.dapwrapper.daplink.stopSerialRead();
}
}
else {
window.daplink.stopSerialRead();
}
closeSerial();
return;
}

console.log("Setting Up Serial Terminal");
// Check if we need to connect
if ($("#command-connect").is(":visible")){
doConnect(true);
Expand Down Expand Up @@ -1757,8 +1725,8 @@ function web_editor(config) {
$(overlayContainer).keydown(function(e) {
if (e.which == 27) {
modalMsgClose();
}
});
}
});
}

function formatMenuContainer(parentButtonId, containerId) {
Expand Down