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

Retry docking if the mower rolls off the charger #114

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

jeremysalwen
Copy link
Contributor

@jeremysalwen jeremysalwen commented Jul 2, 2024

This adds a new boolean to the idle state that tracks whether the mower is on the dock. If the mower detects that it is on the dock and it is no longer charging, it undocks and tries to redock.

This only applies to the cases where the mower has docked itself. If you manually place the mower on the charger this behavior will not activate.

@Apehaenger
Copy link
Collaborator

Hi!
Interesting PR. Unfortunately I'm not familiar enough with ROS to understand it fully.

But could you please explain whats the reason for this PR?
Does your mower stops charging while in the dock?

I'm also curious what would happen if I take my mower manually out of the dock, i.e. to start it via CoverUI?
Or what will happen in the case of a power outage?

@ClemensElflein
Copy link
Owner

It is common behavior on robots, I have seen it on my roborock and neato vacuum robots.

I think instead of doing the full undocking, wait for GPS, redock, a simpler and more robust way would be to just drive forward a max of maybe 10cm very slowly? Reasoning is that we were docked before, so there cannot be much distance to cover and also the alignment has to be correct already.

This way @Apehaenger's concern is also addressed in the sense that if you remove it manually, the wheels will slowly spin for one revolution or so.

What do you think?

@Apehaenger
Copy link
Collaborator

Well...

in my understanding with physics: "mower rolls off the charger" is easiest fixed by changing the gravity effect ;-)

Even though, I probably can't imagine others environment, I also did not know that it's a common behavior for robots.
In addition to the "roll off" issue, one can also have a bad contact on the charger forks which would also get fixed by such a functionality.

But I've doubts with a "undocking phase". It should be more a short "rearrange phase" like @ClemensElflein noted.

@jeremysalwen
Copy link
Contributor Author

The goal of this PR is to fix "flaky" contacts, and issues with the robot rolling backwards over time. I only have personal experience with the former, but I have also seen my wheels jerking slightly while docked which makes me concerned about the latter (an issue which others on the Discord have reported encountering). Other users on discord have already expressed strong interest in this patch. I could get into the specifics of what exactly is happening during docking to cause flaky contact if that is important.

IMO repeating an undock/dock cycle is simpler because it is reusing existing functionality that is well tested, and doesn't introduce a new state for the robot to be in. I agree it's less efficient though.

I think the concerns about picking up the robot could be avoided (for either approach) by turning off stay_docked if we go into emergency mode.

I don't have anything against the "inch forward" approach, I just am unable to properly develop it due to the incredible difficulty I've had setting up an edit-compile-debug loop for openmower with mowgli. I also would note that if the inching forward fails, we probably would want to perform a dock/undock loop anyways, so the approach is not mutually exclusive.

@gytisgreitai
Copy link
Contributor

I would be on the " drive forward a max of maybe 10cm very slowly" . Undocking and docking can bring its own set of issues. Because currently What I do if the robot rolls back, just enter into recording mode, and drive a bit forward

@jeremysalwen
Copy link
Contributor Author

I believe this PR is a step forward for either solution. I would hope it's not blocked because people are enthusiastic about "inching forward".

@ClemensElflein Are there any changes you would want to accept this PR? I could add an environment variable to turn it off.

@ClemensElflein
Copy link
Owner

True, yes let's add a default off environment variable and then we can merge it. Redocking like this is not always robust (on our property for example) due to orientation being only an estimate after undock. i.e. the mower is guessing a lot of state during a redock

This adds a new boolean to the idle state that tracks whether the mower is on the dock. If the mower detects that it is on the dock and it is no longer charging, it undocks and tries to redock.

This only applies to the cases where the mower has docked itself. If you manually place the mower on the charger this behavior will not activate.
@Apehaenger
Copy link
Collaborator

Now that re-docking is optional, I don't see a reason why not merging.

Quite thanks for your support with this PR as well as making our worries optional ;-) !

Once you found a solution for your difficulty of the edit-compile-debug loop, I personally would love to see an additional 1-inch approach, as @ClemensElflein and @gytisgreitai suggested ;-)

@Apehaenger Apehaenger merged commit 45f3d23 into ClemensElflein:main Jul 7, 2024
1 of 4 checks passed
rovo89 added a commit to rovo89/open_mower_ros that referenced this pull request Jul 8, 2024
These were missed in ClemensElflein#114. Especially the failure to reset emergency
mode got my mower stuck in the docking station. I hope I got the other
cases right as well.
@rovo89
Copy link
Contributor

rovo89 commented Jul 8, 2024

Some places didn't know about the new variant yet: #120

ClemensElflein pushed a commit that referenced this pull request Jul 8, 2024
These were missed in #114. Especially the failure to reset emergency
mode got my mower stuck in the docking station. I hope I got the other
cases right as well.
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.

5 participants