Add headings and other geometric pins in motion using State_tags#3995
Add headings and other geometric pins in motion using State_tags#3995rodw-au wants to merge 38 commits into
Conversation
merge latest upstream into state-tags-arcs branch
merged to upstream
merge master to state-tag-arcs branch
…d read active_modes() in rs274ngc_pre.cc still ran the original unclamped loop above the clamped one, so the stack overflow into saved_settings[5] was unchanged. The line-number copy was also commented out. Drop the duplicate and restore the line-number copy. write_state_tag() still dereferenced block on the iscircle assignment (`block->iscircle = (block == NULL) ? 0 : block->iscircle`), reintroducing the M70 NULL-deref. Replace with a plain null-guarded read. control.c output_to_hal() read the iscircle bit out of `tag.fields[]` using the StateFlag index GM_FLAG_IS_CIRCLE, which indexes past `fields[]` (GM_FIELD_MAX_FIELDS == 8). Read it from packed_flags instead, like the update_status() copy does. Also dropped the duplicate interp_normal_heading write in the G2/G3 branch and restored the WATCH_FLAGS debug block to live inside update_status().
Round 2: re-fix M70 segfault and iscircle handling
M_PI_2 is a glibc extension and is not exposed by RTAI's rtapi_math.h, so the RTAI build fails with 'M_PI_2 undeclared' at control.c:2283. M_PI is available in both, so use M_PI/2.0 instead.
Fix RTAI build: M_PI_2 undeclared
|
Some observations:
|
|
Thanks for the feedback.
Ideas to resolve Item 3 would be appreciated. Just reviewing further, it appears #3924 that the ~/linuxcnc/configs folder contents have been added to the root folder on my PC. I don't think they have been included in this PR. Could someone please check and confirm? |
…that was not resolved when work commenced on this PR. 2. Deleted spurious arc_heading file. 3. Moved docs/man/man9/output_buffer.9 to docs/man/.gitignore
Sorry my fault, I forgot to add the generated man page to gitignore (#3941) |
|
Hm, I'm not sure I've been looking too good at the nc directory... Point Point 3 should be a separate PR, I guess. Point 4 needs to be solved by you taking the original file and only change what is necessary. |
|
Just to add a comment on nc files. It is not that you cannot or should not add (example) files. The point is that there needs to be some kind of docs or procedures that use or reference them. Just having files without people knowing how to find them is kinda pointless. |
You are not the first who gets bitten by this. There is a change in the works to let CI fail when this happens. |
As mentioned in the original comment, these new pins are documented in the motion man file eg Re point 2, you now see the problem with the nc_files folder caused by #3924 . I too only saw it it when the files were removed by git add. It was very wrong as it created the files that should have been under ~/linuxcnc in whatever was the current working directory. So saving the sim to your desktop created this mess! |
The man page is a reference for the component. These man pages usually do not do a lot of examples. For that we have the other documentation in the docs/src directory, where specific scenarios are described and more in depth explanations are (should be) presented. This is usually also the place, while explaining and going into depth, to reference the examples provided.
Yes, there was that unfortunate wrong side-effect while fixing the Tcl9 compatibility problem, sorry about that. However, it should be fixed now, isn't it? |
Man pages should find their way, yes. So does the other documentation afaik. |
in preparation to fix format errors caused by editor auto format options Aditional commit to follow
then reapplying chnagfes in this PR to it
|
Formatting issue in I can't see a clean way to correct the .gitignore issue by a PR from the oversight in #3941. Its been corrected in this PR. Re documentation, when I look at a the docs page on the web site, I can't see any chapter where additional documentation of a few extra pins is appropriate except for the man page. Perhaps these gcode samples could be deployed and mentioned on the man page but where do I put them so they turn up in the users nc_file examples? Adding example gcode for read only pins just does not seem right to me. The man page should be sufficient (as it is for every other component). All runtests pass I think this is now OK for approval. It is benign as it does not add any additional features, just exposes some useful information. Breakages are unlikely. Any fixes following user feedback can be added. |
Still todo. |
Don't bother with the NML stuff, that is a different thing, the int and float arrays in the state_tags should remain (for now). |
Thanks. Some simple gcode files in the attached .zip file if anybody wants to test it. Just put all the motion.interp.* pins into halshow and observe the values while running |
|
Would it be possible to add a test under, lets say, |
|
I'll have a think about it tomorrow. You could probably test straight headings as they are set at the beginning of a segment but that's not really helpful as you really want to check the arcs which are dynamic. I wonder if you could use hal streamer? A fail would be where the heading does not change during the run. Could that be done? |
|
I'm a bit stuck here. I have the test mostly working, Linuxcnc opens, the gcode file is processed but I am not seeing any of the halsampler output in the results file. If someone could test and update or advise what to do, it would be much appreciated. Steps Outstanding:
|
|
OK, after a bit more testing with the hal file on a normal axis config, halsampler immediately outputs a buffer full of readings before the GUI opens which means that the buffer is fully depleted before the gcode file is even loaded! Unless this can be resolved, its not possible to implement a runtest for the heading. Any ideas please let me know |
|
It may be appropriate to add more pins to the streamer:
Then instantiate the sampler in HAL (could be a normal HALFILE): In the UI script: ...
# Load program
c.program_open("heading-test.ngc")
# Enable sampling
h.set_p("sampler.0.enable", "1");
# Run program
c.auto(linuxcnc.AUTO_RUN, 1)
...The UI script should wait for the halsampler executable to finish or it must send a SIGTERM to halsample to instruct it to terminate. |
|
Just came to think of it... I actually fixed the python HAL stream interface. You don't need the halsampler program and could read the data directly in the UI wrapper in its loop. sampler = h.stream(h, h.sampler_base, "sfff")
...
# sampler.read() returns a tuple with the sample value
def read_samples():
while sampler.readable:
print(sampler.read()) # maybe this should do something else...
...
# Don't wait, loop with sampling
# l.wait_for_interp_state(linuxcnc.INTERP_IDLE)
while True:
s.poll()
if s.interp_state == linuxcnc.INTERP_IDLE:
break
if sampler.readable:
read_samples()
time.sleep(0.001)
h.set_p("sampler.0.enable", "0")
time.sleep(0.1)
read_samples() # drain rest
...
# Do other stuff |
|
Thanks, I decided to stick with halsampler for now, then try your new method but I am having a bit of trouble with this And yes, I saw your errant C ; line ending. :) I did have a lot of other pins which I had tested in a normal sim but was taking baby steps :) The line number is a good idea though. |
|
You Those semicolons... well, habits die hard ;-) Using halsampler makes it more difficult because it is a program running asynchronous from the UI wrapper and you have to control its termination. Doing the sampler read in the UI wrapper eliminates that async and external program problem go away. But, sure, test it first ;-) |
|
using the old hal way failed as the test-ui never returned so I went to the python method.
I ran up a normal linuxcnc instance for testing and debugging and the script runs to completion and I could see it homing, loading and running the gcode file. But there is no sampler data arriving adding a few more print statements I'm really out of my depth here. I've never used the python interface before and barely know the python language. I much prefer ;'s too :) If somebody could review this after my last commit, it would be great! If we could get data printing, I should be able to devise some tests |
|
I'll have a look. |
|
I have a sampling version of the test. No checks performed on it yet. The attached zip is the content of the tests/heading directory: heading.zip You need to Problems found:
The data is currently dumped in the Please note that the motion-type field lags one sample when the next command is executed. |
|
Thanks so much for this. Sorry, I thought I had deleted the extra files. |
… as we got inconsistant calculations. Also have to complete the validation
|
So had some time today. I have the tests validating now but initially had issues with radius calculations when the machine had imperial units and my gcode was in metric so the tests found an issue before they were released! Updating the ini file in the tests fixed that and all tests passed. I take every 100th entry and perform tests on them as there are 1000s of entries. All lines assessed now pass including g2,g3,g1,g0 |
Yes, the units were off. Also, the feed rate units do probably not match.
That is a problem. This is coming from RT and you do not know how many samples you will get and therefore not reproducible. You probably need to assess the situation whenever the state changes. |
|
result.txt I have the test operating succesfully see reults.txt Please note that unexpected deviations from the expected path set in the gcode by joint.0.pos-fb joint.0.pos-fb are out of scope of this PR. I noted that around the transition between the G2 and the G1 you added, the radius gradually deviates up to 1.2mm at the end and no G64 or G61 settings would alter this. This was for up to 40 readings at the end of the G2 and 5 readings at the start of the G1 In the end, I surmised that the planner must blend the trajectory regardless of the expected gcode controls so I ignore the deviations in the G2 unless they exceed 40 and skip the first 5 readings in the G1 I think this is a valid approach becasuse the remaining 2700 odd readings gave perfect results and we don't touch anything with the actual motion. I still need to review gcode vs machine units but wanted to finish this first |
After the DISPLAY exits, content is flushed to the resuilt file. So we overwite this in test.sh based on sucess or fail. This lets us use an expected file to check pass or fail
|
Runtests now works. |
| global nsamples, lastsample | ||
| sample = sampler.read() | ||
| if sample != lastsample: # Don't print duplicates | ||
| if (nsamples % SAMPLEN == 0): |
There was a problem hiding this comment.
SAMPLEN is one (1) and should always be one. This modulo/if code is therefore noise and should be removed
| if sample[0] not in (4, 5, 6): | ||
| # We are only interested in lines 4,5,6. Do not change gcode file |
There was a problem hiding this comment.
I'm not sure you should actually test on line-number. You have the motion-type field (trailing by one sample) that should be enough? If not, why not?
In principle you want to check heading for your motion types and could do so based on that field. It would certainly simplify this code if I'm not mistaking. You can then also change the g-code to suit you without having to hack this code as well.
There was a problem hiding this comment.
yes, we can probably get rid of the line number test and only test G0,1,2,3 lines based on motion type. Lines was my first idea when I had no clue where to start. I've just completed an update to check gcode units vs machine units but have not tested it. I also want to add tests using imperial machine units. (and rebase to master).
One crazy Idea I had was to write back the current heading to a next heading field in the previous segment. This would allow a component to check if a tangential knife needed to be lifted before the next segment and use external offsets before the knife is twisted while still in the material. Would there be a way to do that? (would make it a seperate PR)
I have seen at least one test that pre-processes the "result" file to leave only the pertinent data before comparing. |
Thanks Andy, I've now done something similar this morning by overwriting the results file with success/failure messages from tests.sh. Thinking about it, I probably could enable verbose messaging so that if the test fails, runtests -n would leave the error messages and test data for review. |
6 participants
Outline
create new [motion.interp.xx ] pins for heading and various geometric data (refer man motion for this PR). Note the following use cases behind this PR.
I am sure the community will find many other use cases.
Code notes
Caveats
We use the state tag motion_type (GM_FIELD_MOTION_MODE) to detect G0,G1,G2,G3 in motion. Currently there is no way to use existing interpreter equates for these values so the numbers are hard coded as 0,10,20,30.
RunTests
These pass locally
Acknowledgements
Many thanks to Luca Toniolo (grandixximo) for his encouragement and help resolving errors with the tests.