From e3cdbffa2a96da31017e24484fbd75d21a19e2ad Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Thu, 4 Apr 2019 15:37:48 -0400 Subject: [PATCH 1/9] Resolves #2089. Partly resolves #2087, #2088. This was happening because scratch-vm is responsible for timed motor commands rather than using the Boost hubs commands to run motors for a specific amount of time. This meant that when we simply sent a motorOn-command, the hub would immediately return feedback for the motor that the command had completed. The fix for this was, for timed motor commands, to not receive feedback from the motor, and instead have scratch-vm modify the BoostMotor._status. TODO: Fix for rotation-based commands. Agreed. Changed to 50%. - BoostMotorMaxPower changed to BoostMotorMaxPowerAdd and now describes an extra boost (no pun intended) to the motor. Documentation added that describes the feature. - _colorBucket renamed to _colorSamples for clarity. - oldColor is renamed to previousColor, and is now initialized in this._sensors. - Modified documentation. --- src/extensions/scratch3_boost/index.js | 44 ++++++++++++++------------ 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index ccd7f6d5f..665fc3fd4 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -33,10 +33,13 @@ const BoostBLE = { }; /** - * Boost Motor Max Power. + * Boost Motor Max Power Add. Defines how much more power than the target speed + * the motors may supply to reach the target speed faster. + * Lower number == softer, slower reached target speed. + * Higher number == harder, faster reached target speed. * @constant {number} */ -const BoostMotorMaxPower = 100; +const BoostMotorMaxPowerAdd = 10; /** * A time interval to wait (in milliseconds) in between battery check calls. @@ -280,7 +283,7 @@ class BoostMotor { * @type {number} * @private */ - this._power = 100; + this._power = 50; /** * This motor's current relative position @@ -440,13 +443,14 @@ class BoostMotor { if (this._power === 0) return; const cmd = this._parent.generateOutputCommand( this._index, - BoostOutputExecution.EXECUTE_IMMEDIATELY ^ BoostOutputExecution.COMMAND_FEEDBACK, + BoostOutputExecution.EXECUTE_IMMEDIATELY, BoostOutputSubCommand.START_SPEED, [ this._power * this._direction, - BoostMotorMaxPower, + MathUtil.wrapClamp(this._power + BoostMotorMaxPowerAdd, 0, 100), BoostMotorProfile.DO_NOT_USE ]); + this._status = BoostPortFeedback.BUSY_OR_FULL; this._parent.send(BoostBLE.characteristic, cmd); @@ -476,12 +480,12 @@ class BoostMotor { const cmd = this._parent.generateOutputCommand( this._index, - BoostOutputExecution.EXECUTE_IMMEDIATELY ^ BoostOutputExecution.COMMAND_FEEDBACK, + (BoostOutputExecution.EXECUTE_IMMEDIATELY ^ BoostOutputExecution.COMMAND_FEEDBACK), BoostOutputSubCommand.START_SPEED_FOR_DEGREES, [ ...numberToInt32Array(degrees), this._power * this._direction * direction, - BoostMotorMaxPower, + MathUtil.wrapClamp(this._power + BoostMotorMaxPowerAdd, 0, 100), BoostMotorEndState.BRAKE, BoostMotorProfile.DO_NOT_USE ] @@ -590,7 +594,8 @@ class Boost { this._sensors = { tiltX: 0, tiltY: 0, - color: BoostColor.NONE + color: BoostColor.NONE, + previousColor: BoostColor.NONE }; /** @@ -598,7 +603,7 @@ class Boost { * @type {Array} * @private */ - this._colorBucket = []; + this._colorSamples = []; /** * The Bluetooth connection socket for reading/writing peripheral data. @@ -734,7 +739,7 @@ class Boost { dataPrefix: [0x97, 0x03, 0x00, 0x40], mask: [0xFF, 0xFF, 0, 0xFF] } - } */ + } commented out until feature is enabled in scratch-link */ }], optionalServices: [] }, this._onConnect, this.disconnect); @@ -760,7 +765,7 @@ class Boost { tiltX: 0, tiltY: 0, color: BoostColor.NONE, - oldColor: BoostColor.NONE + previousColor: BoostColor.NONE }; if (this._ble) { @@ -879,7 +884,7 @@ class Boost { /** * First three bytes are the common header: * 0: Length of message - * 1: Hub ID (always 0x00 at the moment) + * 1: Hub ID (always 0x00 at the moment, unused) * 2: Message Type * 3: Port ID * We base our switch-case on Message Type @@ -914,11 +919,11 @@ class Boost { this._sensors.tiltY = data[5]; break; case BoostIO.COLOR: - this._colorBucket.unshift(data[4]); - if (this._colorBucket.length > BoostColorSampleSize) { - this._colorBucket.pop(); - if (this._colorBucket.every((v, i, arr) => v === arr[0])) { - this._sensors.color = this._colorBucket[0]; + this._colorSamples.unshift(data[4]); + if (this._colorSamples.length > BoostColorSampleSize) { + this._colorSamples.pop(); + if (this._colorSamples.every((v, i, arr) => v === arr[0])) { + this._sensors.color = this._colorSamples[0]; } else { this._sensors.color = BoostColor.NONE; } @@ -953,7 +958,6 @@ class Boost { case BoostMessage.ERROR: log.warn(`Error reported by hub: ${data}`); break; - default: } } @@ -1926,10 +1930,10 @@ class Scratch3BoostBlocks { case BoostColorLabel.ANY: if (Object.keys(BoostColor).find(key => BoostColor[key]) .toLowerCase() !== this.getColor()) { - if (this.getColor() === this._peripheral.oldColor) { + if (this.getColor() === this._peripheral._sensors.previousColor) { return false; } - this._peripheral.oldColor = this.getColor(); + this._peripheral._sensors.previousColor = this.getColor(); return true; } break; From 2c6a9d85cf495dea63929f500f239769605e7e35 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Fri, 5 Apr 2019 13:05:52 -0400 Subject: [PATCH 2/9] Resolves #2087 and #2088 for rotation-based commands. There's quite a few interactions between degrees, their sign, and the currently set direction for the motor the degrees relate to. In this case, BoostMotor.turnOnDegrees() was being run with -degrees, and since that function does a Math.max between 0 and degrees, it resulted in 0 degrees. Because of this, and for clarity, turnOnDegrees now only gets called with positive values. If running CCW, that should be specified in the direction-parameter. --- src/extensions/scratch3_boost/index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index 665fc3fd4..b0d8e8ab5 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -1719,8 +1719,8 @@ class Scratch3BoostBlocks { if (motor.pendingTimeoutDelay) { motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); } else if (motor.pendingPositionDestination) { - const p = motor.pendingPositionDestination - motor.position; - motor.turnOnForDegrees(p, Math.sign(p) * motor.direction); + const p = Math.abs(motor.pendingPositionDestination - motor.position); + motor.turnOnForDegrees(p, Math.sign(p)); } } } @@ -1758,8 +1758,8 @@ class Scratch3BoostBlocks { if (motor.pendingTimeoutDelay) { motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); } else if (motor.pendingPositionDestination) { - const p = motor.pendingPositionDestination - motor.position; - motor.turnOnForDegrees(p, Math.sign(p) * motor.direction); + const p = Math.abs(motor.pendingPositionDestination - motor.position); + motor.turnOnForDegrees(p, Math.sign(p)); } } } From 2ee8042b6a404a17d82d065798fd9d69a0da8bf7 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Fri, 5 Apr 2019 13:35:28 -0400 Subject: [PATCH 3/9] Resolves #2085 This was caused because the case for BoostMessage.PORT_FEEDBACK didn't handle the BoostPortFeedback.DISCARDED type, which corresponds to a command failing on the Boost hub. --- src/extensions/scratch3_boost/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index ccd7f6d5f..51396482e 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -943,8 +943,9 @@ class Boost { const motor = this.motor(portID); if (motor) { motor._status = feedback; - if (feedback === (BoostPortFeedback.COMPLETED ^ BoostPortFeedback.IDLE) && - motor.pendingPromiseFunction) { + // Makes sure that commands resolve both when they actually complete and when they fail + const commandCompleted = feedback & (BoostPortFeedback.COMPLETED ^ BoostPortFeedback.DISCARDED); + if (commandCompleted && motor.pendingPromiseFunction) { motor.pendingPromiseFunction(); } } From e986b0f4cba03d3b75a8b29591be88b47409fa92 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Mon, 8 Apr 2019 13:48:10 -0400 Subject: [PATCH 4/9] - changed max power setting in motor functions to be calculated with MathUtil.clamp() instead of MathUtil.wrapClamp() to avoid values rolling over! - added an else-case to both setMotorDirection() and setMotorPower() so that dynamic speed/direction changes also affect the motorOn()-blocks just like the time- and rotation-based ones. --- src/extensions/scratch3_boost/index.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index b0d8e8ab5..cf17d7a81 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -447,7 +447,7 @@ class BoostMotor { BoostOutputSubCommand.START_SPEED, [ this._power * this._direction, - MathUtil.wrapClamp(this._power + BoostMotorMaxPowerAdd, 0, 100), + MathUtil.clamp(this._power + BoostMotorMaxPowerAdd, 0, 100), BoostMotorProfile.DO_NOT_USE ]); this._status = BoostPortFeedback.BUSY_OR_FULL; @@ -485,7 +485,7 @@ class BoostMotor { [ ...numberToInt32Array(degrees), this._power * this._direction * direction, - MathUtil.wrapClamp(this._power + BoostMotorMaxPowerAdd, 0, 100), + MathUtil.clamp(this._power + BoostMotorMaxPowerAdd, 0, 100), BoostMotorEndState.BRAKE, BoostMotorProfile.DO_NOT_USE ] @@ -1721,6 +1721,8 @@ class Scratch3BoostBlocks { } else if (motor.pendingPositionDestination) { const p = Math.abs(motor.pendingPositionDestination - motor.position); motor.turnOnForDegrees(p, Math.sign(p)); + } else { + motor.turnOn(); } } } @@ -1760,6 +1762,8 @@ class Scratch3BoostBlocks { } else if (motor.pendingPositionDestination) { const p = Math.abs(motor.pendingPositionDestination - motor.position); motor.turnOnForDegrees(p, Math.sign(p)); + } else { + motor.turnOn(); } } } From e24ace83a07497c8f312b2fc578174c6be684e3b Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Mon, 8 Apr 2019 17:48:30 -0400 Subject: [PATCH 5/9] noticed several instances of getting especially power and direction properties from private variables instead of using the getter --- src/extensions/scratch3_boost/index.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index cf17d7a81..b04e07827 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -446,8 +446,8 @@ class BoostMotor { BoostOutputExecution.EXECUTE_IMMEDIATELY, BoostOutputSubCommand.START_SPEED, [ - this._power * this._direction, - MathUtil.clamp(this._power + BoostMotorMaxPowerAdd, 0, 100), + this.power * this.direction, + MathUtil.clamp(this.power + BoostMotorMaxPowerAdd, 0, 100), BoostMotorProfile.DO_NOT_USE ]); this._status = BoostPortFeedback.BUSY_OR_FULL; @@ -462,7 +462,7 @@ class BoostMotor { * @param {number} milliseconds - run the motor for this long. */ turnOnFor (milliseconds) { - if (this._power === 0) return; + if (this.power === 0) return; milliseconds = Math.max(0, milliseconds); this.turnOn(); @@ -475,7 +475,7 @@ class BoostMotor { * @param {number} direction - rotate in this direction */ turnOnForDegrees (degrees, direction) { - if (this._power === 0) return; + if (this.power === 0) return; degrees = Math.max(0, degrees); const cmd = this._parent.generateOutputCommand( @@ -484,15 +484,15 @@ class BoostMotor { BoostOutputSubCommand.START_SPEED_FOR_DEGREES, [ ...numberToInt32Array(degrees), - this._power * this._direction * direction, - MathUtil.clamp(this._power + BoostMotorMaxPowerAdd, 0, 100), + this.power * this.direction * direction, + MathUtil.clamp(this.power + BoostMotorMaxPowerAdd, 0, 100), BoostMotorEndState.BRAKE, BoostMotorProfile.DO_NOT_USE ] ); - this._pendingPositionOrigin = this._position; - this._pendingPositionDestination = this._position + (degrees * this._direction * direction); + this._pendingPositionOrigin = this.position; + this._pendingPositionDestination = this.position + (degrees * this.direction * direction); this._parent.send(BoostBLE.characteristic, cmd); } @@ -501,7 +501,7 @@ class BoostMotor { * @param {boolean} [useLimiter=true] - if true, use the rate limiter */ turnOff (useLimiter = true) { - if (this._power === 0) return; + if (this.power === 0) return; const cmd = this._parent.generateOutputCommand( this._index, @@ -1794,7 +1794,7 @@ class Scratch3BoostBlocks { log.warn('Asked for a motor position that doesnt exist!'); return false; } - if (portID && this._peripheral._motors[portID]) { + if (portID && this.motor([portID])) { return MathUtil.wrapClamp(this._peripheral._motors[portID].position, 0, 360); } return 0; From ecbbacd4c099a4c018b50b510e34cbf53a07b9cd Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Mon, 8 Apr 2019 17:56:54 -0400 Subject: [PATCH 6/9] It seems like there's a tradeoff between how we choose to set the max power of the motors. Previously, I had set the motors' max power (torque) to follow their target speed, meaning they accelerated smoothly, but also lost their torque. Then in the following commit I changed the max power to 100 which means maximum torque to achieve the target speed, which resulted in very abrupt accelerations and erratic motor movement when changing speeds: https://github.com/LLK/scratch-vm/pull/2061/commits/873b56c9855a91b2d5e8797f9268406bcc8777c0 In the following commit I changed the functionality so that we add a fixed amount (10) more power than the target speed, e.g. for speed 50 it would provide power 60. This is fine for high speeds, but for low speeds it provides poor torque: https://github.com/LLK/scratch-vm/pull/2100/commits/e3cdbffa2a96da31017e24484fbd75d21a19e2ad I assume it would be possible to design some sort of calculation that enabled high torque for low speeds, and vice versa. I will discuss with the team. --- src/extensions/scratch3_boost/index.js | 16 ++++++++-------- src/util/math-util.js | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index b04e07827..d10535e70 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -365,16 +365,16 @@ class BoostMotor { } /** - * @param {int} value - this motor's new power level, in the range [0,100]. + * @param {int} value - this motor's new power level, in the range [10,100]. */ set power (value) { - const p = Math.max(0, Math.min(value, 100)); - // The Boost motors barely move at speed 1 - solution is to step up to 2. - if (p === 1) { - this._power = 2; - } else { - this._power = p; - } + /** + * Scale the motor power to a range between 10 and 100, + * to make sure the motors will run with something built onto them. + */ + const p = MathUtil.scale(value, 0, 100, 10, 100); + + this._power = p; } /** diff --git a/src/util/math-util.js b/src/util/math-util.js index c5b5704e8..123d7714d 100644 --- a/src/util/math-util.js +++ b/src/util/math-util.js @@ -77,6 +77,20 @@ class MathUtil { const sorted = elts.slice(0).sort((a, b) => a - b); return elts.map(e => sorted.indexOf(e)); } + + /** + * Scales a number from one range to another. + * @param {number} i number to be scaled + * @param {number} iMin input range minimum + * @param {number} iMax input range maximum + * @param {number} oMin output range minimum + * @param {number} oMax output range maximum + * @return {number} scaled number + */ + static scale (i, iMin, iMax, oMin, oMax) { + const p = (i - iMin) / (iMax - iMin); + return (p * (oMax - oMin)) + oMin; + } } module.exports = MathUtil; From c664fca9d7990fde372561b3f3f5900033988906 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Tue, 9 Apr 2019 08:47:00 -0400 Subject: [PATCH 7/9] changed getMotorPosition() to use the Boost.motor()-function instead of accessing the _motors-property directly --- src/extensions/scratch3_boost/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index d10535e70..debc9e860 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -1794,8 +1794,8 @@ class Scratch3BoostBlocks { log.warn('Asked for a motor position that doesnt exist!'); return false; } - if (portID && this.motor([portID])) { - return MathUtil.wrapClamp(this._peripheral._motors[portID].position, 0, 360); + if (portID && this.motor(portID)) { + return MathUtil.wrapClamp(this.motor(portID).position, 0, 360); } return 0; } From 7b917cabb47e804a7c6777a526048c13da4614f1 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Tue, 9 Apr 2019 09:30:26 -0400 Subject: [PATCH 8/9] Added isBusy-flag in onMessage to make sure promises aren't resolved if the motor is still busy. Added check in motorOnForRotation() that ensure that any previous pendingPromiseFunction() is resolved before creating a new one, to avoid hanging blocks --- src/extensions/scratch3_boost/index.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index fe841244f..5076386e4 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -949,8 +949,9 @@ class Boost { 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 (commandCompleted && motor.pendingPromiseFunction) { + if (!isBusy && commandCompleted && motor.pendingPromiseFunction) { motor.pendingPromiseFunction(); } } @@ -1646,6 +1647,10 @@ 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(); + } return new Promise(resolve => { motor.turnOnForDegrees(degrees, sign); motor.pendingPromiseFunction = resolve; From 3e55841011af1da19b4999e25a1b1551d7a70730 Mon Sep 17 00:00:00 2001 From: Kevin Andersen Date: Tue, 9 Apr 2019 11:45:42 -0400 Subject: [PATCH 9/9] Resolves #2087 and #2088. Because we weren't clearing a motor's _pendingPromiseFunction after executing it, it kept lingering, which made setMotorPower() and setMotorDirection() trigger a rotation-based command even if was responding to a timed or forever motorcommand. By clearing the property every time we fire the function, and by using pendingPromiseFunction as the conditional in setMotorPower() and setMotorDirection(), this should be taken care of. --- src/extensions/scratch3_boost/index.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/extensions/scratch3_boost/index.js b/src/extensions/scratch3_boost/index.js index 5076386e4..dfe0c8b4c 100644 --- a/src/extensions/scratch3_boost/index.js +++ b/src/extensions/scratch3_boost/index.js @@ -953,6 +953,7 @@ class Boost { const commandCompleted = feedback & (BoostPortFeedback.COMPLETED ^ BoostPortFeedback.DISCARDED); if (!isBusy && commandCompleted && motor.pendingPromiseFunction) { motor.pendingPromiseFunction(); + motor.pendingPromiseFunction = null; } } break; @@ -1650,6 +1651,7 @@ class Scratch3BoostBlocks { 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.turnOnForDegrees(degrees, sign); @@ -1724,7 +1726,7 @@ class Scratch3BoostBlocks { if (motor.isOn) { if (motor.pendingTimeoutDelay) { motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); - } else if (motor.pendingPositionDestination) { + } else if (motor.pendingPromiseFunction) { const p = Math.abs(motor.pendingPositionDestination - motor.position); motor.turnOnForDegrees(p, Math.sign(p)); } else { @@ -1765,7 +1767,7 @@ class Scratch3BoostBlocks { if (motor.isOn) { if (motor.pendingTimeoutDelay) { motor.turnOnFor(motor.pendingTimeoutStartTime + motor.pendingTimeoutDelay - Date.now()); - } else if (motor.pendingPositionDestination) { + } else if (motor.pendingPromiseFunction) { const p = Math.abs(motor.pendingPositionDestination - motor.position); motor.turnOnForDegrees(p, Math.sign(p)); } else {