firmataAccelStepper #179

dtex · 2017-08-27T04:34:17Z

Works with this PR in ConfigurableFirmata

Was #176 but that fork got deleted by... uhm... Ninjas or something.

coveralls · 2017-08-27T04:39:31Z

Coverage remained the same at 100.0% when pulling 750c07c on dtex:accelStepper into 0523f58 on firmata:master.

soundanalogous · 2017-08-27T17:41:11Z

examples/stepper-accel.js

+    board.accelStepperAcceleration(0, 100);
+    board.accelStepperStep(0, 2000);
+
+    board.on("stepper-done-0", function(position) {


Please use the callback example instead:

board.accelStepperStep(0, 2000, function(position) { console.log("Current position: " + position); });

Generally I don't think the typical user should ever have to know about the event "stepper-done-0".

soundanalogous · 2017-08-27T17:41:44Z

examples/stepper-multi.js

+      board.accelStepperReportPosition(1);
+    });
+
+    board.on("stepper-position-0", function(value) {


use the callback example instead

Same for "multi-stepper-done-0" above

And "stepper-position-1" below

soundanalogous · 2017-08-27T17:42:15Z

readme.md

  // Arduino is ready to communicate
 });
 ```

-#### With an Etherport object:


I think you had a bad rebase here. This section should not have been removed.

soundanalogous · 2017-08-27T17:45:43Z

lib/firmata.js

+ * @param {number} deviceNum Device number for the stepper (range 0-9)
+ */
+
+Board.prototype.accelStepperReportPosition = function(deviceNum) {


Please add an optional callback to this method so the user doesn't have to worry about listening for "stepper-position-N".

soundanalogous · 2017-08-27T17:49:21Z

lib/firmata.js

+ * @param {number} deviceNum Device number for the stepper (range 0-9)
+ */
+
+Board.prototype.accelStepperStop = function(deviceNum) {


Add an optional callback for this method since it reports the current position.

soundanalogous · 2017-08-27T17:49:58Z

examples/stepper-driver.js

+    board.accelStepperEnable(0, true);
+    board.accelStepperStep(0, 200);
+
+    board.on("stepper-done-0", function(position) {


Use callback example instead.

soundanalogous · 2017-08-27T17:51:28Z

examples/stepper-three-wire.js

+    board.accelStepperSpeed(0, 100);
+    board.accelStepperStep(0, 1000);
+
+    board.on("stepper-done-0", function(position) {


Use callback instead

coveralls · 2017-08-28T20:57:45Z

Coverage decreased (-0.2%) to 99.765% when pulling 3dd52b7 on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-08-30T22:16:40Z

Coverage decreased (-0.2%) to 99.765% when pulling 7328521 on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-08-30T22:16:40Z

Coverage decreased (-0.2%) to 99.765% when pulling 7328521 on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-08-30T22:39:11Z

Coverage decreased (-0.2%) to 99.765% when pulling d8dec42 on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-08-31T00:30:43Z

Coverage remained the same at 100.0% when pulling b487e77 on dtex:accelStepper into 0523f58 on firmata:master.

soundanalogous · 2017-09-04T16:33:28Z

lib/firmata.js

+
+  this.transport.write(new Buffer(data));
+
+  if (callback) {


Please remove this callback. I was wrong in requesting it be added to accelStepperStop. This results in the callback being called twice (once because it's registered when a step sequence starts and a second time when it stops).

I can't tell if this was addressed?

This was resolved. The request was to remove the callback from accelStepperStop (which has been removed). It looks like I had added this comment to the wrong method.

Cool, thanks for clarifying

soundanalogous · 2017-09-04T17:52:42Z

lib/firmata.js

+  this.transport.write(new Buffer(data));
+
+  if (callback) {
+    this.once("multi-stepper-done-" + groupNum, callback);


@dtex If the user calls multiStepperStop, this callback will never be handled. I'm not sure of the details of Node's EventEmitter once method and if it cleans up unhandled events (if the next call to once would deallocate memory from the previous unhandled event) or if we risk a memory leak here. One potential work around is to call reportGroupComplete in the firmware when a multiStepperStop message is received (see comment I left in ConfigurableFirmata).

Curious if @rwaldron has any insight into this as well since it's probably come up when implementing J5 modules.

Added call to reportGroupComplete in the firmware.

coveralls · 2017-10-20T17:09:47Z

Coverage remained the same at 100.0% when pulling fad02ab on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-10-20T18:01:48Z

Coverage remained the same at 100.0% when pulling e850d4f on dtex:accelStepper into 0523f58 on firmata:master.

dtex · 2017-10-20T18:12:49Z

Sorry, I forgot about this PR. I've updated the tests to accommodate the last change, so we should be good to go now.

soundanalogous · 2017-10-21T04:44:20Z

lib/firmata.js

@@ -919,7 +948,7 @@ Board.prototype.pinMode = function(pin, mode) {
 /**
 * Asks the arduino to write a value to a digital pin
 * @param {number} pin The pin you want to write a value to.
- * @param {number} value The value you want to write. Must be board.HIGH or board.LOW
+ * @param {value} value The value you want to write. Must be board.HIGH or board.LOW


Should still be {number}

soundanalogous · 2017-10-21T04:47:53Z

lib/firmata.js

@@ -1148,13 +1177,13 @@ Board.prototype.sendI2CWriteRequest = function(slaveAddress, bytes) {
 * Write data to a register
 *
 * @param {number} address      The address of the I2C device.
- * @param {Array} cmdRegOrData  An array of bytes
+ * @param {array} cmdRegOrData  An array of bytes


I thought there was some weird bug (maybe in jshint or jscs?) that required {Array} instead of {array}. Perhaps it's been fixed though. If you're not seeing any lint errors then I'm sure {array} is fine.

Neither of these changes were intentional. Let me hop back into my commit and make sure I'm not submitting anything else I didn't mean to.

coveralls · 2017-10-21T14:18:20Z

Coverage remained the same at 100.0% when pulling f4c6f7c on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-10-21T14:18:20Z

Coverage remained the same at 100.0% when pulling f4c6f7c on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-10-21T14:18:20Z

Coverage remained the same at 100.0% when pulling f4c6f7c on dtex:accelStepper into 0523f58 on firmata:master.

dtex · 2017-10-21T15:21:13Z

Ya, I improperly rebased at some point. I'll get it cleaned up later today.

coveralls · 2017-10-21T15:30:39Z

Coverage remained the same at 100.0% when pulling fe99622 on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-10-21T15:39:24Z

Coverage remained the same at 100.0% when pulling 0a52738 on dtex:accelStepper into 0523f58 on firmata:master.

First commit

coveralls · 2017-10-21T15:43:53Z

Coverage remained the same at 100.0% when pulling 848b289 on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-10-21T15:49:29Z

Coverage remained the same at 100.0% when pulling 79d8059 on dtex:accelStepper into 0523f58 on firmata:master.

coveralls · 2017-10-21T15:53:28Z

Coverage remained the same at 100.0% when pulling 79d8059 on dtex:accelStepper into 0523f58 on firmata:master.

dtex · 2017-10-21T15:57:54Z

It is cleaned up now. The array->Array changes came from @rumata28 so they might have more info about that jsdoc/jshint bug you mention.

soundanalogous · 2017-10-21T19:12:09Z

Sounds like the lower case 'a' in {array} causes a syntax highlighting error in Webstorm: #174

soundanalogous · 2017-10-24T05:06:36Z

I think this is ready to merge. @rwaldron sound good to you?

rwaldron · 2018-01-04T16:30:19Z

lib/firmata.js

+
+Board.prototype.accelStepperConfig = function(opts) {
+
+  var interface, pinsToInvert = 0x00;


interface is a reserved keyword in strict mode code. Ideally we want this code to be strict mode safe and es module safe (which is implicit strict mode). Since I've taken so long to review, I will make the fixes for you

rwaldron · 2018-01-04T16:36:32Z

lib/firmata.js

      FOUR_WIRE: 4,
    },
+    STEPTYPE: {


This name might be confusing with the unrelated TYPE property? Can we call it STEP_SIZE?

rwaldron · 2018-01-05T15:41:55Z

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

Landed!

soundanalogous · 2018-01-05T16:02:34Z

Yay!!!

rwaldron · 2018-01-05T16:55:04Z

Sorry for taking so damn long to review this, everyone's patience is truly appreciated <3

rwaldron · 2018-01-05T16:55:32Z

I've asked @dtex if we can wait until Monday to release

dtex mentioned this pull request Aug 27, 2017

accelStepper #176

Closed

soundanalogous reviewed Aug 27, 2017

View reviewed changes

dtex force-pushed the accelStepper branch from 750c07c to 3dd52b7 Compare August 28, 2017 20:14

dtex force-pushed the accelStepper branch 2 times, most recently from 7328521 to d8dec42 Compare August 30, 2017 20:41

dtex force-pushed the accelStepper branch from d8dec42 to b487e77 Compare August 30, 2017 22:39

soundanalogous reviewed Sep 4, 2017

View reviewed changes

dtex force-pushed the accelStepper branch from b487e77 to 422705a Compare September 9, 2017 20:49

dtex mentioned this pull request Oct 10, 2017

Stepper motor support nebrius/raspi-io#70

Closed

dtex force-pushed the accelStepper branch 2 times, most recently from fad02ab to e850d4f Compare October 20, 2017 15:38

soundanalogous reviewed Oct 21, 2017

View reviewed changes

dtex force-pushed the accelStepper branch from e850d4f to f4c6f7c Compare October 21, 2017 14:11

dtex force-pushed the accelStepper branch from f4c6f7c to fe99622 Compare October 21, 2017 15:24

dtex force-pushed the accelStepper branch 2 times, most recently from 0a52738 to 848b289 Compare October 21, 2017 15:36

dtex force-pushed the accelStepper branch from 848b289 to 9aa3fef Compare October 21, 2017 15:41

firmataAccelStepper

79d8059

First commit

dtex force-pushed the accelStepper branch from 9aa3fef to 79d8059 Compare October 21, 2017 15:42

dtex mentioned this pull request Oct 26, 2017

Stepper: Runstate / Position tracking rwaldron/johnny-five#1407

Closed

rwaldron reviewed Jan 4, 2018

View reviewed changes

rwaldron closed this Jan 5, 2018

dtex mentioned this pull request Mar 18, 2019

Stepper motors and Johnny five dtex/tharp#3

Open


		Board.prototype.accelStepperConfig = function(opts) {

		var interface, pinsToInvert = 0x00;

firmataAccelStepper #179

firmataAccelStepper #179

Conversation

dtex commented Aug 27, 2017

coveralls commented Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 28, 2017 • edited Loading

coveralls commented Aug 30, 2017 • edited Loading

coveralls commented Aug 30, 2017

coveralls commented Aug 30, 2017 • edited Loading

coveralls commented Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 20, 2017 • edited Loading

coveralls commented Oct 20, 2017 • edited Loading

dtex commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 21, 2017 • edited Loading

coveralls commented Oct 21, 2017

coveralls commented Oct 21, 2017

dtex commented Oct 21, 2017

coveralls commented Oct 21, 2017 • edited Loading

coveralls commented Oct 21, 2017 • edited Loading

coveralls commented Oct 21, 2017 • edited Loading

coveralls commented Oct 21, 2017 • edited Loading

coveralls commented Oct 21, 2017 • edited Loading

dtex commented Oct 21, 2017

soundanalogous commented Oct 21, 2017

soundanalogous commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaldron commented Jan 5, 2018

soundanalogous commented Jan 5, 2018

rwaldron commented Jan 5, 2018

rwaldron commented Jan 5, 2018

coveralls commented Aug 27, 2017 •

edited

Loading

coveralls commented Aug 28, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Aug 31, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading

coveralls commented Oct 21, 2017 •

edited

Loading