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

[jsk_fetch_startup] update navigation-utils.l to work auto-dock correctly. #1160

Merged
merged 12 commits into from
Nov 26, 2020

Conversation

knorth55
Copy link
Member

update auto-dock.l to work correctly.

@knorth55 knorth55 requested a review from 708yamaguchi October 25, 2019 16:58
@knorth55 knorth55 changed the title update auto-dock.l [jsk_fetch_startup] update auto-dock.l Oct 25, 2019
@k-okada
Copy link
Member

k-okada commented Oct 26, 2019

LGTM, but why the fetch can autodock without this modification ? @708yamaguchi

@708yamaguchi
Copy link
Member

In auto-dock.l, *ri* is created when navigation-utils.l is loaded.
Therefore, (require :fetch-interface "package://fetcheus/fetch-interface.l") and (fetch-init) in this PR are not necessarily required.

(require :fetch-interface "package://fetcheus/fetch-interface.l")
(unless (boundp '*ri*)
(fetch-init))

However, I think this PR helps people to understand the code.

@knorth55
Copy link
Member Author

I mean adding :clear-costmap at the beginning is important to start this demo smoothly.

@k-okada
Copy link
Member

k-okada commented Oct 27, 2019

In auto-dock.l, ri is created when navigation-utils.l is loaded.

ok, if so we need decide policy, whether if we should create *ri* within navigation-utils.l or not. If we decided to initialize *ri* outside navigation-utils.l, remove (fetch-init) within navigation-utils.l with comments and add (fetch-init) for all app programs loading navigation-utils.l.

@knorth55
Copy link
Member Author

i meant :clear-costmap is the most important point in this PR.
in order to do it, we first make ri.

@knorth55 knorth55 changed the title [jsk_fetch_startup] update auto-dock.l [jsk_fetch_startup] update auto-dock in navigation-utils.l Nov 9, 2019
@knorth55
Copy link
Member Author

knorth55 commented Nov 9, 2019

I change to do :clear-costmap in (auto-dock) function in navigation-utils.l.

@knorth55
Copy link
Member Author

knorth55 commented Nov 9, 2019

I'm using this function in #1152

@knorth55
Copy link
Member Author

knorth55 commented Nov 9, 2019

@708yamaguchi can you review this again?

@708yamaguchi
Copy link
Member

@k-okada says

ok, if so we need decide policy, whether if we should create ri within navigation-utils.l or not. If we decided to initialize ri outside navigation-utils.l, remove (fetch-init) within navigation-utils.l with comments and add (fetch-init) for all app programs loading navigation-utils.l.

I think making *ri* inside of navigation-utils.l is no problem because we want to use many methods of *ri* in navigation-utils.l.

Regarding :clear-costmap, if you think :clear-costmap is important, how about using :clear-costmap in (go-to-spot) instead of (auto-dock)?

@708yamaguchi
Copy link
Member

thank you!
Looks good to me.

@YutoUchimi YutoUchimi added this to the 1.1.1 milestone Nov 19, 2019
@k-okada k-okada requested a review from pazeshun November 25, 2019 07:24
@knorth55 knorth55 changed the title [jsk_fetch_startup] update auto-dock in navigation-utils.l [jsk_fetch_startup] do clear-costmap everytime in go-to-spot Nov 25, 2019
@knorth55 knorth55 changed the title [jsk_fetch_startup] do clear-costmap everytime in go-to-spot [jsk_fetch_startup] do clear-costmap everytime in go-to-spot function in navigation-utils.l Nov 25, 2019
@knorth55 knorth55 changed the title [jsk_fetch_startup] do clear-costmap everytime in go-to-spot function in navigation-utils.l [jsk_fetch_startup] update navigation-utils.l to work auto-dock correctly. Nov 25, 2019
@knorth55
Copy link
Member Author

Finally, this PR contains several modification.

  • change the distance in auto-dock function
  • do clear-costmap in every go-to-spot function
  • pass dock position to improve auto-dock success ratio.

the comments posted in this thread are totally misunderstood about this PR, and I changed this PR after the comments.
I have no idea what the policy is, but I fix to work auto-dock properly.
It is nonsense to talk about coding policy in this PR.
I'm using this commit in Fetch15 now for go-to-kitchen app.

@knorth55
Copy link
Member Author

knorth55 commented Apr 4, 2020

@k-okada can you merge this PR?

@k-okada k-okada merged commit 0c9df68 into jsk-ros-pkg:master Nov 26, 2020
@knorth55 knorth55 deleted the fetch-fix-dock branch November 26, 2020 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants