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

Add Jerk, L^1 * T^-3 #128

Merged
merged 1 commit into from
Apr 27, 2019
Merged

Add Jerk, L^1 * T^-3 #128

merged 1 commit into from
Apr 27, 2019

Conversation

nicodemus26
Copy link
Contributor

Useful for robotics and motion planning.

Also added some common CNC units to velocity and acceleration.

@coveralls
Copy link

coveralls commented Apr 24, 2019

Coverage Status

Coverage increased (+0.08%) to 97.194% when pulling 09d5811 on nicodemus26:add-jerk into e70b0a4 on iliekturtles:master.

Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Initial review looks really good. I only saw a couple minor items. I still need to do one more detailed review which I hope to find time for in the next few days.

src/si/jerk.rs Outdated Show resolved Hide resolved
src/si/jerk.rs Show resolved Hide resolved
@nicodemus26
Copy link
Contributor Author

If approved, could you cut a minor or patch release that I may pin in my project?

Thanks kindly!

Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

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

Couple more minor changes requested.

I'd also like to see the commits organized / squashed. Ran out of time right now, but will follow up with details.

I'll plan to do a new release once these are merged. I'm hoping to include Angle and Torque at the same time, but I need some more in depth review that may delay them and I'll release this alone.

src/si/mod.rs Outdated
@@ -35,6 +35,7 @@ system! {
energy::Energy,
force::Force,
frequency::Frequency,
jerk::Jerk,
Copy link
Owner

Choose a reason for hiding this comment

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

Move down below inductance::Inductance.

fn test<L: l::Conversion<V>, T: t::Conversion<V>, J: j::Conversion<V>>() {
Test::assert_eq(
&Jerk::new::<J>(V::one()),
&(Length::new::<L>(V::one()) /
Copy link
Owner

Choose a reason for hiding this comment

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

Start next line with / operator. Do the same for the * operators. Tab in the contents of the inner-most ()s where the time^3 factor is calculated.

            fn test<L: l::Conversion<V>, T: t::Conversion<V>, J: j::Conversion<V>>() {
                Test::assert_eq(
                    &Jerk::new::<J>(V::one()),
                    &(Length::new::<L>(V::one())
                        / (Time::new::<T>(V::one())
                            * Time::new::<T>(V::one())
                            * Time::new::<T>(V::one()))));
            }

@iliekturtles iliekturtles merged commit 21b59ad into iliekturtles:master Apr 27, 2019
@iliekturtles
Copy link
Owner

Thanks again for the PR! There are a couple minor changes I'm going to include in another commit shortly rather than have you do it. My plan is do some more review of the Torque PR tomorrow and release a new version to crates.io either way.

@iliekturtles
Copy link
Owner

v0.22.2 released!

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.

3 participants