yujinrobot / kobuki_core

Core (non-ros) kobuki packages.
68 stars 71 forks source link

[dock_drive] fix logic on detecting dock state #55

Closed stonier closed 4 years ago

stonier commented 4 years ago

To cover the case when it equals zero.

Originally motivated by #26 and #32. Logic looks fine, but am unable to test locally since I do not have access to a docking station, so relying on anecdotal evidence from both @molinadavid and @alexthegreat63. Will be easy to unwind if there are further issues.

Thanks @molinadavid and @alexthegreat63!

stonier commented 4 years ago

I don't think you have an overload on dock_detector between 'uninitialised' and 'directly aligned with centre'. That variable is only ever reset to zero whenever you trigger another scan.

What dock_detector does have problems with is that it might come out to be zero if there are equal detections left or right as it rotates around a full circle without detecting the centre signal. Then you might have the robot doing entirely the wrong thing in the find_stream method.

As it is, most times you're only going to get there having detected nothing with the middle sensor. If there's nothing on the left/right sensors, you're going to be stuck in find_stream. Similarly, there exist chances to be stuck in get_stream, which is worse, because that will just propel kobuki forward indefinitely. This has all the characterful hallmarks of something being programmed for success, not for failure :/

This PR at least resolves the crashes. I'm not about to do an extensive overhaul on this today though, but we should probably have an issue floating around documenting these concerns.


Some interesting history. The original cleaning robot that kobuki derived from has a robust, very optimal algorithm. Kobuki, as you can see, barely has a stub for a docking algorithm. The lads decided that extracting the original algorithm from the embedded software in the cleaning robot was going to be a large task - lots of code, branches and some parts of it tuned to the cleaning robot use case (gleaned information from the map to restrict starting locations). So, the stub. There was a starry eyed hope that the open source community would contribute something, but also a thought that the algorithm is pretty tuned to the use case. This stub, as a reference, would do.

We later developed much better non-embedded algorithms for other robots. Unfortunately they never made it back to the open source world :(

stonier commented 4 years ago

Don't have an underlying executable - I think they used the ROS action to test it. Might be the thing to do is let this PR stew until we get that part of it functional again.

stonier commented 4 years ago

Pulling in, we can make bugfixes as we go if necessary. Created an issue at https://github.com/kobuki-base/kobuki_core/issues/14.