Zultron/tracksuite refactor pt1 by zultron · Pull Request #62 · PickNikRobotics/trackjoint

zultron · 2020-09-17T16:01:05Z

This PR implements some of the changes described in the TrackSuite Unification Development document:

"Refactoring tasks" section:
- Item (1), remove calculations from constructor, commit eb48455
- Item (2), move configuration member vars into struct, commit 5b7207c
- (Item (3), convert member vars to member function args, postponed)
"Future milestones" section:
- Item (1), investigate the failing unit test, commits 6413123 and 601f825

It also contains several small cleanups to correct compiler warnings, adjust variable types and remove unneeded files.

zultron · 2020-09-17T19:14:06Z

OK, so this PR is rebased on #63 now, which fixes the previous failures in the clang-related CI checks. Once #63 is merged, the diff here will be a lot skinnier and easier to review. Alternatively, the top four commits can be reviewed one by one.

zultron · 2020-09-17T20:04:18Z

Converting to draft until I find out why I'm getting a segfault running in MoveIt. ;(

zultron · 2020-09-18T19:20:59Z

PickNikRobotics/moveit#10 fixes the segfault. Since the constructor no longer initializes the object, the reset() method needs to be called separately after instantiating the object. With that patch on the moveit side, this work can now be tested in a MoveIt pipeline following the "How To Run TrackJoint on UR5" gdoc.

I will update this branch to not segfault when calling generateTrajectory() on an object that hasn't been reset() first!

zultron · 2020-09-18T20:51:30Z

The newly-added "Member variable refactor: Don't segfault if object not reset()" commit fixes the segfault when reset() isn't called before generateTrajectories().

I'm removing "draft" status; this PR is ready to review after #63 and #58.

AndyZe · 2020-09-20T22:02:55Z

It needs a rebase now that #63 was merged

test/trajectory_generation_test.cpp

AndyZe · 2020-09-20T22:09:20Z

test/trajectory_generation_test.cpp

We almost always use ++joint instead of joint++. Apparently it's usually faster (but that's debatable). See

https://stackoverflow.com/a/38948073/3499467

Fixed. Good to know, thanks!

https://github.com/PickNikRobotics/trackjoint/pull/62/files#diff-65c6fed365fc8c7f2e56ebc530577da6R509

test/trajectory_generation_test.cpp

AndyZe · 2020-09-20T22:14:59Z

src/single_joint_generator.cpp

index_last_successful shouldn't have an underscore since it's an argument, not a member variable

Whoops! I got confused.

Just to be clear, this was unintentional too, right? I'll fix these all at once.
7c937ad#diff-dc20813d2542db5e34c181dc39240a4fR522

D'oh! Reverted.

include/trackjoint/configuration.h

AndyZe · 2020-09-20T22:27:42Z

It looks like the three examples are throwing this error now.

e.g. rosrun trackjoint simple_example

terminate called after throwing an instance of 'std::out_of_range'
  what():  _Map_base::at

zultron · 2020-09-21T15:57:02Z

It needs a rebase now that #63 was merged

Done.

zultron · 2020-09-21T16:00:50Z

It looks like the three examples are throwing this error now. [...]

I'll take care of this. Thanks1

SingleJointGenerator::reset does these same computations, refactor code to use this instead

Configuration variables are moved to a struct, written to by `reset()` and used by other methods.

Record traveled trajectory for inspection by `checkBounds()` in `TearDown()` function

To simulate velocity-, acceleration- and jerk-limited motion, the next state's velocity and acceleration must be updated along with position.

zultron · 2020-09-21T18:13:08Z

It looks like the three examples are throwing this error now. [...]

Ouch, these were a failure on top of a failure, extra embarrassing. Fixed.

#64 adds the example programs to a test to prevent future regressions.

zultron · 2020-09-21T18:22:40Z

OK, I believe I've addressed all comments, and CI is green! Thanks for the detailed review.

AndyZe

LGTM, I'll wait for Nathan to review this one, if he pleases.

* Member variable refactor: Remove calculations from constructor SingleJointGenerator::reset does these same computations, refactor code to use this instead * SingleJointGenerator: Move configuration variables to struct Configuration variables are moved to a struct, written to by `reset()` and used by other methods. * NoisyStreamingCommand test: Record traveled trajectory Record traveled trajectory for inspection by `checkBounds()` in `TearDown()` function * NoisyStreamingCommand test: Add velocity/acceleration to next state To simulate velocity-, acceleration- and jerk-limited motion, the next state's velocity and acceleration must be updated along with position.

zultron force-pushed the zultron/tracksuite-refactor-pt1 branch from 601f825 to 1e9323c Compare September 17, 2020 16:43

zultron mentioned this pull request Sep 17, 2020

andyz/squashed_improvements branch, merged w/master, clang tests fixed #63

Merged

zultron force-pushed the zultron/tracksuite-refactor-pt1 branch from 1e9323c to f7d521a Compare September 17, 2020 18:54

zultron marked this pull request as draft September 17, 2020 20:03

zultron force-pushed the zultron/tracksuite-refactor-pt1 branch from f7d521a to 04148bf Compare September 18, 2020 20:44

zultron marked this pull request as ready for review September 18, 2020 20:51

zultron force-pushed the zultron/tracksuite-refactor-pt1 branch from 04148bf to e4cf070 Compare September 20, 2020 07:16

AndyZe reviewed Sep 20, 2020

View reviewed changes

test/trajectory_generation_test.cpp Outdated Show resolved Hide resolved

AndyZe reviewed Sep 20, 2020

View reviewed changes

AndyZe changed the base branch from andyz/squashed_improvements to master September 20, 2020 22:24

AndyZe changed the base branch from master to andyz/squashed_improvements September 20, 2020 22:25

zultron force-pushed the zultron/tracksuite-refactor-pt1 branch 2 times, most recently from c5beeb1 to 8094097 Compare September 21, 2020 15:52

zultron added 4 commits September 21, 2020 12:53

Member variable refactor: Remove calculations from constructor

1be4225

SingleJointGenerator::reset does these same computations, refactor code to use this instead

SingleJointGenerator: Move configuration variables to struct

8525449

Configuration variables are moved to a struct, written to by `reset()` and used by other methods.

NoisyStreamingCommand test: Record traveled trajectory

b737ec3

Record traveled trajectory for inspection by `checkBounds()` in `TearDown()` function

NoisyStreamingCommand test: Add velocity/acceleration to next state

883c5b1

To simulate velocity-, acceleration- and jerk-limited motion, the next state's velocity and acceleration must be updated along with position.

zultron force-pushed the zultron/tracksuite-refactor-pt1 branch from 8094097 to 883c5b1 Compare September 21, 2020 17:56

zultron mentioned this pull request Sep 21, 2020

Test example programs #64

Open

zultron mentioned this pull request Sep 21, 2020

single_joint_generator: Remove trailing _ from function parameter #65

Merged

AndyZe approved these changes Sep 21, 2020

View reviewed changes

AndyZe merged commit 197218b into PickNikRobotics:andyz/squashed_improvements Oct 5, 2020

Conversation

zultron commented Sep 17, 2020

Uh oh!

zultron commented Sep 17, 2020

Uh oh!

zultron commented Sep 17, 2020

Uh oh!

zultron commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zultron commented Sep 18, 2020

Uh oh!

AndyZe commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AndyZe Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

zultron Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

zultron Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndyZe Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

zultron Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zultron Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndyZe commented Sep 20, 2020

Uh oh!

zultron commented Sep 21, 2020

Uh oh!

zultron commented Sep 21, 2020

Uh oh!

zultron commented Sep 21, 2020

Uh oh!

zultron commented Sep 21, 2020

Uh oh!

AndyZe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zultron commented Sep 18, 2020 •

edited

Loading

AndyZe commented Sep 20, 2020 •

edited

Loading

zultron Sep 21, 2020 •

edited

Loading