Fix infinite loop in reconcilePosition

[bjj]

Between Matt's change to make the control panel position update more reliably when homing and my change to implement reconcilePosition (with M114) in RepRap5d we produced an infinite loop whenever anything tried to call getCurrentPosition() in RepRap5d. I relied on the atomic update nature of the old code to return "anything" after the M114 result arrived asynchronously. After the atomic update was changed an attempt was made to return "old" position for "no change" but that caused an infinite loop.

[cibomahto]

I made some changes here too, can you check if this merge still works for you? https://github.com/makerbot/ReplicatorG/commit/8c11f9b1fbbfb81b22738cd422a03e88c552d22e

The major change is that we don't transition from a null position to a 0 position unless forceUdate is specified. Let me know if that still works for you.

[bjj]

I've rewritten this comment a bunch of times but I am going to keep it short: This diff doesn't work. Can we just go back to what I wrote and tested?

[bjj]

My last comment sounds pretty snippy, but that wasn't my intent. I really did start several detailed responses to the diff but I kept getting interrupted at work. What I meant to say was that my change worked, the new behaviors in the modified patch do not appear to be necessary and create new problems, so can we go back to my patch or do we need further discussion to merge in the goals you were trying to accomplish? ... then I got interrupted and sent the snippy-looking version.

[cibomahto]

The problem is that gen3/4 based devices need to know if the position is actually invalid, so that they can calculate relative motions correctly (we normally use a time-based motion command, but fall back to a feedrate-based command in instances where we don't know our current position and can't ask the machine for it).

I think the easiest thing to do here is just overload the getCurrentPosition() function to provide the behavior 5d expects; would that work? Possible implementation here: https://github.com/makerbot/ReplicatorG/tree/bjj_5dfix

The bigger issue, i think, is that none of these commands or their side effects are documented, so it's essentially impossible to make changes without breaking things :-(.

[bjj]

I still don't understand what you are trying to do.

My change did two things:

1) Rename "update" to "forceUpdate" to clarify the apparent meaning (since it causes a firmware level update even if we think our position is valid)

2) Made it possible for reconcilePosition() to return "null" to mean "no change". Previously no driver even did this. If a driver returns null and the position is invalid it becomes 0. This case never existed before and is only reachable in the RepRap5d driver.

I can see an argument to modify (2) to merely return 0 if the position is invalid and the driver returns null (leaving it invalid). That would mean that there's a driver that can't get the position now but could later.

Your modified code is very strange:

getCurrentPosition(false): If position is invalid, reconcile. if new point is null return 0 (position remains invalid). Fine. getCurrentPosition(true): Call reconcile. If it returns null, invalidate the position (even though I meant null to mean "no change"), then crash (call new Point5d(null))

It's a radical change from what I was trying to accomplish. It breaks RepRap5d while having no effect on any other driver. What was it supposed to do?

[cibomahto]

Hi Ben,

I certainly don't want to break RepRap5d. The side effect I was trying to fix is what you mentioned in 2. The Makerbot4GAlternateDriver is depending on the cached machine position to remain null until the machine is put into a known position. I see now that my patch didn't work the way i intended- does this make more sense: https://github.com/makerbot/ReplicatorG/commit/67ae7bd9a9777cd007bd320082dd676d11acd6a8

[bjj]

The Makerbot4GAlternateDriver is depending on the cached machine position to remain null

Oh, I see, you and I both added "return null from reconcilePosition()" at about the same time, but I made it mean "do nothing" and you made it mean "invalidate position". At the time I made my change the most current makerbot tree had no other drivers returning null so I thought it was safe to assign it a meaning.

I also see now that you interpret "forceUpdate" to mean "invalidatePosition + getCurrentPosition". I didn't interpret it that way because there are some RepRap5D that don't implement M114 so it's fine to try reconcile but it would be best to keep the offset position so you can at least identify relative moves during a control panel session.

And finally, the latest patch won't work because RepRap5D always returns null and the current code will always invalidate the position. It always returns null because the update happens in another thread (the reply thread). Synchronization may end up re-ordering these but I wouldn't like to rely on that.

If you are happy with in DriverBaseImplementation and then I will submit a new diff that makes RepRap5d work by only modifying RepRap5d. It looks like currentPosition is merely "protected" so I can restructure reconcilePosition() so it always returns position (rather that setting out of band) and returns currentPosition.get()

[cibomahto]

Oh, I see, you and I both added "return null from reconcilePosition()" at about the same time, but I made it mean "do nothing" and you made it mean "invalidate position". At the time I made my change the most current makerbot tree had no other drivers returning null so I thought it was safe to assign it a meaning.

Gotcha. My apologies about having nothing in the tree, I've been working out of a badly-named branch (home_positioning) for a while, and my changes weren't reflected in master.

I also see now that you interpret "forceUpdate" to mean "invalidatePosition + getCurrentPosition". I didn't interpret it that way because there are some RepRap5D that don't implement M114 so it's fine to try reconcile but it would be best to keep the offset position so you can at least identify relative moves during a control panel session.

And finally, the latest patch won't work because RepRap5D always returns null and the current code will always invalidate the position. It always returns null because the update happens in another thread (the reply thread). Synchronization may end up re-ordering these but I wouldn't like to rely on that.

If you are happy with in DriverBaseImplementation and then I will submit a new diff that makes RepRap5d work by only modifying RepRap5d. It looks like currentPosition is merely "protected" so I can restructure reconcilePosition() so it always returns position (rather that setting out of band) and returns currentPosition.get()

That sounds good to me!