Repository navigation
Conversation
…CNC#3627) program_units was only written by canon USE_LENGTH_UNITS, when the interpreter read a G20/G21 -- often far ahead of motion -- while gcodes reports the executing move. emcTaskUpdate now sets programUnits from the active G20/G21 it has just computed. Canon's write stays for interpreters that report no G20/G21 (canterp). tests/program-units runs a 5 s G21 move followed by G20 on simulated motion and fails if program_units disagrees with gcodes during the move. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
The change looks right to me and fixes #3627: program_units now comes from the same active G20/G21 as gcodes, and nothing in task or canon reads programUnits back, so overwriting canon's read-ahead value is safe. Three timing points in the test below; @hdiethelm's parallel runs (#4669) turned up several tests that failed under load on exactly these patterns.
|
|
||
| c.state(linuxcnc.STATE_ESTOP_RESET) | ||
| c.state(linuxcnc.STATE_ON) | ||
| c.home(-1) |
There was a problem hiding this comment.
The config has NO_FORCE_HOMING = 1, so the test doesn't need homing. wait_complete() returns before homing finishes, and the AUTO switch two lines down runs emcTaskAbort(), so whether it lands during homing or after depends on timing. Dropping the home, or waiting until s.homed reads all true, removes that.
| start = time.time() | ||
| while time.time() - start < 30: | ||
| s.poll() | ||
| if s.interp_state == linuxcnc.INTERP_IDLE and time.time() - start > 1: |
There was a problem hiding this comment.
This assumes the program has started within 1 s of c.auto(). On a loaded machine it may not have, and the loop then exits with no samples. Waiting first for interp_state to leave INTERP_IDLE (with a timeout), then looping until it returns, avoids the fixed second.
| sys.exit(1) | ||
| print('samples while G21 executed: %d, program_units not mm in them: %d, end gcodes[5]=%d program_units=%d' | ||
| % (in_g21, mismatch, s.gcodes[5], s.program_units)) | ||
| if in_g21 < 100: |
There was a problem hiding this comment.
This needs the poll loop to run at 20 Hz or more for the whole 5 s move; with Python starved it can fall short. in_g21 == 0 is enough to show the move was seen.
…mple counts Review (grandixximo): the config has NO_FORCE_HOMING = 1, so the home and its wait_complete() only raced the AUTO switch; the 1 s guard assumed the program had started within a second; 100 samples needed the poll loop to keep 20 Hz for the whole move. Now: no homing, wait (10 s timeout) for interp_state to leave IDLE, then sample until it returns; fail only if no sample saw the G21 move -- the mismatch count is the real check. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for the review. All three points are addressed in the new commit:
On a Pi 5 (aarch64, 6.12 PREEMPT_RT), the final version ran 5 times alone and 5 times with two CPU hogs pinned to the test's CPU; the version before the sample-count change ran 10 + 10. All passed. With the |
2 participants
Fixes #3627.
While a program runs,
stat.program_unitsshould matchgcodes. Instead it follows the interpreter's read-ahead:Why:
gcodescomes from the executing move;program_unitswas set only when the interpreter read the G20.Fix:
emcTaskUpdate(src/emc/task/emctask.cc) setsprogramUnitsfrom the active G20/G21, as suggested on the issue. This keeps all C readers (linuxcncrsh, emcsh, shcom) consistent. Canon's write stays for canterp. Idle behaviour is unchanged.Test:
tests/program-unitsfails without the fix (every sample wrong: 670/670, 774/774, 853/853) and passes with it.Related: #3657.