Skip to content
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

Revisions to stepper-2.0 #81

Merged
merged 4 commits into from
Jan 19, 2017
Merged

Revisions to stepper-2.0 #81

merged 4 commits into from
Jan 19, 2017

Conversation

dtex
Copy link
Contributor

@dtex dtex commented Jan 12, 2017

Drop member number from multiStepper commands

Comment updates

Change bias for exponent values to -17

Drop member number from multiStepper commands.

Comment updates.

Change bias for exponent values to -17
@dtex
Copy link
Contributor Author

dtex commented Jan 12, 2017

I still need to add details for the float composition

@@ -66,17 +66,16 @@ Protocol

**Stepper step (relative move)**

Position is specified as a 32-bit signed long.
Steps to move is specified as a 32-bit signed long.

The speed value is a float composed of a 23-bit significand (mantissa) and a 4-bit exponent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For just one of the 3 sections that describes how the float is composed, can you provide an example for a specific number or two (maybe 500.0 and 0.000234 or something like that) how this would be broken out into the 3 parts specified in the table?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better if the example(s) align to a specific duration (such as 1 step per hour for "slow" and 10 steps per second for "fast"). You probably have a better sense for practical examples than I do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I might mention the custom float format up front, describe it in detail at the end of the document (with examples) and for each method where it's used just refer users to that detailed description.

Sound good?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

@dtex
Copy link
Contributor Author

dtex commented Jan 14, 2017

Working on those examples and testing as I go, I've realized that I don't have a clear understanding of how AVR-GCC fakes floats. The data format probably should be transmitted in a format that closely matches what we actually need. I think the root of the problem is I've been using decimal32 representation instead of binary32.

@soundanalogous
Copy link
Member

Arduino is not just avr-gcc anymore. Various architectures each have their own compiler and may handle the float to int conversion in different ways. As long as we use a normalized representation of a float we should be okay. On the firmware side that unpacked value will be assigned to an Arduino float so it should then be set properly in memory at that point.

@dtex
Copy link
Contributor Author

dtex commented Jan 19, 2017

I added those examples, and I have one last thought. Both maxSpeed and minPulseWidth require separate calls to change their default values in accelStepper. Since a reasonable default exists for both should we remove those values from the configuration command and give them their own command?

@soundanalogous
Copy link
Member

We can pull them out to their own commands and if they don't already have default values within the AccelStepper implementation, we can give them default values upon receiving the config command. That will allow the user to not have to worry about those values in many cases.

@dtex
Copy link
Contributor Author

dtex commented Jan 19, 2017

Done.

1 Stepper Command (0x62)
2 set minPulseWidth command (0x0A)
3 device number (0-9) (Supports up to 10 motors)
4 maxSpeed (minimum pulse width in microseconds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minPulseWidth

@dtex
Copy link
Contributor Author

dtex commented Jan 19, 2017

Thanks, fixed.

@@ -184,6 +181,33 @@ using Stepper 2.0's custom float format described below.
8 END_SYSEX (0xF7)
```

**Stepper set maxSpeed**

Sets the maximum speed in steps per second
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to decide here if we want to specify in steps/second, in which case we probably need to use the same 4 byte float we're using for speed and accel. Or we can allow specifying steps/sec as an integer here since it will always be > 0 and < 1000 and we shouldn't lose too much in rounding. However we should probably call out the need to round if that is the route we want to take. I prefer the former for consistency, but the later for simplicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think consistency should rule here. Besides, accelStepper's setMaxSpeed receives a float so it's not like we're making a performance improvement by using an integer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg, lets change setMaxSpeed to use the 4 byte float then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's done.

@soundanalogous
Copy link
Member

Cool, this LGTM. I'll merge now unless you wanted to make any other changes.

@dtex
Copy link
Contributor Author

dtex commented Jan 19, 2017

I feel pretty good about it so I'd say merge away. Starting in on configurableFirmata now.

@soundanalogous soundanalogous merged commit 8bdb8b2 into firmata:master Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants