Fix synched spindle moves exceeding linear axis limits #479
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…culation. The 2.x target velocity calculation in position-sync mode causes jitter around the target position. This commit replaces the error calculation with a simpler version that better tracks spindle velocity. Other changes: * Rename and refactor to be clearer about scope and purpose * Create a separate header for missing math functions from C math (signum and integer min / max). * Rename the signed spindle position helper function to something more meaningful Signed-off-by: Robert W. Ellenberg <[email protected]>
The original "error velocity" formula was correct if the initial and final velocity was zero. However, during spindle position tracking, the target velocity is non-zero (or it wouldn't be tracking!). This lead to a slight over-correction, and steady-state jitter even with a perfect encoder signal. The new formula accounts for the ideal target velocity, and should therefore over-correct less. This fixes the jitter (in simulation with a perfect encoder, at least). Signed-off-by: Robert W. Ellenberg <[email protected]>
…ating encoders) Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
Since Canon knows the spindle speed for a given segment, it's trival to calculate the nominal feed for a synchronized motion segment (assuming ideal spindle behavior). This nominal velocity is passed to motion so that blend arcs can be sized correctly. Also, report a canon error if the planner can't meet the required feed for spindle synchronization. Signed-off-by: Robert W. Ellenberg <[email protected]>
Fixes issue machinekit#68 by preventing any blending between position-synced motions (G33) and other motion modes (velocity-sync and normal), according to this table. Mode Transitions | blending allowed --------------------+----------------- normal -> position | no position -> normal | yes all others | yes These now cases match 2.6.x behavior, though the blending itself can be done with tangent / arc blends. Signed-off-by: Robert W. Ellenberg <[email protected]>
We know that feed override can't exceed 1.0 during position-sync moves, so it's a waste to plan blend arcs that allow for higher feed overrides. This makes blend arcs a bit smaller in position-synced moves. Signed-off-by: Robert W. Ellenberg <[email protected]>
Previously, the user could command an arbitrarily large spindle speed with G33 / G95 motion, even if the machine axes couldn't keep up. The user has no way of knowing that this is happening (except in extreme cases). This fix does two things: 1) Pop a warning message to the user telling them the maximum spindle speed possible for the current motion. 2) Attempt to limit the spindle speed by issuing a new speed command in the background before the synced motion starts. Note that (2) will have no effect if the machine does not have active spindle speed control. Signed-off-by: Robert W. Ellenberg <[email protected]>
Reduce spam in TP debug logs. Signed-off-by: Robert W. Ellenberg <[email protected]>
Parabolic blends require reduced max acceleration, so that the worst-case blend does not exceed the limits. If a parabolic blend can be "upgraded" to a tangent / arc blend, then we don't need this restriction. Previously, this check occured too early in the process of creating a new segment, which meant some segments ended up with reduced acceleration unnecessarily. Signed-off-by: Robert W. Ellenberg <[email protected]>
Mostly for troubleshooting, doesn't affect end-user performance Signed-off-by: Robert W. Ellenberg <[email protected]>
Instead of just low-pass filtering the measured speed, filter the spindle speed command sent to the simulated spindle. Now, both the position and velocity signals will simulate a spindle with inertia. Signed-off-by: Robert W. Ellenberg <[email protected]>
The spindle-speed-in pin will in many cases be a better estimate of velocity than the internal estimate we currently use in the TP. Encoder counters can have much finer time resolution than the servo thread, leading to smoother velocity estimates. One benefit is that the "sync_accel" phase actually works now. With a low-count encoder, the estimated velocity could sometimes be zero if there were no counts within one servo timestep (sam's test case shows this). Then, sync_accel ends early, and the position tracker is left with a large error to correct. Signed-off-by: Robert W. Ellenberg <[email protected]>
Like the PID component, motion can now estimate spindle velocity if the spindle-speed-in pin is left unconnected. motion computes a simple 2-point backward difference, which is usable at high speeds and with high PPR encoders. Thanks to Peter Wallace for this suggestion (and Jeff Epler for the original idea in pid.c). Signed-off-by: Robert W. Ellenberg <[email protected]>
Now, all spindle command settings are collected into one status struct. Similarly, spindle feedback statuses (position, velocity, etc.) are collected into a single struct. Note that this change is internal to motion. HAL pin names and externally-facing status fields should be unaffected. Signed-off-by: Robert W. Ellenberg <[email protected]>
Rigid tapping used to estimate spindle position both from the measured spindle revs, and the commanded direction. This approach is numerically dangerous, however, because the spindle position itself could be large. Therefore, flipping the sign on the position would cause an apparent jump. The previous approach to rigid tapping accounted for this. However, this new approach should be cleaner and simpler. Signed-off-by: Robert W. Ellenberg <[email protected]>
…ata. Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
…osition tracking should be. Currently, we correct position errors in spindle-sync as aggressively as possible, given machine acceleration limits. This new HAL pin lets the user control how aggressive it should be. The valid range is [0.0,1.0], where 0.0 means no position tracking (pure velocity). 1.0 is equivalent to 2.x position tracking. In some cases, reducing the gain will greatly shrink the acceleration jitter, without appreciably affecting position tracking performance. Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
This commit adds a new spindle tracking algorithm based on the trapezoidal motion profile. Particularly for high resolution encoders, this algorithm should have significantly reduced acceleration jitter. This also adds a (probably temporary) pin to HAL: motion.pos-tracking-mode: 0 (default) = new spindle tracking algorithm based on trapezoidal velocity calcs 1 = 2.7.x stock algorithm 2 = 2.7.x algorithm with a correction (here for completeness, but doesn't offer much over the trapezoidal method) Signed-off-by: Robert W. Ellenberg <[email protected]>
Signed-off-by: Robert W. Ellenberg <[email protected]>
…hed motion. Feed / speed synch is always disabled after a synced segment (even if it's then re-enabled for the next one). Unfortunately, this means that limiting spindle RPM, and then restoring it, has the effect of inserting two spindle speed commands between each segment of synched motion. This completely disrupts blending, and affects the acceleration used during the motion (potentially affecting threading performance). A safer and easier approach is to pop up a nuisance message to the user informing them that the spindle speed will be limited, and then just leave it at the limited speed. Generally, the user will set a new spindle speed for the next operation anyway. Signed-off-by: Robert W. Ellenberg <[email protected]>
In a spindle-synched G33 move, it is possible to specify a feed per revolution and spindle speed that requires linear motion exceeding axis velocity limits. This test sets up this kind of condition and measures actual pitch during the motion, failing if pitch is not within an epsilon value. See LCNC machinekit#167 for more discussion. LinuxCNC#167 Signed-off-by: John Morris <[email protected]>
During a spindle-synched move, too high a spindle speed may require an axis to move faster than it is able. This patch looks for that situation using programmed spindle speed and INI per-axis max velocity settings. See LinuxCNC issue machinekit#167 for more information. Signed-off-by: John Morris <[email protected]>
The test in LinuxCNC#180 fails because that issue is also dealing with feed per rev. This patch adapts Seb Kuzminsky's fix to Rob Ellenberg's "Enable smarter blending in spindle-synchronized motion" patch.
This was referenced Aug 2, 2018
|
There seems to be quite a collection of open issues around this. |
|
I haven't looked at this for a long time. Maybe @robEllenberg remembers this better? This has been in Tormach's tree since the PR was first opened, so it's had a few years of testing, albeit only in that single environment. |
|
There are some additional fixes that need to be backported before this can be merged into 2.7 (and those fixes also need to make it into 2.8). I'd like to hold off on the merge until I can straighten them out. |
|
any update on this? sounds like a good feature for the upcoming 2.8 release. |
|
Agreed, I think this is an important feature for 2.8. The branch as it stands needs some additional threading fixes backported. Depending on how that goes I'll either update the PR or make a new branch based on the current 2.8. |
|
Did this go in? I am suspecting not? |
|
As far as I know this didn't get merged. The last time I touched this I ran
into a bunch of merge conflicts trying to backport my changes from the
PathPilot fork, and simply haven't revisited this in a while. I still
believe that these changes would be a valuable improvement for 2.8. The
good news is that in the intervening time I've created a few test cases
(for runtests) to verify performance of spindle synchronization in sim, so
it will make it easier to verify that it's working as expected. If I can
lean on a few people with hardware to put the new code through its paces
(e.g. Gene), then we can make some progress here.
…On Sun, Jan 17, 2021 at 3:45 PM andypugh ***@***.***> wrote:
Did this go in? I am suspecting not?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#479 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJHUPFUICHMJBDEJPZYOSDS2ND77ANCNFSM4FNUUXUQ>
.
|
|
Looking at the merge conflicts reported here in the web editor it doesn't look too hard, but I think that a rebase on to 2.9 would be needed. And then all the multi-spindle stuff is likely to cause trouble. |
| timeout = 5 | ||
| avg_sum = 0; avg_count = 0 | ||
| while timeout > 0: | ||
| if abs(h['pitch']) > 0.001: |
There was a problem hiding this comment.
I tried porting the spindlesync-exceeds-maxvel test to the latest master, and ran into a problem with h['pitch'] never being above 0.001. Where is it supposed to be set?
|
https://github.com/petterreinholdtsen/linuxcnc/tree/479-test-spindlesync-exceeds-maxvel-on-master is my port of the test to python 3 and upgrade of the ini file to get linuxcnc to accept it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See "Warn users that spindle is too fast in G33 / G95" #167.
This is @robEllenberg's work, when too high spindle speed during spindle-synched move would exceed linear axis limits, at the time of the motion, give a warning and attempt to reduce spindle speed.
On top, I added an interp-side warning, shown at program start time, and a regression test.
Issue #167 was pointed out in related issue #354.