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

Finalized cob_cartesian_controller #74

Merged

Conversation

MaChristoph
Copy link
Contributor

I finalized the cob_cartesian_controller. There was a bug in the cob_frame_tracker which threw an transform exception which I also fixed in my latest commit.

{
double tb, te, tv;
unsigned int steps_tb, steps_te, steps_tv;
bool ok;
Copy link
Contributor

Choose a reason for hiding this comment

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

could we get rid of this?
E.g. by letting the according getProfileTimings function return a bool instead of a ProfileTimings struct. The struct should then be passed to the function as a reference parameter (also see my comment below https://github.com/ipa320/cob_control/pull/74/files#r47380086)

double vel_max = params_.profile.vel;

// Calculate the Profile Timings
pt = getProfileTimings(pa.Se_, pt_max_.te, accl_max, vel_max, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

see according comments in "ramp"

@fmessmer
Copy link
Contributor

Just another browser review...
If I'm able, I'll do some tests myself...maybe even do the roslint-styling!
I'll let you know about it...

@fmessmer
Copy link
Contributor

In my updated branch, I considered all styling/restructure comments I made here three days ago...simply git pull origin-fxm test_new_cartesian_controller once again to get the updates!

Beside the styling, I significantly reduced unused/obsolete variables/functions, simplified function parameters by using structs/member variables, remove code duplicates....have a look at my commit history to see the steps...

I tested it in simulation with schunk_lwa4d...the behavior of lwa4d_circle.py is as expected, i.e. the same as without my changes 😉

@ipa-fxm-cm, could you review my additional changes and test again?
Please also run a test on the real robot (schunk_lwa4d)...and comment on this PR whether the hardware test was successful!

To finalize this PR, could you have a look at these comments of mine and try them out?

Finally, after the tests, the lwa4d scripts should be removed from cob_control and added to the schunk_lwa4d package directly (Please git rm the files in cob_control and add them to schunk_robots...then provide PR there, too)

@fmessmer
Copy link
Contributor

Closes #52 (after the changes from my test_new_cartesian_controller branch have been merged and tested!)

@fmessmer
Copy link
Contributor

Do not merge yet....branches are messed up....I'm trying to fix it as soon as @ipa-fxm-cm has made me Collaborator to his repo...

@fmessmer fmessmer force-pushed the test_new_cartesian_controller branch from 8ab4b14 to 1a29538 Compare December 19, 2015 17:55
@fmessmer
Copy link
Contributor

I pushed my branch test_new_cartesain_controller onto @ipa-fxm-cm branch...now, the history of this PR is ok and includes only feature-related changes....(some minor roslint hints in cob_twist_controller)

Can be merged!

@fmessmer
Copy link
Contributor

Merging now in order to go on with this...

fmessmer added a commit that referenced this pull request Jan 12, 2016
@fmessmer fmessmer merged commit a24bc30 into 4am-robotics:indigo_dev Jan 12, 2016
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.

2 participants