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 35 commits intoLinuxCNC:masterfrom
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. |
I already looked into this a long time ago for kerf correction: |
Well that was a long time ago! I have been told water jet machines adjust the cutting head angle to allow for perpendicular cutting. Even plasma has a tapered kerf. I was thinking of countersinking with something simple like But external offsets would allow the cutting angle to be adjusted dynamically for kerf correction. QTPYVCP's Monokrom plasma GUI includes a material/machine SQL database I contributed containing high definition cutting parameters so it could be extended to include kerf correction for specific materials. The beauty is that it can all be done in 2D which avoids expensive 5 axis post processing and keeps the operator in familiar 2D territory. |
Indeed, but there may still be some valid points raised in that discussion. |
Agreed, but I think actioning those points is out of scope of this PR. This is an enabler for other derived enhancements which most likely can be delivered via components. Is there anything else I need to do to finalize the PR so it can be accepted? |
|
I think my comment about ACTIVE_SETTINGS is still open. The situation is somewhat ugly. If state_tags or settings was a "proper" structure with named members, you wouldn't use a for loop but explicit assignments. The code as it is is a bit fragile and will lead to a silent bug if somebody extends ACTIVE_SETTINGS without also inserting the same field into the float fields of state tag structure at the same offset. I would probably add a static_assert that ensures ACTIVE_SETTINGS is 5, but that also is kinda ugly. |
I'll look at the assert tomorrow. In fairness, some decisions were made by the original dev to make the tag structure compact because it is attached to every interpreter segment. Clearly from the code comments state_tags was designed to be extended. This is the first serious attempt to extend the float values and its identified some issues. The challenge will be not to break existing usage now its been in place for 5 years or so. Its a bit of a chicken and egg problem now. It needs a seperate PR but now it needs this to be committed before we can do that. I have an idea that may solve your concern which may work. But I need to give it some serious thought. |
|
AFAIK it was reluctance to explicitly write out the serialization stuff for NML but you are right that's a different cake. |
1. Included support for arcs on all planes 2. Updated man page 3. Set iscircle pin to false if G2/G3 is cutting a helix This is a good example of how to add a state_tag and create a pin for it. Also demonstrates calculating trig data in interp::tag_arc in interp_convert.cc before passing data to motion.c where it receives additional processing in update_status() Note in motion we again have to resort to hard coded numeric values for planes (170,180,190) as we can't get access to the enum/equate in motion.
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. |
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.