Skip to content

[py] Let setTargetPose fail when invalid kinematic group name passed.#1113

Open
130s wants to merge 2 commits intofkanehiro:masterfrom
130s:fix/py/settargetpose_false_arg
Open

[py] Let setTargetPose fail when invalid kinematic group name passed.#1113
130s wants to merge 2 commits intofkanehiro:masterfrom
130s:fix/py/settargetpose_false_arg

Conversation

@130s
Copy link
Copy Markdown
Contributor

@130s 130s commented Mar 11, 2017

setTargetPose takes a kinematic group as its 1st arg. When invalid value passed, it still tries to solve IK and always fails with an IK solution failure message, which is ambiguous because it failed not because
the computation but because the target group name is simply invalid.

With this change it first checks if the passed value is a registered kinematic group name and if not it fails with the reason accordingly.

@130s 130s force-pushed the fix/py/settargetpose_false_arg branch from 79a5ac2 to 7498603 Compare March 11, 2017 01:20
130s added a commit to 130s/rtmros_hironx that referenced this pull request Mar 11, 2017
…passed.

**Background**
(Same commit message as fkanehiro/hrpsys-base#1113)
`setTargetPose` takes a kinematic group as its 1st arg. When invalid value passed, it still tries to solve IK and always fails with an IK solution failure message, which is ambiguous because it failed not because
the computation but because the target group name is simply invalid.

**Approach to fix**
With this change it first checks if the passed value is a registered kinematic group name and if not it fails with the reason accordingly.

**Additional note at rtmros_hironx**
Same change as fkanehiro/hrpsys-base#1113. This method should be removed as part of start-jsk#470, once fkanehiro/hrpsys-base#1063 resolves.
@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2809/
Test PASSed.

@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2810/
Test PASSed.

@130s 130s force-pushed the fix/py/settargetpose_false_arg branch from 7498603 to 1dbb113 Compare March 11, 2017 03:25
130s added a commit to 130s/rtmros_hironx that referenced this pull request Mar 11, 2017
…passed.

**Background**
(Same commit message as fkanehiro/hrpsys-base#1113)
`setTargetPose` takes a kinematic group as its 1st arg. When invalid value passed, it still tries to solve IK and always fails with an IK solution failure message, which is ambiguous because it failed not because
the computation but because the target group name is simply invalid.

**Approach to fix**
With this change it first checks if the passed value is a registered kinematic group name and if not it fails with the reason accordingly.

**Additional note at rtmros_hironx**
Same change as fkanehiro/hrpsys-base#1113. This method should be removed as part of start-jsk#470, once fkanehiro/hrpsys-base#1063 resolves.
130s added a commit to 130s/rtmros_hironx that referenced this pull request Mar 11, 2017
…passed.

**Background**
(Same commit message as fkanehiro/hrpsys-base#1113)
`setTargetPose` takes a kinematic group as its 1st arg. When invalid value passed, it still tries to solve IK and always fails with an IK solution failure message, which is ambiguous because it failed not because
the computation but because the target group name is simply invalid.

**Approach to fix**
With this change it first checks if the passed value is a registered kinematic group name and if not it fails with the reason accordingly.

**Additional note at rtmros_hironx**
Same change as fkanehiro/hrpsys-base#1113. This method should be removed as part of start-jsk#470, once fkanehiro/hrpsys-base#1063 resolves.
@130s 130s force-pushed the fix/py/settargetpose_false_arg branch from 1dbb113 to 4d15d26 Compare March 11, 2017 03:58
@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2811/
Test PASSed.

@130s
Copy link
Copy Markdown
Contributor Author

130s commented Mar 11, 2017

PR 出した時点では group name は case-sensitive にしていましたが,下流のテストが通らなそう (group name 自体は小文字だが当該メソッドに大文字で入力する箇所がある) 等,api breakeage となりかねないため,case-insensitive に変更済.

@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2812/
Test PASSed.

`setTargetPose` takes a kinematic group as its 1st arg. When invalid value passed, it still tries to solve IK and always fails with an IK solution failure message, which is ambiguous because it failed not because the computation but because the target group name is simply invalid.

With this change it first checks if the passed value is a registered kinematic group name and fails with the reason printed.
@130s 130s force-pushed the fix/py/settargetpose_false_arg branch from 4d15d26 to 3db84c0 Compare March 11, 2017 04:39
@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2813/
Test PASSed.

@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 11, 2017 via email

@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 11, 2017

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2814/
Test PASSed.

@130s
Copy link
Copy Markdown
Contributor Author

130s commented Mar 11, 2017

print(self.configurator_name

足して見ました.テストは pass しています.メソッド名も足していますが,そちらの表記は他と整合しているか自信あまりありません.

@130s
Copy link
Copy Markdown
Contributor Author

130s commented Mar 21, 2017

Review ねがいます

@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented Mar 22, 2017

#1118
と混同しているかもしれないですが、
start-jsk/rtmros_hironx#488
の例もあるし、手元で古いhrpsysでもちゃんと動くかテストしたほうがいいんじゃないかとおもいました.

@130s
Copy link
Copy Markdown
Contributor Author

130s commented May 9, 2017

start-jsk/rtmros_hironx#488 の例もあるし、手元で古いhrpsysでもちゃんと動くかテストしたほうがいいんじゃないかとおもいました.

start-jsk/rtmros_hironx#488 の問題については,それを確認する testcase が無かったから CI で発覚しなかった,ということで start-jsk/rtmros_hironx#489 でそれを足しています.

PR 出した本人が手元で確認して "はい OK でした",というのは,テスト基準がその人依存になってしまうので,基本 CI に委ねるべきと思います (このリポジトリはそうやって判断してると思ってたんですが).

@k-okada
Copy link
Copy Markdown
Contributor

k-okada commented May 10, 2017 via email

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