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

[baxtereus] Remove the loading baxter-util.l in baxter-interface.l #593

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@wkentaro
Copy link
Member

commented Apr 20, 2016

In my case, I use baxter-interface.l but don't use baxter-util.l.

@k-okada

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

baxter-util.l re-define IK method, so robot behavior may change if you load baxter-util.l or not , I think this is very dangerous .
Could you explain why you do not need them?

◉ Kei Okada

2016/04/21 0:40、Kentaro Wada notifications@github.com のメッセージ:

In my case, I use baxter-interface.l but don't use baxter-util.l.

You can view, comment on, or merge this pull request online at:

#593

Commit Summary

Remove the loading baxter-util.l in baxter-interface.l
File Changes

M jsk_baxter_robot/baxtereus/baxter-interface.l (1)
Patch Links:

https://github.com/jsk-ros-pkg/jsk_robot/pull/593.patch
https://github.com/jsk-ros-pkg/jsk_robot/pull/593.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

@wkentaro

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

Reasons are:

  1. Firstly, I think this kind of customization should be applied with user option, but currently user does not have option to use baxter-util or not.
  2. Secondly, we use our own customized baxter model in our demo repository, it loads 1. baxter-interface.l, 2. customized baxter.l. So I'm scared about if the loading order is correct to overwrite the method (ex :inverse-kinematics) of baxter-robot class. (if 1. customized baxter.l 2. baxter-interface.l does not work, right?)

Is there best practice to use customized robot model with same robot interface?
I don't know much about :baxter in (require :baxter "package://baxtereus/baxter.l"), but does below work?

(require :baxter "customized-baxter.l")
(require "package://baxtereus/baxter-interface.l") ;; this also run (require :baxter "package://baxtereus/baxter-util.l")

@wkentaro wkentaro force-pushed the wkentaro:remove-baxter-util branch from 3c97df9 to d0dc9f6 Apr 20, 2016

@wkentaro wkentaro changed the title Remove the loading baxter-util.l in baxter-interface.l [baxtereus] Remove the loading baxter-util.l in baxter-interface.l Apr 20, 2016

k-okada and others added some commits Apr 21, 2016

@wkentaro wkentaro force-pushed the wkentaro:remove-baxter-util branch from d0dc9f6 to 544db09 Apr 21, 2016

@k-okada

This comment has been minimized.

Copy link
Member

commented May 12, 2016

A method defined in baxter-utils.l should be considered as a "default" method for the baxter robot, if you create your custom robot and override the ik, then re-define in your source code, if you want to change the IK behavior, change ik in baxter-utils, and if you want to debug IK, use something like (apply #'send-message self (class . super) :inverse-kinematics args)
I think this is no longer needed as discussed in start-jsk/jsk_apc#1365 (comment)

@wkentaro

This comment has been minimized.

Copy link
Member Author

commented May 12, 2016

Yeah, thank you for checking our discussion.
I also reached the same conclusion with you that this PR is not required.

@wkentaro wkentaro closed this May 12, 2016

@wkentaro wkentaro deleted the wkentaro:remove-baxter-util branch May 12, 2016

@wkentaro

This comment has been minimized.

Copy link
Member Author

commented May 13, 2016

I sent PR for chainging baxter-util.l. Could you please review it?
#602

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.