Conversation
Why --- Spindle synchronized motion always starts at the spindle index pulse, so every threading pass begins at the same angular position. Cutting a multi-start thread means starting successive passes at a fixed angle past the index instead, and there was no way to ask for that. D on G33 and G76 now gives that start angle in degrees. The synchronized move is held at rest after the index until the spindle has turned through it. D is optional and defaults to zero, which is the existing behaviour of starting at the index pulse; a two-start thread is cut by running the program twice, with D0 and D180. What changed ------------ Interpreter: interp_check.cc accepts D on G33 and G76, which previously rejected it as "D word with no G41 ... to use it". convert_straight() and convert_threading_cycle() read it and pass it to the canon layer. A negative D is taken as its magnitude rather than rejected -- the value is a delay past the index and has no direction. Canon and NML: START_SPEED_FEED_SYNCH() gains an angle_degrees parameter, defaulted to 0.0 so the interface and its non-task implementations stay source compatible. EMC_TRAJ_SET_SPINDLESYNC carries angular_offset_degrees, which reaches the planner through emcTrajSetSpindleSync() and EMCMOT_SET_SPINDLESYNC. Planner: tpSetSpindleSync() converts degrees to revolutions once and stores it in tp->spindle.pending_offset; tcSetupState() copies that into each TC as it is queued, so the passes of a G76 cycle each carry their own value rather than reading whatever the TP holds when they run. The hold itself is in tpSyncPositionMode(): spindle.revs resets to 0 at the index pulse, so while it is below the requested offset the move is held at target_vel 0. On reaching the angle, spindle.offset is set to the current revs so position mode starts tracking from zero error there. tpCheckAtSpeed() skips the sync_accel ramp when an offset is requested, because that ramp exists to catch the axis up to an already-turning spindle, which is what the hold is there to prevent. The hold is scoped to one index pulse by tp->spindle.angle_hold_pending, armed where the index wait is armed and cleared once the angle is reached. This matters for G76 with an entry taper: such a pass emits three synchronized moves, all carrying the same angle_offset, and only the first of them follows an index pulse. Without the flag the later segments also reset spindle.offset, discarding the value tpCompleteSegment() accumulates and shifting the thread phase part way through a pass. Compatibility: the canon signature change is a break in the public interface tests/interp/compile guards, so use-rs274.cc is updated -- per that test's README this is noted deliberately. canonmodule.cc spells out its argument list so the C++ default survives into Python; a bare function pointer would have made the new argument mandatory and broken every existing three-argument caller. Docs: the D word is documented for both G33 and G76, including that it defaults to zero, that a negative value is used as its magnitude, and that values of 360 or more are not reduced. The letter table in overview.adoc gains the second meaning of D. A note under G33 Technical Info records that a D offset holds the axis at rest instead of using the usual post-index acceleration compensation. Testing ------- tests/interp/g76-d-word (new) runs the interpreter over G33 and G76 with no D, with D, and with a negative D, and checks what reaches the canon layer. saicanon prints the angle only when one was asked for, so the expected output of the existing g33.1 and g76 tests is unchanged. tests/motion/spindle-angle-offset (new) runs a sim machine with sim_spindle, whose index pulse resets spindle.0.revs -- the same reset D is measured against. It samples Z, X, revs and the index pin every servo period and checks that G33 without D starts at the index, that D180 waits half a turn longer, that D-180 behaves identically to D180, that a G76 cycle with D180 starts half a turn later than an otherwise identical cycle without it, and that both G76 cycles still cut the programmed pitch. Each angle check is a difference between two passes, because detecting that the tool has started moving costs a fixed amount of rotation that only cancels that way. The new test was confirmed to fail when the hold is disabled. tests/interp and tests/motion pass in full, 90 tests, with tests/interp/compile skipped as it already was. Not covered by the tests ------------------------ The segment scoping described above is not caught by any automated test. It was confirmed by restoring the unscoped version and rerunning: the test still passes, because core_sim.hal feeds motor-pos-cmd straight back as motor-pos-fb, so the simulated axis tracks perfectly and the discrepancy the fault feeds on is essentially zero. Reproducing it needs a machine, or a sim with a servo model that has real following error. Reviewers with a lathe are asked to look at a G76 pass with an entry taper (L1 or L3) and a D word. tests/interp/compile is still disabled and still does not link, for the unrelated and pre-existing reason that it is also missing SET_MOTION_CONTROL_MODE. Only the START_SPEED_FEED_SYNCH signature was brought up to date here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Suggest holding this until #4453 and #4441 are in, then rebasing. All three touch the same G33/G76 spindle-sync code. #4453 rewrites the sync-origin handoff in tpSyncPositionMode and the G33 Technical Info paragraph this PR also edits; #4441 overlaps in tpSetSpindleSync, tp_types.h, convert_straight (G33 block) and convert_threading_cycle. The conflicts are mechanical and the semantics compose cleanly (the D-hold re-anchors with the axis at rest, so no lag is folded in; the overrun fault and pitch check are unaffected), but one rebase after both land is less churn than resolving piecemeal. |
This file contains hidden or 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
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.
Why
Spindle synchronized motion always starts at the spindle index pulse, so every threading pass begins at the same angular position. Cutting a multi-start thread means starting successive passes at a fixed angle past the index instead, and there was no way to ask for that.
D on G33 and G76 now gives that start angle in degrees. The synchronized move is held at rest after the index until the spindle has turned through it. D is optional and defaults to zero, which is the existing behaviour of starting at the index pulse; a two-start thread is cut by running the program twice, with D0 and D180.
What changed
Interpreter: interp_check.cc accepts D on G33 and G76, which previously rejected it as "D word with no G41 ... to use it". convert_straight() and convert_threading_cycle() read it and pass it to the canon layer. A negative D is taken as its magnitude rather than rejected -- the value is a delay past the index and has no direction.
Canon and NML: START_SPEED_FEED_SYNCH() gains an angle_degrees parameter, defaulted to 0.0 so the interface and its non-task implementations stay source compatible. EMC_TRAJ_SET_SPINDLESYNC carries angular_offset_degrees, which reaches the planner through emcTrajSetSpindleSync() and EMCMOT_SET_SPINDLESYNC.
Planner: tpSetSpindleSync() converts degrees to revolutions once and stores it in tp->spindle.pending_offset; tcSetupState() copies that into each TC as it is queued, so the passes of a G76 cycle each carry their own value rather than reading whatever the TP holds when they run. The hold itself is in tpSyncPositionMode(): spindle.revs resets to 0 at the index pulse, so while it is below the requested offset the move is held at target_vel 0. On reaching the angle, spindle.offset is set to the current revs so position mode starts tracking from zero error there. tpCheckAtSpeed() skips the sync_accel ramp when an offset is requested, because that ramp exists to catch the axis up to an already-turning spindle, which is what the hold is there to prevent.
The hold is scoped to one index pulse by tp->spindle.angle_hold_pending, armed where the index wait is armed and cleared once the angle is reached. This matters for G76 with an entry taper: such a pass emits three synchronized moves, all carrying the same angle_offset, and only the first of them follows an index pulse. Without the flag the later segments also reset spindle.offset, discarding the value tpCompleteSegment() accumulates and shifting the thread phase part way through a pass.
Compatibility: the canon signature change is a break in the public interface tests/interp/compile guards, so use-rs274.cc is updated -- per that test's README this is noted deliberately. canonmodule.cc spells out its argument list so the C++ default survives into Python; a bare function pointer would have made the new argument mandatory and broken every existing three-argument caller.
Docs: the D word is documented for both G33 and G76, including that it defaults to zero, that a negative value is used as its magnitude, and that values of 360 or more are not reduced. The letter table in overview.adoc gains the second meaning of D. A note under G33 Technical Info records that a D offset holds the axis at rest instead of using the usual post-index acceleration compensation.
Testing
tests/interp/g76-d-word (new) runs the interpreter over G33 and G76 with no D, with D, and with a negative D, and checks what reaches the canon layer. saicanon prints the angle only when one was asked for, so the expected output of the existing g33.1 and g76 tests is unchanged.
tests/motion/spindle-angle-offset (new) runs a sim machine with sim_spindle, whose index pulse resets spindle.0.revs -- the same reset D is measured against. It samples Z, X, revs and the index pin every servo period and checks that G33 without D starts at the index, that D180 waits half a turn longer, that D-180 behaves identically to D180, that a G76 cycle with D180 starts half a turn later than an otherwise identical cycle without it, and that both G76 cycles still cut the programmed pitch. Each angle check is a difference between two passes, because detecting that the tool has started moving costs a fixed amount of rotation that only cancels that way.
The new test was confirmed to fail when the hold is disabled. tests/interp and tests/motion pass in full, 90 tests, with tests/interp/compile skipped as it already was.
Not covered by the tests
The segment scoping described above is not caught by any automated test. It was confirmed by restoring the unscoped version and rerunning: the test still passes, because core_sim.hal feeds motor-pos-cmd straight back as motor-pos-fb, so the simulated axis tracks perfectly and the discrepancy the fault feeds on is essentially zero. Reproducing it needs a machine, or a sim with a servo model that has real following error. Reviewers with a lathe are asked to look at a G76 pass with an entry taper (L1 or L3) and a D word.
tests/interp/compile is still disabled and still does not link, for the unrelated and pre-existing reason that it is also missing SET_MOTION_CONTROL_MODE. Only the START_SPEED_FEED_SYNCH signature was brought up to date here.