Added(optional) option-flags to the moveTo command.

[doppelgrau]

Description

The moveTo() command in the Movable-interface (and all classes implementing it) now accepts optional flags, that are described in the enum "MoveToOption".

In the GcodeDriver these options are evaluated.

Justification

This allows other functions selectively to disable certain features, that usually improves the movement quality, but might not suitable in this situation.
Examples:

• backslash compensation can create mechanical stress during nozzle change (pressing into the nozzle tip holder)
• small movements without the need for precision are way slower with backslash compensation. (e.g. "stirring in a heap feeder)

Instructions for Use

only used by developers.

Implementation Details

1. How did you test the change? Be descriptive. Untested code will not be accepted.

Tested it in the simulator. Larger tests following with the HeapFeeder implementation.

2. Did you follow the coding style?

yes

3. If you made changes in the org.openpnp.spi or org.openpnp.model packages you will need to add additional justification for these changes. Changes to these packages require extensive review and testing.

yes

4. Be sure to run mvn test before submitting the Pull Request. If the tests do not pass the Pull Request will not be accepted.

passes.

Recommend to wait a bit until the heap feeder exist as a first "User" so there have been more testing.

[markmaker]

Hi @doppelgrau

I had the same reservations about Backlash Compensation until I realized this:

1. If the nozzle tip changer is a) aligned with the axes and b) open towards the backlash compensation offset, then there is no such stress. You can reverse the sign of your backlash compensation offset if needed.
2. If you have Squareness Compensation, then align the tip changer openings with the X axis, so it never needs to move in Y, i.e. so Squareness Compensation is never used (I'm just guessing that's the reason for your other flag).
3. I believe that if backlash compensation is properly tuned, then there is virtually no physical movement and no stress. Backlash compensation should ideally only take up the slack in the mechanics (belt, pulleys etc.). It would be nice to calibrate this using vision (and I think its quite easy to implement).

Having said that, I agree, that the system needs to become smarter, see here:
https://groups.google.com/forum/#!topic/openpnp/bEVZvYoXO98
So I'm open for new ideas, hopefully integrating with these new plans.

One goal of these plans is to take universal application logic out of the GCodeDriver and make it available to all the drivers (including future ones). So new PRs should whenever possible not add new features exclusively to GCodeDriver.

The new plans (see the link for details) will change the moveTo() on the driver significantly. For instance the axes will already have been transformed (including squareness compensation) and the mapped axes "package" will be passed to the driver moveTo(). The driver moveTo() should only be concerned with the raw motion.

The PR does not show, how this will be used from the caller. So it's not possible to assess the dependencies and how easily it could be adapted in the future, e.g. to be compatible with the new plans. So I hope you understand, I'm a bit concerned.

Do you see a path?

_Mark

[doppelgrau]

hi @markmaker

I had s short discussion with Jason about this PR.
Will move the enum to the Movable interface and make the entries more generic (NO_ADDITIONAL_MOVE and RAW - suggestions for better names happily accepted. Guess RAW is ok, but the first one could also something like SPEED_OVER_PRECISION or or).
Then test it with the HeapFeeder implementation. But I guess moving the options to Movable and passing it along the "chain" might make changing the logic easier.

[markmaker]

Hi @doppelgrau

Yes, that's much better.

But now I start to wonder...

Could this be made into a real object instead of the enum? So we can create new subclasses and add new parameters, without each time breaking the interface? (Note, I'm not quite familiar with Java enums, perhaps they are more powerful and already allow subclassing?)

I also wonder if the waitForMoveToComplete() could be made into just another flag (property) of that parameter object. So if you wanted to move the camera to take a vision capture, you'd just add the "wait" flag and the "precision" flag and perhaps the "antivibration" flag (hope you read my post and understand what I'm talking about ;-).

https://makr.zone/wp-content/uploads/2020/04/SimulatedJerkControl.mp4

_Mark

[doppelgrau]

Yes, a real objects would also be possible. but since you can pass as many enum as you want, you can just make the enum longer without breaking anything.
See it as a a way to pass an arbitrary number of boolean flags around. (is passed or not)

So yes, something like "WAIT_FOR_COMPLETION", "LOW_VIBRATION" would be possible, passed around and every method along the "path" can react if it is set. (e.g. GCodeDriver and ReferenceNozzle. In the nozzle only on the RAW enum something is done. And you can see the loop, since the number can be arbitrary)

[vonnieda]

This looks good to me, and I think it's a good first step that will not really get in the way of @markmaker's larger changes.

I have two concerns:

1. OpenPnP uses CamelCase for enums throughout the system, so I'd like the names changed from e.g. NO_ADDITIONAL_MOVE to NoAdditionalMoves.

2. At first, I thought Mark was incorrect about backlash compensation (BC) but I just worked though an example and now I agree with him. BC should not actually cause an additional move if set correctly.

So, for the two use cases you described in the PR, I think maybe the first is only an issue if BC is not tuned. The second makes sense, but maybe should be renamed, as you said, to something like SpeedOverPrecision.

Either way, since these are strongly typed, it's a single click to rename them later on, so I'm also fine with just merging this (after changing to CamelCase, at least).

Thanks,
Jason

[doppelgrau]

Changed to CamelCase and also made the second enum entry name more generic. (SpeedOverPrecision)

[markmaker]

Hi @vonnieda / @doppelgrau

Looking closer, I'm not so happy with the RawMove part of it.

It begins with the term "raw" that is already taken and means raw controller coordinates. That's not the same! There can be a NegatingTransform between those two "raw".

When suppressing the nozzle tip calibration in moveTo() we need to keep in mind that the reverse runout compensation happens in getLocation(). But it is not suppressed there. Calling Code including any Nozzle sub-class would need to be careful not to get an "inverted runout compensation effect". That could be triggered through NaN substitution or any Location.derive() both in the actual as well as in subsequent moves. Yes it will probably often work as intended but there is a booby trap in there.

In short: A moveTo() should always do what its told and be consistent with any subsequent getLocation().

Just to be clear, SpeedOverPrecision is fine, because it does not affect the semantics of the move, just the physical implementation (e.g. no backlash compensation). If you call getLocation() after that, the result is still the best guess of that location.

But why not turn the tables? One solution would be to add another set of options to getLocation().

org.openpnp.spi.Locatable.getLocation(LocationOption...)

So you yould get an answer to the following question:

"To what location do I need to send the nozzle, if I don't want compensation moves in X/Y to happen, even if I want this new rotation?" Only the calling code would then need to know what they are doing, the rigged location stays outside the stack. Consistency guaranteed.

The same could be done for Non-Squareness Compensation:

"To what location do I need to send the nozzle, if I don't want Non-Squareness Compensation moves (in X) to happen, even if I want this new Y coordinate?".

If you're OK with that, I'll add this to my upcoming PR. As it happens, I already had to lay the foundation of that.

_Mark

[doppelgrau]

Hi Mark,

I'm fine with every solution. I'm not (yet) deep enough in the architecture to decide where is the best place to put these options.
Guess @vonnieda is the best to decide.

greetings
Johannes

[vonnieda]

@markmaker Before you spend time coding this, tap the brakes. I don't want to add more API for a feature that is extremely fringe.

@doppelgrau: Do you agree that your point #1 is not actually an issue if backlash compensation is tuned properly? If so, we can just drop RawMove, right?

And for point #2, the problem is that you need to make frequent small moves in your feeder implementation and backlash compensation makes these slow. How slow are we talking about, and is it possibly because backlash compensation is not tuned enough?

My thinking here is that we're adding an awful lot of API and complexity for a feature that is only used by one unreleased feeder implementation, and I wonder if it might make sense to think harder about how to solve the problems in the feeder, rather than changing all of OpenPnP to support the feeder.

[doppelgrau]

#1: yes, raw move can be dropped.

#2:
Don't know if my backslash compensation is already "probably tuned" (distance and speed), but since currently the speed is relatively low for backshlash compensation my guess would be, that nearly doubles the duration of a feeder motion. On the other hand running the first experiments now: after the "initial part level" has been reached (that can be painful slow, so there the longer duration would hurt a bit, on the other hand the user can reduce that by making a guess for the initial feed depth), the time spend in the heap is lower than expected from the "old" code and videos. Most of the time is vision and travel.

So if that creates a headache in the API to support it cleanly, throw it away. The HeapFeeder will never be a really fast feeder and the speed increase with these options is lower than expected.

[markmaker]

Hi @vonnieda
#1 is not for backlash compensation but for runout compensation. That's the problem.

#2 suppressing backlash compensation is not a problem.

If have this now in Moveable:

    /**
     * Contains all possible options for getting approximative locations.
     * KeepX, KeepY, KeepZ, KeepRotation: keep these raw coordinates the same. 
     * SuppressCompensation: calculate a location that will result in no extra moves for compensation.
     */
    public enum LocationOption { KeepX, KeepY, KeepZ, KeepRotation, SuppressCompensation }

    /**
     * Get an approximative Location in order to avoid extra compensation moves. 
     * 
     * @param currentLocation The current location, usually obtained using Headmountable.getLocation()
     * @param desiredLocation The desired location to approximate. 
     * @param options Options for the approximation.
     * @return
     */
    Location getApproximativeLocation(Location currentLocation, Location desiredLocation, LocationOption... options);

This does make sense in order to bypass squareness compensation, runout compensation, etc.

Keeping the axes still also means there will be no backlash compensation. So if done cleverly this can be used to calculate moves and paths that have full precision and still avoid extra moves.

_Mark

[vonnieda]

Hi @markmaker - I feel a little bit lost as to how this will all work together, but again, I trust that you will do it right. I'm not close enough to this code right now to really grok it. It sounds like this solves the issue you identified and keeps @doppelgrau's ability to have fast moves, so sounds like win win to me.