From 63726044e491946923754f482715bce9e32a69c2 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Thu, 11 Apr 2019 10:39:56 -0400 Subject: [PATCH 1/8] Major change of motor state handling to increase reliability, clear responsibility of handling state, and readability of the code. BoostMotor now has a status getter/setter that replaces isOn() and is responsible for clearing various motor state parameters. A new BoostMotorState-enum contains the possible states a motor can be in. Since time-based motor commands really just trigger a BoostMotor.turnOn(), it's the opcodes that are responsible for setting the motor state. --- src/extensions/scratch3_boost/index.js | 110 ++++++++++++++++++------- 1 file changed, 79 insertions(+), 31 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index f1657f8f4..e6dd894da 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -219,6 +219,17 @@ const BoostMode = { UNKNOWN: 0 // Anything else will use the default mode (mode 0) }; +/** + * Enum for Boost motor states. + * @param {number} + */ +const BoostMotorState = { + OFF: 0, + ON_FOREVER: 1, + ON_FOR_TIME: 2, + ON_FOR_ROTATION: 3 +}; + /** * Helper function for converting a JavaScript number to an INT32-number * @param {number} number - a number @@ -297,7 +308,7 @@ class BoostMotor { * @type {boolean} * @private */ - this._status = BoostPortFeedback.IDLE; + this._status = BoostMotorState.OFF; /** * If the motor has been turned on or is actively braking for a specific duration, this is the timeout ID for @@ -394,13 +405,31 @@ class BoostMotor { } /** - * @return {boolean} - true if this motor is currently moving, false if this motor is off or braking. + * @return {BoostMotorState} - the motor's current state. */ - get isOn () { - if (this._status & (BoostPortFeedback.BUSY_OR_FULL ^ BoostPortFeedback.IN_PROGRESS)) { - return true; + get status () { + return this._status; + } + + /** + * @param {BoostMotorState} value - set this motor's state. + */ + set status (value) { + switch (value) { + case BoostMotorState.OFF: + case BoostMotorState.ON_FOREVER: + this._clearRotationState(); + this._clearTimeout(); + break; + case BoostMotorState.ON_FOR_TIME: + this._clearRotationState(); + break; + case BoostMotorState.ON_FOR_ROTATION: + this._clearRotationState(); + this._clearTimeout(); + break; } - return false; + this._status = value; } /** @@ -452,7 +481,6 @@ class BoostMotor { MathUtil.clamp(this.power + BoostMotorMaxPowerAdd, 0, 100), BoostMotorProfile.DO_NOT_USE ]); - this._status = BoostPortFeedback.BUSY_OR_FULL; this._parent.send(BoostBLE.characteristic, cmd); @@ -497,7 +525,6 @@ class BoostMotor { ] ); - this._pendingPositionOrigin = this.position; this._pendingPositionDestination = this.position + (degrees * this.direction * direction); this._parent.send(BoostBLE.characteristic, cmd); } @@ -556,6 +583,19 @@ class BoostMotor { this._pendingTimeoutStartTime = Date.now(); this._pendingTimeoutDelay = delay; } + + /** + * Clear the motor states related to rotation-based commands, if any. + * Safe to call even when there is no pending timeout. + * @private + */ + _clearRotationState () { + if (this._pendingPromiseFunction) { + this._pendingPromiseFunction(); + this._pendingPromiseFunction = null; + } + this._pendingPositionDestination = null; + } } /** @@ -953,13 +993,13 @@ class Boost { const feedback = data[4]; const motor = this.motor(portID); if (motor) { - motor._status = feedback; // Makes sure that commands resolve both when they actually complete and when they fail const isBusy = feedback & BoostPortFeedback.IN_PROGRESS; const commandCompleted = feedback & (BoostPortFeedback.COMPLETED ^ BoostPortFeedback.DISCARDED); - if (!isBusy && commandCompleted && motor.pendingPromiseFunction) { - motor.pendingPromiseFunction(); - motor.pendingPromiseFunction = null; + if (!isBusy && commandCompleted) { + if (motor.status === BoostMotorState.ON_FOR_ROTATION) { + motor.status = BoostMotorState.OFF; + } } } break; @@ -1619,6 +1659,7 @@ class Scratch3BoostBlocks { this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); if (motor) { + motor.status = BoostMotorState.ON_FOR_TIME; motor.turnOnFor(durationMS); } }); @@ -1654,12 +1695,8 @@ class Scratch3BoostBlocks { const promises = motors.map(portID => { const motor = this._peripheral.motor(portID); if (motor) { - if (motor.pendingPromiseFunction) { - // If there's already a promise for this motor it must be resolved to avoid hanging blocks. - motor.pendingPromiseFunction(); - motor.pendingPromiseFunction = null; - } return new Promise(resolve => { + motor.status = BoostMotorState.ON_FOR_ROTATION; motor.pendingPromiseFunction = resolve; motor.turnOnForDegrees(degrees, sign); }); @@ -1684,6 +1721,7 @@ class Scratch3BoostBlocks { this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); if (motor) { + motor.status = BoostMotorState.ON_FOREVER; motor.turnOn(); } }); @@ -1706,6 +1744,7 @@ class Scratch3BoostBlocks { this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); if (motor) { + motor.status = BoostMotorState.OFF; motor.turnOff(); } }); @@ -1729,15 +1768,18 @@ class Scratch3BoostBlocks { const motor = this._peripheral.motor(motorIndex); if (motor) { motor.power = MathUtil.clamp(Cast.toNumber(args.POWER), 0, 100); - if (motor.isOn) { - if (motor.pendingTimeoutDelay) { - motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); - } else if (motor.pendingPromiseFunction) { - const p = Math.abs(motor.pendingPositionDestination - motor.position); - motor.turnOnForDegrees(p, Math.sign(p)); - } else { - motor.turnOn(); - } + switch (motor.status) { + case BoostMotorState.ON_FOREVER: + motor.turnOn(); + break; + case BoostMotorState.ON_FOR_TIME: + motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); + break; + case BoostMotorState.ON_FOR_ROTATION: { + const p = Math.abs(motor.pendingPositionDestination - motor.position); + motor.turnOnForDegrees(p, Math.sign(p)); + break; + } } } }); @@ -1770,14 +1812,20 @@ class Scratch3BoostBlocks { break; } // keep the motor on if it's running, and update the pending timeout if needed - if (motor.isOn) { - if (motor.pendingTimeoutDelay) { + if (motor) { + motor.power = MathUtil.clamp(Cast.toNumber(args.POWER), 0, 100); + switch (motor.status) { + case BoostMotorState.ON_FOREVER: + motor.turnOn(); + break; + case BoostMotorState.ON_FOR_TIME: motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); - } else if (motor.pendingPromiseFunction) { + break; + case BoostMotorState.ON_FOR_ROTATION: { const p = Math.abs(motor.pendingPositionDestination - motor.position); motor.turnOnForDegrees(p, Math.sign(p)); - } else { - motor.turnOn(); + break; + } } } } From ba2aaf90dd55dc5c16909310c9f88e40e1fd58d0 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Thu, 11 Apr 2019 11:12:52 -0400 Subject: [PATCH 2/8] Corrected documentation for BoostMotor._clearRotationState() --- src/extensions/scratch3_boost/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index e6dd894da..a1be433ba 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -586,7 +586,7 @@ class BoostMotor { /** * Clear the motor states related to rotation-based commands, if any. - * Safe to call even when there is no pending timeout. + * Safe to call even when there is no pending promise function. * @private */ _clearRotationState () { From c3908b5f2cfe7e4ab26b0eaf63567e8842a1c59d Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Thu, 11 Apr 2019 11:16:51 -0400 Subject: [PATCH 3/8] removed power wrongly being set in setMotorDirection() --- src/extensions/scratch3_boost/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index a1be433ba..b22916b4e 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -1813,7 +1813,6 @@ class Scratch3BoostBlocks { } // keep the motor on if it's running, and update the pending timeout if needed if (motor) { - motor.power = MathUtil.clamp(Cast.toNumber(args.POWER), 0, 100); switch (motor.status) { case BoostMotorState.ON_FOREVER: motor.turnOn(); From cd7319d044aea166a6458b4ef305f5a09d4cafed Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Thu, 11 Apr 2019 14:07:07 -0400 Subject: [PATCH 4/8] Added state-change for Boost.stopAllMotors() --- src/extensions/scratch3_boost/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index b22916b4e..3049d1ee4 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -719,6 +719,7 @@ class Boost { // Send the motor off command without using the rate limiter. // This allows the stop button to stop motors even if we are // otherwise flooded with commands. + motor.status = BoostMotorState.OFF; motor.turnOff(false); } }); From 3f0816bac8523551c97f8d3bc8688d9506d9db14 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Fri, 12 Apr 2019 11:33:10 -0400 Subject: [PATCH 5/8] This commit addresses point 1 from the discussion with @ericrosenbaum around moving the setting of motor-state into the BoostMotor-class rather than having it in the opcodes. - turnOn() renamed to _turnOn() and marked as a private function, i.e. it should only be called by BoostMotor-functions, not opcodes. - New function turnOnForever() to be called by opcodes. - turnOff() now sets the motor state. - _clearRotationState now does a check for null rather than a truthy value - all motor state setting removed from Boost-class and opcodes: stopAllMotors(), motorOnFor(), motorOnForRotation(), motorOn(), motorOff() - turnOnForever(), turnOnFor() and turnOnForDegrees() now have a resetState-parameter with the default value of true. This allows the setMotorPower() and setMotorDirection()-functions to not reset state, to avoid them resolving the promises of the original motor commands that they are affecting. --- src/extensions/scratch3_boost/index.js | 53 +++++++++++++++----------- 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index 3049d1ee4..ea2d7c7be 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -468,9 +468,10 @@ class BoostMotor { } /** - * Turn this motor on indefinitely. + * Turn this motor on indefinitely + * @private */ - turnOn () { + _turnOn () { if (this._power === 0) return; const cmd = this._parent.generateOutputCommand( this._index, @@ -483,19 +484,29 @@ class BoostMotor { ]); this._parent.send(BoostBLE.characteristic, cmd); + } - this._clearTimeout(); + /** + * Turn this motor on indefinitely + * @param {boolean} [resetState=true] - whether to reset the state of the motor when running this command. + */ + turnOnForever (resetState = true){ + if (this.power === 0) return; + if (resetState) this.status = BoostMotorState.ON_FOREVER; + this._turnOn(); } /** * Turn this motor on for a specific duration. * @param {number} milliseconds - run the motor for this long. + * @param {boolean} [resetState=true] - whether to reset the state of the motor when running this command. */ - turnOnFor (milliseconds) { + turnOnFor (milliseconds, resetState = true) { if (this.power === 0) return; milliseconds = Math.max(0, milliseconds); - this.turnOn(); + if (resetState) this.status = BoostMotorState.ON_FOR_TIME; + this._turnOn(); this._setNewTimeout(this.turnOff, milliseconds); } @@ -503,10 +514,11 @@ class BoostMotor { * Turn this motor on for a specific rotation in degrees. * @param {number} degrees - run the motor for this amount of degrees. * @param {number} direction - rotate in this direction + * @param {boolean} [resetState=true] - whether to reset the state of the motor when running this command. */ - turnOnForDegrees (degrees, direction) { + turnOnForDegrees (degrees, direction, resetState = true) { if (this.power === 0) { - this.pendingPromiseFunction(); + this._clearRotationState(); return; } @@ -524,7 +536,8 @@ class BoostMotor { BoostMotorProfile.DO_NOT_USE ] ); - + + if (resetState) this.status = BoostMotorState.ON_FOR_ROTATION; this._pendingPositionDestination = this.position + (degrees * this.direction * direction); this._parent.send(BoostBLE.characteristic, cmd); } @@ -547,6 +560,7 @@ class BoostMotor { ] ); + this.status = BoostMotorState.OFF; this._parent.send(BoostBLE.characteristic, cmd, useLimiter); } @@ -590,7 +604,7 @@ class BoostMotor { * @private */ _clearRotationState () { - if (this._pendingPromiseFunction) { + if (this._pendingPromiseFunction !== null) { this._pendingPromiseFunction(); this._pendingPromiseFunction = null; } @@ -719,7 +733,6 @@ class Boost { // Send the motor off command without using the rate limiter. // This allows the stop button to stop motors even if we are // otherwise flooded with commands. - motor.status = BoostMotorState.OFF; motor.turnOff(false); } }); @@ -1660,7 +1673,6 @@ class Scratch3BoostBlocks { this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); if (motor) { - motor.status = BoostMotorState.ON_FOR_TIME; motor.turnOnFor(durationMS); } }); @@ -1697,9 +1709,8 @@ class Scratch3BoostBlocks { const motor = this._peripheral.motor(portID); if (motor) { return new Promise(resolve => { - motor.status = BoostMotorState.ON_FOR_ROTATION; - motor.pendingPromiseFunction = resolve; motor.turnOnForDegrees(degrees, sign); + motor.pendingPromiseFunction = resolve; }); } return null; @@ -1722,8 +1733,7 @@ class Scratch3BoostBlocks { this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); if (motor) { - motor.status = BoostMotorState.ON_FOREVER; - motor.turnOn(); + motor.turnOnForever(); } }); @@ -1745,7 +1755,6 @@ class Scratch3BoostBlocks { this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); if (motor) { - motor.status = BoostMotorState.OFF; motor.turnOff(); } }); @@ -1771,13 +1780,13 @@ class Scratch3BoostBlocks { motor.power = MathUtil.clamp(Cast.toNumber(args.POWER), 0, 100); switch (motor.status) { case BoostMotorState.ON_FOREVER: - motor.turnOn(); + motor.turnOnForever(false); break; case BoostMotorState.ON_FOR_TIME: - motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); + motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now(), false); break; case BoostMotorState.ON_FOR_ROTATION: { - const p = Math.abs(motor.pendingPositionDestination - motor.position); + const p = Math.abs(motor.pendingPositionDestination - motor.position, false); motor.turnOnForDegrees(p, Math.sign(p)); break; } @@ -1816,14 +1825,14 @@ class Scratch3BoostBlocks { if (motor) { switch (motor.status) { case BoostMotorState.ON_FOREVER: - motor.turnOn(); + motor.turnOnForever(false); break; case BoostMotorState.ON_FOR_TIME: - motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); + motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now(), false); break; case BoostMotorState.ON_FOR_ROTATION: { const p = Math.abs(motor.pendingPositionDestination - motor.position); - motor.turnOnForDegrees(p, Math.sign(p)); + motor.turnOnForDegrees(p, Math.sign(p), false); break; } } From 8ece9757aa51e6f74c2c2b676d0adab1ce1f6a8d Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Fri, 12 Apr 2019 12:27:54 -0400 Subject: [PATCH 6/8] changes BoostMotor.status(value) to reset all motor state --- src/extensions/scratch3_boost/index.js | 31 ++++++-------------------- 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index ea2d7c7be..b46b546f5 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -415,20 +415,9 @@ class BoostMotor { * @param {BoostMotorState} value - set this motor's state. */ set status (value) { - switch (value) { - case BoostMotorState.OFF: - case BoostMotorState.ON_FOREVER: - this._clearRotationState(); - this._clearTimeout(); - break; - case BoostMotorState.ON_FOR_TIME: - this._clearRotationState(); - break; - case BoostMotorState.ON_FOR_ROTATION: - this._clearRotationState(); - this._clearTimeout(); - break; - } + // Clear any time- or rotation-related state from motor and set new status. + this._clearRotationState(); + this._clearTimeout(); this._status = value; } @@ -472,7 +461,7 @@ class BoostMotor { * @private */ _turnOn () { - if (this._power === 0) return; + if (this.power === 0) return; const cmd = this._parent.generateOutputCommand( this._index, BoostOutputExecution.EXECUTE_IMMEDIATELY, @@ -1672,9 +1661,7 @@ class Scratch3BoostBlocks { return new Promise(resolve => { this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); - if (motor) { - motor.turnOnFor(durationMS); - } + if (motor) motor.turnOnFor(durationMS); }); // Run for some time even when no motor is connected @@ -1732,9 +1719,7 @@ class Scratch3BoostBlocks { // TODO: cast args.MOTOR_ID? this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); - if (motor) { - motor.turnOnForever(); - } + if (motor) motor.turnOnForever(); }); return new Promise(resolve => { @@ -1754,9 +1739,7 @@ class Scratch3BoostBlocks { // TODO: cast args.MOTOR_ID? this._forEachMotor(args.MOTOR_ID, motorIndex => { const motor = this._peripheral.motor(motorIndex); - if (motor) { - motor.turnOff(); - } + if (motor) motor.turnOff(); }); return new Promise(resolve => { From a98f3af2e1bea648e51a184897c42ddb75d1bb8b Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Fri, 12 Apr 2019 13:56:29 -0400 Subject: [PATCH 7/8] Added a special case in motorOnForRotation() to avoid hanging blocks if power is 0 --- src/extensions/scratch3_boost/index.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index b46b546f5..87f1937f1 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -1695,6 +1695,8 @@ class Scratch3BoostBlocks { const promises = motors.map(portID => { const motor = this._peripheral.motor(portID); if (motor) { + // to avoid a hanging block if power is 0, return an immediately resolving promise. + if (motor.power === 0) return new Promise(resolve => resolve()); return new Promise(resolve => { motor.turnOnForDegrees(degrees, sign); motor.pendingPromiseFunction = resolve; From 12e969119aff23e17068fa36f6a83d8655ced81e Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Fri, 12 Apr 2019 14:07:26 -0400 Subject: [PATCH 8/8] Simplified the return value for when power is 0 in motorOnForRotation() --- src/extensions/scratch3_boost/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index 87f1937f1..a791c5bb3 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -1696,7 +1696,7 @@ class Scratch3BoostBlocks { const motor = this._peripheral.motor(portID); if (motor) { // to avoid a hanging block if power is 0, return an immediately resolving promise. - if (motor.power === 0) return new Promise(resolve => resolve()); + if (motor.power === 0) return Promise.resolve(); return new Promise(resolve => { motor.turnOnForDegrees(degrees, sign); motor.pendingPromiseFunction = resolve;