ClemensElflein / open_mower_ros

Other
498 stars 122 forks source link

Retry docking if the mower rolls off the charger #114

Closed jeremysalwen closed 1 month ago

jeremysalwen commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

Apehaenger commented 1 month ago

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 ;-)

rovo89 commented 1 month ago

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