Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes for issues 68, 164, 167, 546, and 550 #581

Open
wants to merge 131 commits into
base: 2.7
Choose a base branch
from
Open

Fixes for issues 68, 164, 167, 546, and 550 #581

wants to merge 131 commits into from

Conversation

robEllenberg
Copy link
Collaborator

@robEllenberg robEllenberg commented May 2, 2019 •
edited
Loading

This branch combines a bunch of old work, and some recent fixes from other branches to address some outstanding TP issues in 2.7

Fix #68 (reduce large blend arcs in G33 and prevent blends between synced and non-synced modes)
Fix #164 (smoother error correction and better velocity estimation in position sync)
Fix #167 (warns users if axis feeds are not fast enough for a G33 move at the current spindle speed)
Fix #546 (Fix some edge cases in TP including a regression in parabolic blend fallback)
Fix #550 (Fix mismatch between canon and TP's idea of what a zero-length move is, since they each have to arrive at that conclusion independently)

Note: all runtests and TP unit tests pass in a Ubuntu 18.04 x64 uspace build

robEllenberg added 30 commits May 1, 2019 18:31
@robEllenberg
tp: Improve position-mode spindle sync performance with new error cal…
89d05c4 
…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]>
@robEllenberg
tp: new formula for spindle position tracking
300045b 
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]>
@robEllenberg
sim: Add a simple component for quantizing a signal (useful for simul…
e76bb40 
…ating encoders)

Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
sim: First cut of spindle encoder simulation with quantized output
a95fee6 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
canon: Add a missing forward declaration
fb6b108 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
canon: Minor whitespace cleanup
ddd71e4 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
canon: Replace unit conversion macro with a function
050adc6 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
canon: Enable smarter blending in spindle-synchronized motion.
150751e 
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]>
@robEllenberg
tp: disallow blending when entering position-synch mode
132d8cc 
Fixes issue LinuxCNC#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]>
@robEllenberg
tp: Limit max feed scale in position-sync blends.
09c5949 
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]>
@robEllenberg
canon: Automatically limit spindle speed in spindle-sync motion.
68b5e73 
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]>
@robEllenberg
tp: Fix debug level for some debug print statements
6082892 
Reduce spam in TP debug logs.

Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
tp: Re-order checks for parabolic blends
f714b3d 
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]>
@robEllenberg
tp: Add debug information for spindle sync
a10bd0b 
Mostly for troubleshooting, doesn't affect end-user performance

Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
sim: Improve simulation of inertia in spindle sim
3bcda2a 
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]>
@robEllenberg
tp: Use motion's spindle-speed-in input for spindle velocity
8c4c0a0 
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]>
@robEllenberg
motion: Add spindle velocity estimator
ae402de 
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]>
@robEllenberg
motion: Refactor spindle status for clarity and organization.
873aac4 
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]>
@robEllenberg
tp: Remove discontinuous spindle positions in rigid tapping internals.
f8af6dc 
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]>
@robEllenberg
tp: Fix spindle position tracking to work with motion's new spindle d…
5390ee4 
…ata.

Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
canon: actually return angular feed rate when asked
c4dfe6f 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
test: Test case for steady state error
aa53d9f 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
test: Match sam's lathe config settings (axis vel / acc).
7b78fbc 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
Re-arrange a calculation to remove a division
a7e35f5 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
motion: Add a HAL pin to motion to let users control how aggressive p…
39f7434 
…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]>
@robEllenberg
sim: model velocity quantization in encoder near 360RPM
4864c45 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
tp: Allow switching between position-sync algorithms based on a HAL pin.
f7a654c 
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]>
@robEllenberg
canon: Simplify spindle speed limits for synced motion
dbc4be1 
Signed-off-by: Robert W. Ellenberg <[email protected]>
@robEllenberg
canon: don't restore spindle speed after limiting it for spindle-sync…
6ef4c5e 
…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]>
@robEllenberg
Added tracking error pin to motion HAL for spindle sync testing
2446097 
Signed-off-by: Robert W. Ellenberg <[email protected]>
robEllenberg and others added 8 commits May 1, 2019 18:31
@robEllenberg
unit_test: Hide pedantic debug ouput by default in unit tests
5a815e0
@robEllenberg
Merge branch 'bugfixes-546-550-167-68-164-2.7' into 2.7
a77ea52
@robEllenberg
tp: fix missing symbol
b515f12
@robEllenberg
Fix runtests for TP consistency checks
24e6a35
@zultron @robEllenberg
canon: fix feed rate problem in arc corner case
dd22d36 
When an arc is 360 degrees, the endpoint and the starting point are
the same.  Naively calculating the XYZ movement using only the
difference between the endpoints overlooks XYZ motion.  When such a
move is combined with a rotary axis movement, the resulting feed rate
will be incorrect.

This patch fixes the problem by passing a parameter to the function
calculating that distance that forces XYZ movement to be recognized.

This may not address the entire issue, since later code in that
function makes further use of the endpoint differences.
@robEllenberg
Fix formatting inconsistencies in canon
0d08b10
@robEllenberg
canon: fix a merge / rebase error that dropped a variable rename in a…
fa8c17d 
… previous commit
@zultron @robEllenberg
Fixup 80c9258: update motion-logger/basic test
347a6b5 
Update motion-logger/basic test after changing status buffer fields in
80c9258, "Add spindle-specific fields to emcmotCommand rather than
reusing other fields (with confusing names)."
@samcoinc
Copy link
Collaborator

samcoinc commented May 2, 2019 via email

@samcoinc
Copy link
Collaborator

samcoinc commented May 2, 2019 via email

@samcoinc
Copy link
Collaborator

samcoinc commented May 2, 2019 via email

@robEllenberg
Copy link
Collaborator Author

robEllenberg commented May 2, 2019 via email

@robEllenberg robEllenberg deleted the rellenberg/bugfixes-546-550-167-68-164-for-2.7 branch June 14, 2019 13:17
@robEllenberg robEllenberg restored the rellenberg/bugfixes-546-550-167-68-164-for-2.7 branch June 14, 2019 15:07
@mozmck
Copy link
Collaborator

mozmck commented Jun 9, 2020

@robEllenberg what is the status of these fixes? Have they been merged somewhere? I have a problem on 2.7 with a pause in motion on an ellipse and I'm wondering if a fix for this is somewhere in these fixes.

@robEllenberg
Copy link
Collaborator Author

robEllenberg commented Jun 14, 2020 •
edited
Loading

I opened a pull request for #68 and backported the fix for #704 in another PR for 2.7. I need to pull out the blending fix from this branch and merge it separately because the spindle-sync stuff in the branch is a little much for a point release in 2.7.

@robEllenberg robEllenberg reopened this Jun 14, 2020
@petterreinholdtsen
Copy link
Collaborator

Is there any chance that this huge collection of independent changes can be split up into individual change sets that are easier to review, to increase the chance of these fixes making it into the master branch?

@petterreinholdtsen petterreinholdtsen mentioned this pull request Aug 20, 2022
Open
@petterreinholdtsen
Copy link
Collaborator

Rene has volunteered to try to figure out this patch.

@petterreinholdtsen
Copy link
Collaborator

@rene-dev, did you have any luck figuring out this patch?

@petterreinholdtsen
Copy link
Collaborator

Any news here?

@andypugh andypugh self-assigned this Jan 18, 2023
@mozmck mozmck mentioned this pull request Mar 13, 2023
Open
@SebKuzminsky
Copy link
Collaborator

SebKuzminsky commented Apr 23, 2023 •
edited
Loading

#550 was fixed in 2.7 by @robEllenberg here: 10c3678

#68 and #546 are both fixed in 2.7.

Only #164 and #167 remain to be examined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rene-dev rene-dev Awaiting requested review from rene-dev

Assignees

@andypugh andypugh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@robEllenberg @samcoinc @mozmck @petterreinholdtsen @SebKuzminsky @andypugh @zultron