-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
updates to stepper 2.0 proposal based on comments #79
Conversation
7 num steps, bits 14-20 | ||
8 num steps, bits 21-27 | ||
9 num steps, bits 28-32 | ||
10 speed, bits 0-6 (steps per second * 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need 32-bits for speed as well and may also need to use a higher multiplier than 1000. What we need to determine is the high and low limit. The AccelStepper documentation specifies that values > 1000 steps / sec become unstable, so that can be the upper limit. However the desired resolution of the lower limit will determine the size of the multiplier. For example, if someone wanted 1 step / hour, they'd need to specify a value of 0.00027777, but to pass that as a 32-bit value rather than a float (which Firmata doesn't support) it would require dividing by 100,000 to get 0.00027, and even higher numbers to get better precision.
If the high limit is 1000 steps/sec, then we'd need to send a value of 1000 * 100,000 which still fits within a 32-bit value so we're good there. So I think increasing the resolution to 32-bits and changing the multiplier from 1000 to 100,000 will be sufficient. We could even push the multiplier to 1,000,000 (277 / 1000000 = 0.000277) and still be okay on the high limit since 1 billion still fits in a 32-bit unsigned long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use a separate sign byte could we pass the multiplier value in there?
So...
- Take our number X from N bytes (1-5)
- Take our exponent value (Z) from bits 3-7 of the sign byte
- If the first bit of the sign byte is 1 then X = X * -1
- If the second bit of the sign byte is 1 then Z = Z * -1
Then X=X*10^Z
That limits the multiplier range to [0.00001, 100000] but the client developer isn't locked into the multiplier we prescribe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting general purpose solution to the problem. You get even higher resolutions by using the upper two bits for the sign and then using the 5 lower bits for the exponent, where the value of those 5 bits is the exponent value (up to 31).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked this up... with Arduino, floats are 32-bit and the highest precision is 6 or 7 decimal digits (total digits, not just to the right of the decimal point) so the highest precision we can get is 10^6 or maybe 10^7) according to this: https://www.arduino.cc/en/Reference/Float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't mention what standard they use for representing floats in memory. I'm guessing IEEE 754, Do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if I'm wrong, but wouldn't a 4-bit exponent yield the range +/- 7?
Do these tables line up with what you're thinking?
Stepper speed value
27 | 26-23 | 22-0 |
---|---|---|
sign | exponent | significand |
1 bit | 4 bits | 23 bits |
Stepper accel value
20 | 19-16 | 15-0 |
---|---|---|
sign | exponent | significand |
1 bit | 4 bits | 16 bits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With 4 bits (no sign) we get 0-15. If we bias that by -18 we git a range -18 to -3 though I see now that the range example I gave assumes an exponent of -2 so that's a -17 bias.
A -17 bias gets us a max value of ~83,000 and a -18 bias gets us ~8,300. Both are well above our max steps/second of 1,000 allowed by accelStepper, but I'm inclined to go with the -17 bias because I've found projects where people have micro-stepped at 25,000 steps per second. I know we can't support micro-stepping yet but we might one day and it would suck to change the bias on people.
Yes, those tables are exactly what I was thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with the exponent offset of -3 you represent 1000.0 as 1000000.0 for the mantissa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the radix point would be at the end of the mantissa. The exponent value passed would be 14 with the -17 bias applied on the receiving end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may also be useful to use 4 bytes for the accel value as well. It simplifies things on the firmware side to only have to use a single calculation to get the float.
9 speed, bits 0-6 (steps per second * 1000) | ||
10 speed, bits 7-13 | ||
11 speed, bits 14-16 | ||
12 [optional] accel, bits 0-6 (acceleration in steps/sec^2 * 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if accel and decel belong here. If so, we should also define for the step
command. Also AccelStepper only allows specifying an acceleration value, which is applied as both the acceleration and deceleration (you can't sepecify them independently). However I think it's worth leaving decel in case AccelStepper adds support in the future or if this protocol is implemented using a different library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, setting the acceleration value is an expensive procedure in the firmware since it involves calculating a square root which is slow in 8/16 bit microcontrollers. Therefore it's best to set acceleration once and change it only as necessary. One issue with AccelStepper however is it appears you cannot unset an acceleration value, you can only change it once it has been set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had already decided to remove accel and decel from here and add them back as separate commands. I've done that, but it doesn't appear to be in GH or on my laptop so it's still back on my workstation at home (I haven't synced yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries... No rush here. I've just had a bunch of free time the past couple of days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize now that I did merge the addition of setAccel (and it is here), I just didn't remove those bytes from the to() method. I can think of no compelling reason not to axe it (Also, it erroneously asks for separate values for accel and decel).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets remove the optional accel and decel bytes then. We can always add them back later if use determines they would be convenient and not negatively impact performance.
4 accel LSB (acceleration in steps/sec^2) | ||
5 accel MSB | ||
6 END_SYSEX (0xF7) | ||
4 accel, bits 0-6 (acceleration in steps/sec^2 * 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD if 16 bits is sufficient for accel and also if 1000 is the best multiplier. We need to determine the min and max values to achieve the best resolution.
Anyone have any numbers for reasonable min and max acceleration values in steps/sec^2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go with 4 bytes here - same as speed. This shouldn't be called too frequently anyway (due to the square root calculation AccelStepper does when an acceleration value is set), so the 4th byte isn't a lot of overhead.
accel
27 | 26-23 | 22-0 |
---|---|---|
sign | exponent | significand |
1 bit | 4 bits | 23 bits |
8 num speed LSB (steps per second) | ||
9 num speed MSB | ||
10 END_SYSEX (0xF7) | ||
5 num steps, bits 0-6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed steps and position to a 32-bit long because that is what AccelStepper uses and it seems like a reasonable value. One thing to figure out here however is if this value should be a signed long where the sign indicates the direction or if we should have a separate direction byte (as specified above) and then use an unsigned long for the number of steps. Currently this is inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd favor a signed long because it doesn't really impact the number of bytes required though I think it should really be addressed in this bigger picture firmata/arduino issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A signed long would impact the number of bytes required. You'd need enough bytes to span the full 32 bits in order to capture the signed value which is indicated by the most significant bit. If we used unsigned and a separate direction byte, you could use any number of bytes and cast to a signed long (multiplying by -1 if the direction is negative).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it. We would just use however many bits/bytes are required (plus a sign byte) to represent the range of min/max values in the particular situation w/o regard to matching certain C types. It could be 1-5 bytes plus a sign byte depending on the situation.
I figured the same, I just thought we could always use the first bit of the MSB (regardless of number of bytes) to indicate sign. There will be occasions where loss of that one extra bit will force us to add a byte to get the range we need.
Given that 7-bit bytes are such an awkward fit for the bit sizes of the number types we're using, I think we're not going to run into that complication very often.
I feel a little silly arguing to shave 1-byte but given some of the other bit-wizardry happening in firmata I always assumed that message length really mattered. If it doesn't than there is no real value in my argument... A dedicated sign byte is easier to deal with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it gets interesting when considering the properties of numbers in the various languages that a Firmata client library has been created for. With JavaScript for example, Numbers are 64-bits so if you wanted to send a 16 bit value and shifted 16 bits into 3 Firmata bytes, you would not get the signed bit since that would be the the MSB of the 64-bit Number (different of course if using typed Arrays in JS). So I guess a separate sign byte (same as direction byte) may be necessary, or the MSB bit would need to be moved to the MSB of the size expected by Firmata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way having util functions on both sides will be necessary to ensure the correct value is sent and received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soundanalogous, @dtex Sorry I had missed this thread before.
I agree with @dtex that using a signed 32-bit step and dropping the extra direction byte seems better here. This would make it consistent with move absolute, which currently takes a signed 32-bit as well.
More importantly, as the current design already indicates, directionality is clearly an important part of the semantics of this command (which is why the direction byte is there). If this is the case, then why not use the natural semantics of signed 32-bit to represent this?
If there are no foreseeable use cases where you would need the extra bit anyway, I would rather have a simpler (and slimmer) protocol.
@@ -40,7 +40,7 @@ Protocol | |||
6 num steps-per-revolution LSB | |||
7 num steps-per-revolution MSB | |||
8 maxSpeed LSB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably get away with maxSpeed as a 14-bit number if we don't require a multiplier for it like we do when setting the speed value. In this case maxSpeed would probably rarely exceed 1000.
Next big decisions to make here are:
Discussions around 1 and 2 are ongoing in comments above. |
Just got notice of this pull request. Great job, I did myself a SYSEX using AccelStepper about 5 years ago and it was really very similar, so I'm happy to see it finally happen in the official Firmata. A couple of minor comments (also, let me know if there is anything I can do to help, I would love to see this folded into Firmata ASAP):
I'm folding this into my client C# Firmata implementation as we speak, and will be testing it with a group of steppers in the next couple of days, so I will provide more feedback as I stumble on stuff. Good work incorporating position reports at the end of move commands, I've used them so many times already! |
The most extreme long running example I can think of is a sidereal tracking drive for a telescope. With a typical worm gear/spur combo and a 1.8°/step stepper you could still explicitly reference every single step the telescope would make in an entire year with a 31-bit integer so a signed 32-bit should be plenty big. I also believe we can pack sign bit from the MSB on the host side into the message's MSB so I'm on board with @glopesdev here
All good points. Thinking out loud here... We can't do variable length 32-bit signed ints (with the traveling sign bit) when passing an array of values. That trick only works when passing a single value with no optional parameters in the message (otherwise no way to know where one value ends and another begins). That means 5 7-bit bytes with three wasted bits for every stepper in the group. If we limit groups to 8 members per group we could pack the member number into those unused 3 bits.
If a user is frequently controlling just two steppers within a larger group and doesn't want to send the other values, there is no reason they couldn't create a second group with just those two steppers and pass their values to that (it appears steppers can be in more than one group). |
@dtex Perfect, completely agree! |
@dtex and @glopesdev if you could make inline comments for sections with you would like changed, along with the updated text, any tables to add, detailed specs for how a user should specify a floating-point value, etc, that would be helpful. Alternatively I can merge this tonight and then you can create separate PRs with your changes. |
I'd favor a separate PR just because there are so many comments here already it would be difficult to digest (some guy named @dtex blathers on a lot). |
@@ -181,12 +210,15 @@ Stepper instances that have been created with the stepper configuration command | |||
2 multi to command (0x21) | |||
3 group number (0-127) | |||
4 member number (0-9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like member number could be dropped here. MultiStepper To command would expect to receive as many blocks of "num steps" as there are members in the group, and the index of each block implicitly specifies the group member.
As @dtex suggested, in cases where only a subset of the steppers needs to be controlled, a new group can be created with only the subset as members.
@soundanalogous done, thanks for the feedback! |
I've merged this as is. Please submit separate pull requests to address topics such as composition of floats (I took a high level pass, but needs to be more detailed) as well updates per other comments that have not yet been integrated into the spec. |
Update addressing a number of comments from #42 (comment).
Please add any additional comments inline.