No retry docking #87
No retry docking #87
Conversation
|
Thank you for the PR, LGTM |
| @@ -203,8 +203,8 @@ Behavior *DockingBehavior::execute() { | |||
| return &IdleBehavior::INSTANCE; | |||
| } | |||
|
|
|||
| // Reset retryCount | |||
| reset(); | |||
| // Reset retryCount here would cause an infinite loop | |||
There was a problem hiding this comment.
@ClemensElflein @bodop Are you sure about this? If retry_count is 0 and the dummy docking approach point is not reachable then should never get here.
I don't think the regular docking behaviour logic should be changed to accommodate a special case such as this. Would be better to add a new "disable_docking" param and just immediately return IdleBehavior.
There was a problem hiding this comment.
As I see it, the change does not have anything to do with the PR, it's a general docking loop issue, if reset() is called in line 207, then retryCount is always 1 in line 222, so for non-zero retries, the thing would always retry.
There was a problem hiding this comment.
Usually I'd agree with the "lets not add some weird behavior for a specific use case"-statement, but making the parameter value '0' valid seems ok to me.
There was a problem hiding this comment.
ok but changing the retry logic for dock_straight? This has changed the number of dock_straight attempts from 1 + config.docking_retry_count to 1 + (config.docking_retry_count - approach_attempts).
Don't understand how this can cause an infinite loop if docking_retry_count is 0. It would try dock_straight once and fail to Idle.
There was a problem hiding this comment.
ah, missed the prev comment. Ok, so it's to stop a Docking -> Undocking -> Docking loop when retry count > 0
There was a problem hiding this comment.
Just replaces one issue with a different issue though. Now a bad approach can disable dock_straight retries. Maybe should count approach and dock_straight retries separately?
|
At least reset() in line 207 caused an infinite loop ignoring the number of configured retries. OK, I assumed, that approach_docking_point() always succeeds. Perhaps one should also add reset() after line 202. "disable_docking" would be a different approach. My approach has the advantage, that it is simple and ends in a defined end position. |
ah, so you want it to move to the approach point and stop? What if the docking approach point is in a different isolated area? |
|
If the approach point is in a different isolated area, the behavior is
identical to the one before this pull request. Only the number of
retries to reach an unreachable point may now be set to zero.
|
|
Ok, so just want to skip the dock_straight stage. Why not just add a param to explicitly disable this? Think you're correct about needing a reset() at 202 before returning to idle, plus the reset() at the end before returning to idle. |
My garden is so small, that the mower does not need to charge to complete the mowing area. So I decided to place the base station in an unreachable place and recorded a dummy docking location instead. Obviously docking will never succeed. So I created the possibility to configure no retries. In addition it turned out, that even one configured retry ended in an infinite loop.