From 40b2a50a9a77d3f320afe18055c7a7fe58d1ea3d Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 12:56:32 +0000 Subject: [PATCH 01/12] Fix for reset/panic overriding the user's stop. Switch to an enum to make the possible states easier to follow and so we can check we're in the default state before committing to a reset or panic. Closes https://github.com/microbit-foundation/micropython-microbit-v2-simulator/issues/86 --- src/board/index.ts | 87 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 67 insertions(+), 20 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index 9cf57b4c..22bf45b0 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -35,6 +35,36 @@ import { Radio } from "./radio"; import { RangeSensor, State } from "./state"; import { ModuleWrapper } from "./wasm"; +/** + * Controls the behaviour after the program has come to a stop. + * + * We use this to communicate between the start + */ +enum StopKind { + /** + * The main Wasm function returned control to us in a normal way. + */ + Default = "default", + /** + * The program called panic. + */ + Panic = "panic", + /** + * The user or the program requested a reset. + */ + Reset = "reset", + /** + * An internal mode where we do not display the stop state UI as we plan to immediately restart. + */ + BriefStop = "brief", + /** + * The user requested the program be interrupted. + * + * Note the program could finish for other reasons, but should always count as a user stop. + */ + UserStop = "user", +} + export class PanicError extends Error { constructor(public code: number) { super("panic"); @@ -108,10 +138,9 @@ export class Board { */ private module: ModuleWrapper | undefined; /** - * If undefined, then when main finishes we stay stopped. - * Otherwise we perform the action then clear this field. + * Controls the action after the user program completes. */ - private afterStopped: (() => void) | undefined; + private stopKind: StopKind = StopKind.Default; constructor( private notifications: Notifications, @@ -365,11 +394,17 @@ export class Board { this.displayRunningState(); await module.start(); } catch (e: any) { + // Take care not to overwrite another kind of stop just because the program + // called restart or panic. if (e instanceof PanicError) { - panicCode = e.code; + if (this.stopKind === StopKind.Default) { + this.stopKind = StopKind.Panic; + panicCode = e.code; + } } else if (e instanceof ResetError) { - const noChangeRestart = () => {}; - this.afterStopped = noChangeRestart; + if (this.stopKind === StopKind.Default) { + this.stopKind = StopKind.Reset; + } } else { this.notifications.onInternalError(e); } @@ -386,23 +421,36 @@ export class Board { this.modulePromise = undefined; this.module = undefined; - if (panicCode !== undefined) { - this.displayPanic(panicCode); - } else { - if (this.afterStopped) { - this.afterStopped(); - this.afterStopped = undefined; + switch (this.stopKind) { + case StopKind.Panic: { + if (panicCode === undefined) { + throw new Error("Must be set"); + } + this.displayPanic(panicCode!); + break; + } + case StopKind.Reset: { setTimeout(() => this.start(), 0); - } else { + break; + } + case StopKind.BriefStop: { + // Skip the stopped state. + break; + } + case StopKind.UserStop: /* Fall through */ + case StopKind.Default: { this.displayStoppedState(); + break; + } + default: { + throw new Error("Unknown stop action: " + this.stopKind); } } + this.stopKind = StopKind.Default; } - async stop( - afterStopped: (() => void) | undefined = undefined - ): Promise { - this.afterStopped = afterStopped; + async stop(kind: StopKind = StopKind.UserStop): Promise { + this.stopKind = kind; if (this.panicTimeout) { clearTimeout(this.panicTimeout); this.panicTimeout = null; @@ -425,8 +473,7 @@ export class Board { * reset() in MicroPython code throws ResetError. */ async reset(): Promise { - const noChangeRestart = () => {}; - this.stop(noChangeRestart); + this.stop(StopKind.Reset); } async flash(filesystem: Record): Promise { @@ -440,7 +487,7 @@ export class Board { }; if (this.modulePromise) { // If it's running then we need to stop before flash. - return this.stop(flashFileSystem); + await this.stop(StopKind.BriefStop); } flashFileSystem(); return this.start(); From 8666faa3f39584f284f2ac299306c1d1dddd39a7 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon <44397098+microbit-matt-hillsdon@users.noreply.github.com> Date: Mon, 31 Oct 2022 13:13:55 +0000 Subject: [PATCH 02/12] Review tweaks --- src/board/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index 22bf45b0..a565fc8e 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -426,7 +426,7 @@ export class Board { if (panicCode === undefined) { throw new Error("Must be set"); } - this.displayPanic(panicCode!); + this.displayPanic(panicCode); break; } case StopKind.Reset: { @@ -443,7 +443,7 @@ export class Board { break; } default: { - throw new Error("Unknown stop action: " + this.stopKind); + throw new Error("Unknown stop kind: " + this.stopKind); } } this.stopKind = StopKind.Default; From c8c4d44bbd20bac4b496d68bcca7bf8a4859b486 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 13:16:11 +0000 Subject: [PATCH 03/12] Fix floating promise. I don't think it was consequential as it's called only from the message handler which doesn't need to wait for the result. --- src/board/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/board/index.ts b/src/board/index.ts index a565fc8e..19af48ee 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -473,7 +473,7 @@ export class Board { * reset() in MicroPython code throws ResetError. */ async reset(): Promise { - this.stop(StopKind.Reset); + return this.stop(StopKind.Reset); } async flash(filesystem: Record): Promise { From dd131c9a3bf0061cda32b2e7eb005597a6ac1827 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 13:35:37 +0000 Subject: [PATCH 04/12] Don't set the stop kind if we're not running. --- src/board/index.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/board/index.ts b/src/board/index.ts index 19af48ee..b4ce5260 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -450,7 +450,6 @@ export class Board { } async stop(kind: StopKind = StopKind.UserStop): Promise { - this.stopKind = kind; if (this.panicTimeout) { clearTimeout(this.panicTimeout); this.panicTimeout = null; @@ -458,6 +457,8 @@ export class Board { this.displayStoppedState(); } if (this.modulePromise) { + this.stopKind = kind; + // Avoid this.module as we might still be creating it (async). const module = await this.modulePromise; module.requestStop(); From 237b6b6ef6a9912eafa289924680e4c983a878ca Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 13:50:21 +0000 Subject: [PATCH 05/12] Just use Reset for programmatic resets. --- src/board/index.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index b4ce5260..2957f9e0 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -50,11 +50,12 @@ enum StopKind { */ Panic = "panic", /** - * The user or the program requested a reset. + * The program requested a reset. */ Reset = "reset", /** - * An internal mode where we do not display the stop state UI as we plan to immediately restart. + * An internal mode where we do not display the stop state UI as we plan to immediately reset. + * Used for user-requested flash or reset. */ BriefStop = "brief", /** @@ -471,10 +472,10 @@ export class Board { /** * An external reset. - * reset() in MicroPython code throws ResetError. */ async reset(): Promise { - return this.stop(StopKind.Reset); + await this.stop(StopKind.BriefStop); + return this.start(); } async flash(filesystem: Record): Promise { From 2c173180e00478d339b16ca1bbd1ba739cf398f9 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 15:32:07 +0000 Subject: [PATCH 06/12] Cancel pending restarts as required. --- src/board/index.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index 2957f9e0..83a63758 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -142,6 +142,10 @@ export class Board { * Controls the action after the user program completes. */ private stopKind: StopKind = StopKind.Default; + /** + * Timeout for a pending start call due to StopKind.Restart. + */ + private pendingRestart: any; constructor( private notifications: Notifications, @@ -386,6 +390,8 @@ export class Board { if (this.modulePromise || this.module) { throw new Error("Module already exists!"); } + clearTimeout(this.pendingRestart); + this.pendingRestart = undefined; this.modulePromise = this.createModule(); const module = await this.modulePromise; @@ -431,7 +437,10 @@ export class Board { break; } case StopKind.Reset: { - setTimeout(() => this.start(), 0); + if (this.pendingRestart) { + throw new Error("Unexpected state"); + } + this.pendingRestart = setTimeout(() => this.start(), 0); break; } case StopKind.BriefStop: { @@ -457,9 +466,13 @@ export class Board { this.display.clear(); this.displayStoppedState(); } + if (this.pendingRestart) { + clearTimeout(this.pendingRestart); + this.pendingRestart = undefined; + this.displayStoppedState(); + } if (this.modulePromise) { this.stopKind = kind; - // Avoid this.module as we might still be creating it (async). const module = await this.modulePromise; module.requestStop(); From dcd8ab3dfcdc23987f74d0e1aeba1d0dbeb0db34 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 15:35:26 +0000 Subject: [PATCH 07/12] Simplify external interface. Reset/Panic are only applicable internally. --- src/board/index.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index 83a63758..b12f45ac 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -459,7 +459,7 @@ export class Board { this.stopKind = StopKind.Default; } - async stop(kind: StopKind = StopKind.UserStop): Promise { + async stop(brief: boolean = false): Promise { if (this.panicTimeout) { clearTimeout(this.panicTimeout); this.panicTimeout = null; @@ -472,7 +472,7 @@ export class Board { this.displayStoppedState(); } if (this.modulePromise) { - this.stopKind = kind; + this.stopKind = brief ? StopKind.BriefStop : StopKind.UserStop; // Avoid this.module as we might still be creating it (async). const module = await this.modulePromise; module.requestStop(); @@ -487,7 +487,7 @@ export class Board { * An external reset. */ async reset(): Promise { - await this.stop(StopKind.BriefStop); + await this.stop(true); return this.start(); } @@ -502,7 +502,7 @@ export class Board { }; if (this.modulePromise) { // If it's running then we need to stop before flash. - await this.stop(StopKind.BriefStop); + await this.stop(true); } flashFileSystem(); return this.start(); From 8f8c5c20f5220e957c518bfd46cad953d3635a63 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 15:43:56 +0000 Subject: [PATCH 08/12] Minor consistency tweaks. --- src/board/index.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index b12f45ac..754df056 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -35,11 +35,6 @@ import { Radio } from "./radio"; import { RangeSensor, State } from "./state"; import { ModuleWrapper } from "./wasm"; -/** - * Controls the behaviour after the program has come to a stop. - * - * We use this to communicate between the start - */ enum StopKind { /** * The main Wasm function returned control to us in a normal way. @@ -105,8 +100,6 @@ export class Board { radio: Radio; dataLogging: DataLogging; - private panicTimeout: any; - public serialInputBuffer: number[] = []; private stoppedOverlay: HTMLDivElement; @@ -140,12 +133,18 @@ export class Board { private module: ModuleWrapper | undefined; /** * Controls the action after the user program completes. + * + * Determined by a combination of user actions (stop, reset etc) and program actions. */ private stopKind: StopKind = StopKind.Default; /** * Timeout for a pending start call due to StopKind.Restart. */ private pendingRestart: any; + /** + * Timeout for the next frame of the panic animation. + */ + private panicTimeout: any; constructor( private notifications: Notifications, @@ -391,7 +390,7 @@ export class Board { throw new Error("Module already exists!"); } clearTimeout(this.pendingRestart); - this.pendingRestart = undefined; + this.pendingRestart = null; this.modulePromise = this.createModule(); const module = await this.modulePromise; @@ -468,7 +467,7 @@ export class Board { } if (this.pendingRestart) { clearTimeout(this.pendingRestart); - this.pendingRestart = undefined; + this.pendingRestart = null; this.displayStoppedState(); } if (this.modulePromise) { From fa3c9327dffaa38f8c9d8301d06d2a8723543a86 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 15:46:23 +0000 Subject: [PATCH 09/12] No longer useful. --- src/board/index.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index 754df056..550ba5e1 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -436,9 +436,6 @@ export class Board { break; } case StopKind.Reset: { - if (this.pendingRestart) { - throw new Error("Unexpected state"); - } this.pendingRestart = setTimeout(() => this.start(), 0); break; } From ec27341969fbc6a86b7da6a5b54697e133a9d1b5 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 15:47:49 +0000 Subject: [PATCH 10/12] Consistently avoid showing stopped state. --- src/board/index.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index 550ba5e1..fd840cfa 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -460,12 +460,16 @@ export class Board { clearTimeout(this.panicTimeout); this.panicTimeout = null; this.display.clear(); - this.displayStoppedState(); + if (!brief) { + this.displayStoppedState(); + } } if (this.pendingRestart) { clearTimeout(this.pendingRestart); this.pendingRestart = null; - this.displayStoppedState(); + if (!brief) { + this.displayStoppedState(); + } } if (this.modulePromise) { this.stopKind = brief ? StopKind.BriefStop : StopKind.UserStop; From 5794d7e291c31235522bc980d0639c75334430f5 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon <44397098+microbit-matt-hillsdon@users.noreply.github.com> Date: Mon, 31 Oct 2022 16:29:12 +0000 Subject: [PATCH 11/12] Comment fix from review Co-authored-by: Robert Knight <95928279+microbit-robert@users.noreply.github.com> --- src/board/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/board/index.ts b/src/board/index.ts index fd840cfa..c7eefacb 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -138,7 +138,7 @@ export class Board { */ private stopKind: StopKind = StopKind.Default; /** - * Timeout for a pending start call due to StopKind.Restart. + * Timeout for a pending start call due to StopKind.Reset. */ private pendingRestart: any; /** From 5657b7a13c00065753e10f133cbd7c8efd61bb35 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 31 Oct 2022 16:30:30 +0000 Subject: [PATCH 12/12] Rename for consistency. --- src/board/index.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/board/index.ts b/src/board/index.ts index c7eefacb..9f662c61 100644 --- a/src/board/index.ts +++ b/src/board/index.ts @@ -140,7 +140,7 @@ export class Board { /** * Timeout for a pending start call due to StopKind.Reset. */ - private pendingRestart: any; + private pendingRestartTimeout: any; /** * Timeout for the next frame of the panic animation. */ @@ -389,8 +389,8 @@ export class Board { if (this.modulePromise || this.module) { throw new Error("Module already exists!"); } - clearTimeout(this.pendingRestart); - this.pendingRestart = null; + clearTimeout(this.pendingRestartTimeout); + this.pendingRestartTimeout = null; this.modulePromise = this.createModule(); const module = await this.modulePromise; @@ -436,7 +436,7 @@ export class Board { break; } case StopKind.Reset: { - this.pendingRestart = setTimeout(() => this.start(), 0); + this.pendingRestartTimeout = setTimeout(() => this.start(), 0); break; } case StopKind.BriefStop: { @@ -464,9 +464,9 @@ export class Board { this.displayStoppedState(); } } - if (this.pendingRestart) { - clearTimeout(this.pendingRestart); - this.pendingRestart = null; + if (this.pendingRestartTimeout) { + clearTimeout(this.pendingRestartTimeout); + this.pendingRestartTimeout = null; if (!brief) { this.displayStoppedState(); }