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

Fix timeout calculation for Create 3 navigate_to() #51

Open
wants to merge 1 commit into
base: pre_0.6.0
Choose a base branch
from

Conversation

zur6322
Copy link

@zur6322 zur6322 commented Oct 18, 2024

See modification & description between lines 165 to 185; to address premature timeout when navigating to origin using the navigate_to() function from longer distances

Original line was calculating the timeout based off the passed destination coordinates, not the distance traveling.
When traveling a longer distance to the origin(0,0), or a destination near the origin, the navigate_to() function
would time out before the destination was reached. For example, if traveling to the origin (0,0) from a location
further away than the robot can transit in 7 seconds, the robot will return from navigate_to() and begin processing
the next function, long before the robot ever reaches (0,0).
To correct this issue, the current position is polled, and delta_x & delta_y, which represent the actual distance
to the destination are calculated by subtracting the current position from the destination position. Then, these
values are used in place of X and Y in the original timeout calculation.

See modification & description between lines 165 to 185; to address premature timeout when navigating to origin using the navigate_to() function from longer distances
@shamlian shamlian changed the base branch from main to pre_0.6.0 March 17, 2025 01:10
Copy link
Collaborator

@shamlian shamlian left a comment

Choose a reason for hiding this comment

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

Thank you for this bugfix, but there are some changes necessary before the PR can be accepted.

Comment on lines +5 to +6
#-------------See changes in lines 164 through 185 to address premature timeout when navigating to origin using navigate_to()-----------------

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these lines. These, and the commentary below are helpful, but they belong in the pull request, not in the final source.

Comment on lines +179 to +180
delta_x=(x-self.pose.x)
delta_y=(y-self.pose.y)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should not be parenthesis around the expressions -- the lines should read something like

delta_x = x - self.pose.x
delta_y = y - self.pose.y

Comment on lines +182 to +185

#original line# timeout = self.DEFAULT_TIMEOUT + int(math.sqrt(x * x + y * y) / 10) + 4 # 4 is the timeout for a potential rotation.

#-------End Modification------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove these lines. No reason to keep the dead code.

self.get_position()
delta_x=(x-self.pose.x)
delta_y=(y-self.pose.y)
timeout = self.DEFAULT_TIMEOUT + int(math.sqrt(delta_x*delta_x + delta_y * delta_y) / 10) + 4 # 4 is the timeout for a potential rotation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add spaces between the first * as you did with the second * -- that is, the line should read:

        timeout = self.DEFAULT_TIMEOUT + int(math.sqrt(delta_x * delta_x + delta_y * delta_y) / 10) + 4  # 4 is the timeout for a potential rotation.

@shamlian shamlian changed the title Update create3.py Fix timeout calculation for Create 3 navigate_to() Mar 17, 2025
@shamlian
Copy link
Collaborator

I see now that you have allowed maintainers to edit your PR; I'm happy to try to make the changes and merge after testing.

@zur6322
Copy link
Author

zur6322 commented Mar 22, 2025 via email

@shamlian
Copy link
Collaborator

I really appreciate it! For me to edit this PR, you'd have to add me as a collaborator. Here are the directions to do that. If you'd rather not bother, let me know, and I can reproduce this PR in the repository, but I'd love for you to have the credit on this PR if you'd like it.

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