[WIP] accelStepper for configurable firmata #72

dtex · 2017-07-03T18:11:16Z

I have lots of testing to do here so don't merge. Need to track down lots of different steppers and drivers to test with hardware. Which micro controllers do you recommend testing on?

No hablo C++ so I am wide open to comments on code and style.

@soundanalogous You mentioned that you want to just call this stepper instead of stepper2. It seems possible that you could get higher step speed from the original stepper code because there is less overhead. I need to test and prove that true, but maybe there is an argument to be made for having both. If so, maybe we could call this accelStepperFirmata instead of stepper2?

accelStepper & multiStepper are unmodified from the original so should I remove them from the PR and just instruct user to install?

soundanalogous · 2017-07-03T18:56:24Z

examples/ConfigurableFirmata/ConfigurableFirmata.ino

+#include <StepperFirmata2.h>
+StepperFirmata2 stepper;
+
+#include <StepperFirmata2.h>


Why is this included a second time?

Because I wanted to make damn sure it got included...

or I messed up a rebase somewhere...

Ya, it's probably that second one.

soundanalogous · 2017-07-09T03:26:48Z

src/StepperFirmata2.cpp

+        }
+
+        if (wireCount == 0) {
+          stepper[deviceNum] = new AccelStepper(AccelStepper::DRIVER, directionPin, stepPin);


The direction and step parameters are reversed here according to the AccelStepper docs. I also tested in a Arduino sketch to verify.

Rather than just swap the parameters here I made the order consistent in protocol as well. It bothered me having a signature that looked like

stepper[deviceNum] = new AccelStepper(AccelStepper::DRIVER, directionOrMotorPin2, stepOrMotorPin1);

soundanalogous · 2017-07-09T03:36:06Z

src/StepperFirmata2.cpp

+
+      }
+
+      else if (stepCommand == STEPPER_SET_SPEED) {


This is misleading since the AccelStepper API defines both max speed and speed. This should be renamed to STEPPER_SET_MAX_SPEED. Or is this supposed to call setSpeed() instead of setMaxSpeed()?

I wanted to keep the API generic enough that it could be used with other stepper libraries.

I didn't understand the hack you had set up, but now I think it's fine to leave this as is. However add a comment in here that setMaxSpeed is being used to perform the same task as setSpeed when the MAX_ACCELERATION is set.

Done. Added the comments and the constant

soundanalogous · 2017-07-09T04:30:39Z

Okay so it turns out that supporting AccelStepper with and without acceleration in Firmata is going is a bit tricky. I've done some experimenting with the AccelStepper library directly and have a better sense for how it needs to be approached.

In order to step without acceleration, you need to call runSpeedToPosition() rather than run(). This works with both move() and moveTo(). However in order for it to work (and this is the tricky part) move() or moveTo() must be called before calling setSpeed(). And then we need to figure out how to know when to call run() or runSpeedToPosition() for a given active stepper in the update() function.

This also means we'd have to support setSpeed (used for stepping without acceleration) and setMaxSpeed separately. setMaxSpeed is used in both cases, but from what I understand is more critical in calculating the acceleration.

Here are examples of driving with a motor and without acceleration:

// with acceleration (basically what you have working currently)
#include <AccelStepper.h>

// params: interface, step, direction
AccelStepper stepper(AccelStepper::DRIVER, 3, 2);

void setup() {  
   stepper.setMaxSpeed(3000);
   stepper.setAcceleration(500);
   stepper.move(2000);
}

void loop() {  
   stepper.run();
}

// without acceleration (what we need to figure out how to support)
#include <AccelStepper.h>

// params: step, direction
AccelStepper stepper(AccelStepper::DRIVER, 3, 2);

void setup() {  
   stepper.setMaxSpeed(3000);

   // move or moveTo must be called before setSpeed (no fun) 
   stepper.move(10*8); // take 10 steps with an EasyDriver
   // stepper.moveTo(2000); // this is also supported
   stepper.setSpeed(50); // need to add this to the protocol
}

void loop() {  
   stepper.runSpeedToPosition();
}

soundanalogous · 2017-07-09T06:51:12Z

src/StepperFirmata2.cpp

+
+        if (stepper[deviceNum]) {
+          if (decodedAcceleration == 0.0) {
+            stepper[deviceNum]->setAcceleration(99999);


I had run through the run vs runSpeed options and even had an implementation where it chose which to call based on the current acceleration value for that particular stepper. I was having to track the acceleration value in firmata since it cannot be read from the accelStepper instance. It just seemed simpler to set acceleration to a high enough value that acceleration would be complete on the first step. That's why 99999.

I see, you've come up with a clever hack here! I ran some tests to get a sense for how this affects accuracy vs using setSpeed directly. It's not too bad. Just off by a few milliseconds. Here's what I did (and then run with USE_HACK true, then false and compare output):

#include <AccelStepper.h> #define MAX_ACCELERATION 99999 #define USE_HACK true // params: step, direction AccelStepper stepper(AccelStepper::DRIVER, 3, 2); long numSteps = 1000 * 8; // 1000 steps long startTime; bool runOnce = true; void setup() { Serial.begin(9600); if (USE_HACK) { stepper.setMaxSpeed(500); stepper.move(numSteps); stepper.setAcceleration(MAX_ACCELERATION); } else { stepper.setMaxSpeed(3000); stepper.move(numSteps); stepper.setSpeed(500); } startTime = millis(); } void loop() { if (USE_HACK) { stepper.run(); } else { stepper.runSpeedToPosition(); } if (runOnce && stepper.currentPosition() == numSteps) { Serial.println(millis() - startTime); runOnce = false; } }

I found that using a lower number than 99999 improves accuracy, but I'm not sure if that is always the case so 99999 is probably safe, but please add a constant MAX_ACCELERATION to define it. Also add a comment about how this approach is a hack.

Cool test!

I don't understand how the lower values for MAX_ACCELERATION could be more accurate. At 500 steps/sec and 99999 MAX_ACCELERATION, we won't reach 500 steps / second until something like .05 seconds in. I think this is where our few milliseconds discrepancy is coming from. Lower MAX_ACCELERATION values should take even longer to reach full speed and be even further off.

The MAX_ACCELERATION value should be 1000^2 to ensure that we reach the max accel value by the first step. I've only achieved about 450 steps/sec with Stepper 2, but accelStepper says 1000 steps/sec are achievable so for future proofing: 1000000.

I don't have an Arduino with me, but I will run some tests tonight.

I think there's something about the rate of acceleration balancing with the step speed. For example if I set the max speed to 50, then an acceleration value of 2000 will result in the same time-to-complete as using setSpeed with runSpeedToPosition. There may be an equation in there somewhere or it could be just a random chance.

soundanalogous · 2017-07-09T18:20:59Z

src/StepperFirmata2.cpp

+
+    if (deviceNum < MAX_STEPPERS) {
+
+      if (stepCommand == STEPPER_CONFIG) {


I think somewhere in STEPPER_CONFIG it may be helpful to call stepper[deviceNum]->setAcceleration(MAX_ACCELERATION)` to set stepping without acceleration as the default case. That way the user doesn't have to call setAcceleration(0.0) in the client (if that's even how other client library others - non JavaScript - choose to implement the API).

Unless of course you feel that most users would prefer acceleration by default - I'm open to debate on that as I don't have a strong opinion either way.

I guess for a first time user, hearing acceleration is probably more satisfying than a standard step sequence without acceleration, but in either case it may be helpful to have a default.

lets just leave this as is and then decide how to document in the client library

AccelStepper does set an initial default value of 1 for setAcceleration at the end of the constructor.

A runaway stepper in an already assembled robot (or whatever) could be disastrous so going slow by default (1 step per second) makes sense.

On the other hand the default acceleration value is so low that users won't be able to perceive it and acceleration should probably just be off.

soundanalogous · 2017-07-09T21:13:58Z

accelStepper & multiStepper are unmodified from the original so should I remove them from the PR and just instruct user to install?

I think we should keep it bundled as that library tends to get updated with some frequency and there's a chance an update could always break something here. I have AccelStepper installed in my Arduino/libraries directory and the compiler is using the bundled version which is good. There's a chance users of older Arduino IDEs (pre 1.6) may have issues if they also have installed AccelStepper separately but I haven't looked into that yet.

soundanalogous · 2017-07-09T21:25:08Z

src/StepperFirmata2.cpp

+          stepper[deviceNum] = new AccelStepper(AccelStepper::FULL2WIRE, directionPin, stepPin);
+        } else if (wireCount == 3 && stepType == 0) {
+          stepper[deviceNum] = new AccelStepper(AccelStepper::FULL3WIRE, directionPin, stepPin, motorPin3);
+        } else if (wireCount == 3 && stepType == 2) {


should be stepType == 1. Also on line 149. I recommend adding constants for all of the step types.

actually I think this is correct since you are not currently shifting the stepType value. However the lack of a constant makes this confusing.

After seeing the easyDriver's step types and how they are controlled I want to allow more flexibility for micro stepping types for the future. So instead of just this in protocol:

4th - 6th bits = step type XXX000X = whole step XXX001X = half step XXX010X = micro step)

We would have:

4th - 6th bits = step type XXXnnnX = where step size = 1/2^nnn

That way we can send any step size from this set in the same 3 bits (as a bonus, the existing values don't change):
[WHOLE, HALF, QUARTER, EIGHTH, SIXTEENTH, 1/32, 1/64, 1/128]

I could change motorPin3 to motorPin3OrMS1 and motorPin4 to motorPin4OrMS2 and we could manage the step size on easyDrivers too.

Whether or not to use motor pins 3 and 4 depends on how common it is among stepper drivers to use 2 pins to manage the number of microsteps. I'm leaning more toward just managing that using regular old digital pin messages. It can be added to the stepper API in a Firmata client as a convenience to the user (that way it could even be wrapped up in the Stepper2 constructor).

It's also easy enough for a user to configure this purely in hardware using jumpers.

Honestly, the only board I know of that let's you control step size with pins is the easyDriver. Anything smaller than half stepping is achieved using PWM on the motorPins for non-driver boards.

It's also easy enough for a user to configure this purely in hardware using jumpers

That's where my mind was until about twenty minutes ago. I've kinda fallen in love with the easyDriver and want to support it better.

soundanalogous · 2017-07-09T22:40:55Z

src/StepperFirmata2.cpp

+
+        if (wireCount == 0) {
+          stepper[deviceNum] = new AccelStepper(AccelStepper::DRIVER, directionPin, stepPin);
+        } else if (wireCount == 2 && stepType == 0) {


Could change remove stepType == 0 here since stepType when using 2 wires. Otherwise document either in the protocol or in the client library that 2 wire configurations must set the stepType to WHOLE.

soundanalogous · 2017-07-09T22:44:04Z

src/StepperFirmata2.cpp

+        } else if (wireCount == 4 && stepType == 0) {
+          stepper[deviceNum] = new AccelStepper(AccelStepper::FULL4WIRE, directionPin, stepPin, motorPin3, motorPin4, false);
+        } else if (wireCount == 4 && stepType == 2) {
+          stepper[deviceNum] = new AccelStepper(AccelStepper::HALF4WIRE, directionPin, stepPin, motorPin3, motorPin4);


This may also need the last param set to false?

soundanalogous · 2017-07-13T05:38:59Z

src/StepperFirmata2.cpp

@@ -87,7 +87,7 @@ boolean StepperFirmata2::handleSysex(byte command, byte argc, byte *argv)
  if (command == ACCELSTEPPER_DATA) {

    byte stepCommand, deviceNum, interface, wireCount, stepType;
-    byte stepOrMotorPin1, directionOrMotorPin2, motorPin3, motorPin4, enablePin, invertPins = 0;
+    byte stepOrMotorPin1, directionOrMotorPin2, ms1OrMotorPin3, ms2OrMotorPin4, enablePin, invertPins = 0;


I'm not convinced that I want to include support for the MS1 and MS2 pins in this way. It's too specific to the EasyDriver. If you can find other stepper driver boards that use the same 2 pin convention, I could probably be convinced. Otherwise I'd prefer using standard DIGITAL_MESSAGE commands to set the 2 pin values from firmata.js. That could also be set in the Stepper2 constructor in firmata.js, just using DIGITAL_MESSAGE instead of Stepper2.config.

All the driver style boards I can find are derivatives of the easyDriver, but I did some digging and found that there are even versions of the easyDriver (v3 and earlier) that do not have the MS pins. Also, the config section became one of those things that would be infuriatingly obfuscated for future maintainers.

It was neat to see it working, but I think I'm on board with moving the functionality out of here and into either firmata.js or johnny-five. Since J5 is purpose built to normalize controller and device interfaces I think that's a good place for it and we wouldn't need to add any whacky logic to firmata.js.

Also, Big Easy Driver has an MS3 pin so that just adds another variation. I'm further convinced that J5 and other dependents of firmata.js are the right place for this.

soundanalogous · 2017-07-15T17:11:47Z

It seems possible that you could get higher step speed from the original stepper code because there is less overhead.

I kinda doubt there is much difference between StepperFirmata and AccelStepper in terms of speed since both libraries are based off of the same white paper and implement the same algorithms, but worth looking into I guess. The thing that may make AccelStepper slower is the use of floats, but I'm not sure how big of an impact that is. A fair comparison would be to determine the max speed achievable for a given motor using both Stepper and Stepper2 in whole step 4-wire configuration and also both in DRIVER mode.

dtex · 2017-07-15T20:01:16Z

I'll do some testing to see what the max speeds with each are. I discovered during my firmata.js testing that my custom floats were not being encoded the way I expected which means the decoder in here might be wrong.

soundanalogous · 2017-07-15T20:09:06Z

I think it would be helpful to add unit tests for the encoder/decoder functions. Add a stepper_test directory to ConfigurableFirmata/test/. I'm not looking for full coverage, just the encoder and decoder functions. See this test file from FirmataEncoder for an example of tests written for a specific feature. At some point I plan to get around to adding tests for all of the features.

Except your test file would be pretty simple since you don't even need a ConfigurableFirmata or FakeStream instance. You should just be able to call those functions on Stepper2 with a few parameter sets and evaluate the results.

soundanalogous · 2017-07-15T23:30:36Z

src/StepperFirmata2.cpp

+    byte stepCommand, deviceNum, interface, wireCount, stepType;
+    byte stepOrMotorPin1, directionOrMotorPin2, motorPin3, motorPin4, enablePin, invertPins = 0;
+    long numSteps;
+    int maxSpeed, stepSpeed, accel, decel;


none of these 4 ints are currently used

soundanalogous · 2017-07-15T23:37:17Z

src/StepperFirmata2.cpp

+
+long StepperFirmata2::decode32BitSignedInteger(byte arg1, byte arg2, byte arg3, byte arg4, byte arg5)
+{
+  long result = (long)arg1 | (long)arg2 << 7 | (long)arg3 << 14 | (long)arg4 << 21 | ((long)arg5 << 28) & 0x07;


Add a set of parens around the last byte: (((long)arg5 << 28) & 0x07). That will make a complier warning go away.

soundanalogous · 2017-07-19T03:37:27Z

src/StepperFirmata2.h

+#include "FirmataFeature.h"
+
+#define MAX_STEPPERS 10 // arbitrary value... may need to adjust
+#define MAX_GROUPS 5 // arbitrary value... may need to adjust


defines are global across all files and therefore should be named somewhat specifically. Please rename to MAX_STEPPER_GROUPS.

soundanalogous · 2017-07-19T03:38:30Z

src/StepperFirmata2.h

+
+#define MAX_STEPPERS 10 // arbitrary value... may need to adjust
+#define MAX_GROUPS 5 // arbitrary value... may need to adjust
+#define MAX_ACCELERATION 1000000 // 1000^2 so that full speed is reached on first step


change to STEPPER_MAX_ACCELERATION

soundanalogous · 2017-07-19T03:40:45Z

Lets rename StepperFirmata2 to AccelStepperFirmata as you had previously recommended. I'll add some inline comments regarding this change.

soundanalogous · 2017-07-19T03:41:41Z

examples/ConfigurableFirmata/ConfigurableFirmata.ino

@@ -178,6 +178,9 @@ OneWireFirmata oneWire;
 #include <StepperFirmata.h>
 StepperFirmata stepper;

+#include <StepperFirmata2.h>
+StepperFirmata2 stepper2;


After renaming class to AccelStepperFirmata, change the instance name to accelStepper.

soundanalogous · 2017-07-19T03:42:13Z

src/StepperFirmata2.h

+  See file LICENSE.txt for further informations on licensing terms.
+*/
+
+#ifndef StepperFirmata2_h


Change to AccelStepperFirmata_h

soundanalogous · 2017-07-19T03:42:49Z

src/StepperFirmata2.h

+#define MULTISTEPPER_STOP 0x23
+#define MULTISTEPPER_MOVE_COMPLETE 0x24
+
+class StepperFirmata2: public FirmataFeature


change class name to AccelStepperFirmata

soundanalogous · 2017-07-20T03:05:52Z

test/stepper_test/stepper_test.ino

+ */
+
+#include <ArduinoUnit.h>
+#include <StepperFirmata2.h>


Please update this file as well.

soundanalogous · 2017-07-20T03:06:19Z

src/AccelStepperFirmata.h

+  See file LICENSE.txt for further informations on licensing terms.
+*/
+
+#ifndef StepperFirmata2_h


Please change to AccelStepperFirmata2_h for consistency.

Done. Figured I missed at least one.

dtex · 2017-07-25T19:35:16Z

@soundanalogous I can compile for zero no problem as long as I comment out the servo includes. Servo is throwing a ton of errors. Is that expected?

soundanalogous · 2017-08-05T22:05:55Z

src/AccelStepperFirmata.cpp

+}
+
+/*==============================================================================
+ * Helpers


@zfields please take a look at the following 3 functions. This is a scheme that @dtex and I devised for encoding and decoding floats (documented here) and 32 bit signed integers to transmit and receive via Firmata. Currently this is only implemented (and necessary) here in this AccelStepper feature, but I think it could be useful as a general utility. However I know at one point you had voiced your opinion about making such a thing widely available. One option is to keep these functions in the AccelStepperFirmata class for now and then decide later if they have wider appeal.

@soundanalogous @dtex The level of compression is great! That being said, I have a couple of notes/questions...

I don't see encodeCustomFloat, why is that function not necessary?

The float encoding strategy works great since you have defined the memory layout, but I can see the possibility of difference in endianess between the host and client trashing your integer. Have you considered and tested this scenario?

Finally, I think an abstraction between encoding/decoding and content (i.e. data types) should exist. As you have observed above, it takes 5 bytes to encode a 4-byte field. However, this is true of any contiguous 4-byte block of data. Furthermore, a slightly more generalized compression strategy can be applied to "n" contiguous bytes regardless of data type. While your compression level is spot on, it would be my vote to leave this in the AccelStepperFirmata class until a more generalized solution is realized.

@zfields Thank you for taking a look.

I don't see encodeCustomFloat, why is that function not necessary?

We only return integers to the client. The float values that are received are used but not changed by accelStepper so there was no need to ever pass them back. I had decodeCustomFloat in there but realized it wasn't being used so I removed it.

possibility of difference in endianess... Have you considered and tested this scenario?

No, I hadn't. Do you know of any platforms where this is the case? Also, is endianness only an issue with longs or could that also be an issue with ints for example?

a slightly more generalized compression strategy can be applied to "n" contiguous bytes

Would that be the decoding equivelant of encodeByteStream in firmataMarshaller?

Oh, and I have no objections to leaving this in firmataAccelStepper. If a more generalized solution lands at a later date, I'm happy to update this.

Also, is endianness only an issue with longs or could that also be an issue with ints for example?

It affects both, however I would be hesitant to "fix" this until it presents itself. We assume the Arduino is big endian at the byte level and in the protocol we send bytes little endian.

If you look at the protocol, it shows the bytes going out little endian. However, if you look to the implementation you can see we assume bytes are ordered "76543210", because send bytes as (byte & 0x7F) and ((byte >> 7) & 0x01).

Would that be the decoding equivelant of encodeByteStream in firmataMarshaller?

There isn't one, unless you can tell how long the data is; I only realized it after I wrote it. The data length can be implicit (as in data types) or explicit (as in a length parameter). In Firmata 3.0 we will likely provide support for this style of encoding in the protocol one way or another.

dtex · 2017-08-05T22:21:54Z

I've removed that line & thank you for testing on the Intel platforms. I hope I'm never in a situation where my survival depends on compiling for any of those (It's not them, it's me).

It's my turn to be on vacation. We are traveling to the Bay Area and I won't be on line regularly until Thursday.

soundanalogous · 2017-08-05T22:23:44Z

If you're going to be in San Francisco and have a few minutes to spare I'd love to buy you a coffee or a beer :)

dtex · 2017-08-05T22:33:50Z

Hey, I'd like that. I'll DM you on Twitter.

zfields · 2017-08-12T23:50:59Z

Hey guys! I've been workin' like a mad fool, and I just saw your message. I'm looking it over now.

dtex · 2017-08-20T14:38:46Z

Just wanted to make sure there weren't any to-do's in my court on this.

zfields · 2017-08-20T15:42:28Z

I haven't had an opportunity to test it, but it looks like a good addition. I would leave the float functionality where it sits for now, and use it as a reference for Firmata 3.0. I'll leave it to @soundanalogous to finalize.

soundanalogous · 2017-08-20T18:07:13Z

I'll try to take a final look today. One reason I've been delaying a bit on this is that it's the first time we'll have 2 different approaches to a single feature in Firmata. There is currently no way for a client to know which version of Stepper (legacy or new AccelStepper version) the firmware supports. In the past, the presence of Stepper pins in the capability query would have been the signal that the firmware supported driving stepper motors. That will no longer be the case going forward since there will be 2 stepper implementations. For this reason, I was hoping to release some initial version of a feature / firmware query at the same time, but I'm not sure yet if it's better to wait until Firmata 3.0 to introduce such a feature. If that is the case the user will just have to know which Stepper firmware is included until Firmata 3.0 comes out.

dtex · 2017-08-20T19:18:31Z

It helps that this feature is only in configurableFirmata as it will require some degree of thoughtfulness on the part of the end user. Do you have a process for notifying the developers of client libraries of changes like this? I imagine the vast majority of end users will be going through one of those.

Heck, it might be worthwhile to see which client libraries actually support the original stepper library and just hassle those maintainers.

soundanalogous · 2017-08-20T20:30:54Z

It probably won't have a large impact immediately. There are a lot of stale Firmata client libraries. To my knowledge very few added Stepper support and some even added their own version of Stepper. I can ping the maintainers of the most active ones that I'm aware of.

soundanalogous · 2017-08-21T05:58:33Z

Ended up having to work most of the day. I promise I'll get to this next weekend when I'm back to a normal schedule.

soundanalogous · 2017-08-27T19:43:37Z

src/AccelStepperFirmata.cpp

+    // if one or more stepper motors are used, update their position
+    for (byte i = 0; i < MAX_STEPPERS; i++) {
+      if (stepper[i] && isRunning[i] == true) {
+        stepsLeft = stepper[i]->run();


If the user calls stop, stepsLeft will be false even if the step sequence is not complete because the run() function returns false if _speed == 0 which is the case if stop is called. This results in the done event being triggered when the STEPPER_STOP message is sent.

One way to get around this is to change the condition below to the following:

if (!stepper[i]->isRunning()) { ... }

the isRunning() method checks if the speed == 0 AND the step sequence is complete.

A change like this would require rethinking the callback strategy in firmata.js since you could be left with un unhandled event in the case that stop is called and the event is listening for 'done' and the sequence is never resumed.

This however depends on whether or not a user should be able to resume a step sequence after sending a STEPPER_STOP message.

After further review, I've determined no change is necessary here. I was unaware initially that the stop() behavior was terminal.

soundanalogous · 2017-08-27T19:50:27Z

src/AccelStepperFirmata.cpp

+        if (stepper[deviceNum]) {
+          stepper[deviceNum]->stop();
+          isRunning[deviceNum] = false;
+          reportPosition(deviceNum, true);


Should the 2nd param here be true or false. Currently this will send a MOVE_COMPLETE message. However is it also assumed a user can resume a stopped stepper? If so then the 2nd param should be false and this would then send a REPORT_POSITION message.

Ok, I read up on AccelStepper.stop(). It is actually terminal (the user cannot resume a step sequence after calling stop). Therefore, we can simply remove reportPosition(deviceNum, true); here since the STEPPER_COMPLETE message will be sent from the update() method after a STEPPER_STOP message is received. In that case SETPPER_COMPLETE means the sequence has either completed or was stopped (terminated at the current step count).

Hummm, I noticed something else interesting here in experimenting further. Do we want stop() to stop immediately or do we want stop() to ramp down. The behavior in AccelStepper according to the documentation is actually to ramp down as quickly as possible using the current speed and acceleration parameters. If you comment out isRunning[deviceNum] = false and reportPosition(deviceNum, true), you'll see that behavior if you run something like this in JS:

board.accelStepperSpeed(0, 400); board.accelStepperAcceleration(0, 100); board.accelStepperTo(0, 2000, function(position) { console.log("Current position: " + position); }); setTimeout(function() { // we need to decide if we want this to stop immediately or if we want it to ramp // down if acceleration was enabled board.accelStepperStop(0); }, 2000);

Here's an example from the AccelStepper documentation: http://www.airspayce.com/mikem/arduino/AccelStepper/Quickstop_8pde-example.html. There they allow the motor to decelerate quickly after the stop() function.

I don't want to presume I know better than Mike, but I would think stop() should be immediate (for example, when a limit switch is hit). I know enable(false) would give us that immediate behavior, but not everyone will have their enable pins wired up.

soundanalogous · 2017-08-27T19:53:40Z

src/AccelStepperFirmata.h

+#define MAX_ACCELERATION 1000000 // 1000^2 so that full speed is reached on first step
+#define STEP_TYPE_WHOLE 0x00
+#define STEP_TYPE_HALF 0x01
+#define STEPPER_CONFIG 0x00


Change all of these constant prefixes from STEPPER_ to ACCELSTEPPER_ to avoid conflicts if a user includes both the legacy stepper and accelStepper (which hopefully is rare).

soundanalogous · 2017-08-27T19:54:47Z

src/AccelStepperFirmata.h

+#include "utility/MultiStepper.h"
+#include "FirmataFeature.h"
+
+#define MAX_STEPPERS 10 // arbitrary value... may need to adjust


The legacy stepper class declares MAX_STEPPERS as 6. In the rare event a user includes both stepper classes there is a potential conflict here.

Is it okay to just do this?

#ifndef MAX_STEPPERS #define MAX_STEPPERS 10

Nope because if the legacy stepper is included first, that would result in MAX_STEPPERS being defined as 6.

Imagine defines as global variables in a web application.

soundanalogous · 2017-08-27T19:56:16Z

src/AccelStepperFirmata.cpp

+        } else if (wireCount == 4 && stepType == STEP_TYPE_HALF) {
+          stepper[deviceNum] = new AccelStepper(AccelStepper::HALF4WIRE, stepOrMotorPin1, directionOrMotorPin2, motorPin3, motorPin4, false);
+        }
+


nit: remove this extra space

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

src/AccelStepperFirmata.cpp

+
+      }
+
+      else if (stepCommand == ACCELSTEPPER_ZERO) {


What is the use case of Zero? Currently it will stop the motor when called since a position of zero will cause the AccelStepper run method to return false. The only difference between Zero and Stop, is that when calling Zero, the position reported is 0 vs Stop where the position is the current step count. I'm assuming the use case for Zero however is to typically call it while the motor is not running. Is this a correct assumption?

This is usually called after an initial positioning routine has run at start up.

accelStepper instances have an absolute position counter. When an accelStepper is initialized the counter starts at zero. Calling stepper zero resets that value.

setCurrentPosition explicitly sets the speed of the motor to zero. I assume this is because any already running movements could behave unpredictably.

soundanalogous · 2017-09-04T17:47:01Z

src/AccelStepperFirmata.cpp

+      else if (stepCommand == MULTISTEPPER_STOP) {
+
+        groupIsRunning[deviceNum] = false;
+


@dtex what do you think about also callingreportGroupComplete here to make the api more consistent with single stepper stop behavior. This would also help eliminate an issue on the client side (firmata.js) where the multi-stepper-done event is left unhandled if the user calls multiStepperStop.

I think making the API more consistent is an excellent thing to do. I'll make that change.

soundanalogous · 2017-09-04T17:56:00Z

I've tested the latest iterations and finally got around to testing muti-stepper as well. We're super close to being done here. I've left a few comments (hopefully the last round), addressing Zero, a potentially unhandled event in Multi-Stepper stop and an issue in firmata.js with the callback in the accelStepperStop method (was my mistake for requesting that change).

soundanalogous · 2017-09-09T15:35:26Z

I must have unintentionally closed this. Reopening.

dtex · 2017-09-09T15:46:06Z

Should be able to get back on this tomorrow. We are still dealing with recovery (not us fortunately, just neighbors and family).

soundanalogous · 2017-09-16T22:41:44Z

Tested one more time with 4-wire (whole and half step), Driver and multi (one driver + one 4-wire). No issues. This is finally good to go! Thanks for your patience @dtex!

dtex mentioned this pull request Jul 3, 2017

accelStepper firmata/firmata.js#176

Closed

dtex changed the title ~~[WIP] Stepper for configurable firmata~~ [WIP] Stepper2 for configurable firmata Jul 3, 2017

dtex mentioned this pull request Jul 3, 2017

Stepper library callbacks issue rwaldron/johnny-five#1099

Closed

soundanalogous reviewed Jul 3, 2017

View reviewed changes

soundanalogous reviewed Jul 9, 2017

View reviewed changes

soundanalogous reviewed Jul 13, 2017

View reviewed changes

soundanalogous reviewed Jul 15, 2017

View reviewed changes

soundanalogous reviewed Jul 19, 2017

View reviewed changes

dtex changed the title ~~[WIP] Stepper2 for configurable firmata~~ [WIP] accelStepper for configurable firmata Jul 19, 2017

dtex mentioned this pull request Jul 19, 2017

Trying to use 6 steppers. rwaldron/johnny-five#1133

Closed

soundanalogous reviewed Jul 20, 2017

View reviewed changes

soundanalogous reviewed Aug 5, 2017

View reviewed changes

dtex mentioned this pull request Aug 27, 2017

firmataAccelStepper firmata/firmata.js#179

Closed

soundanalogous reviewed Aug 27, 2017

View reviewed changes

soundanalogous reviewed Sep 4, 2017

View reviewed changes

soundanalogous closed this Sep 4, 2017

soundanalogous reopened this Sep 9, 2017

Stepper 2

870f502

soundanalogous merged commit 4c0c4a1 into firmata:master Sep 16, 2017

soundanalogous mentioned this pull request Sep 16, 2017

Update Stepper to wrap AccelStepper #33

Closed

dtex mentioned this pull request Jan 24, 2019

Stop is not correctly implemented? #86

Open

dtex mentioned this pull request Mar 18, 2019

Stepper motors and Johnny five dtex/tharp#3

Open


		if (deviceNum < MAX_STEPPERS) {

		if (stepCommand == STEPPER_CONFIG) {

		else if (stepCommand == MULTISTEPPER_STOP) {

		groupIsRunning[deviceNum] = false;

[WIP] accelStepper for configurable firmata #72

[WIP] accelStepper for configurable firmata #72

Conversation

dtex commented Jul 3, 2017

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

soundanalogous commented Jul 9, 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

Choose a reason for hiding this comment

soundanalogous commented Jul 9, 2017

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

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

soundanalogous commented Jul 15, 2017

dtex commented Jul 15, 2017

soundanalogous commented Jul 15, 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

soundanalogous commented Jul 19, 2017

soundanalogous Jul 19, 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

dtex commented Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtex Aug 13, 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

dtex commented Aug 5, 2017

soundanalogous commented Aug 5, 2017

dtex commented Aug 5, 2017

zfields commented Aug 12, 2017

dtex commented Aug 20, 2017

zfields commented Aug 20, 2017

soundanalogous commented Aug 20, 2017

dtex commented Aug 20, 2017

soundanalogous commented Aug 20, 2017

soundanalogous commented Aug 21, 2017

Choose a reason for hiding this comment

soundanalogous 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

soundanalogous commented Jul 9, 2017 •

edited

Loading

soundanalogous commented Jul 15, 2017 •

edited

Loading

soundanalogous Jul 19, 2017 •

edited

Loading

dtex commented Jul 25, 2017 •

edited

Loading

dtex Aug 13, 2017 •

edited

Loading

soundanalogous Aug 27, 2017 •

edited

Loading