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 1 commit into
Conversation
|
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? |
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. |
|
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 think you also need to restore all the files you deleted from nc_files. You removed much more than you probably intended by simply deleting the entire directory. (This may be partly my fault because I suggested something like it, but I was actually only referring to the added files.) |
|
What a nightmare, nc-files restored. I thought the folder had been created by #3924 and did not realise that was a genine folder. So much to learn about this all! |
|
Thanks. You are still missing file I also added some comments on the gitignore changes. |
| // overrunning the caller's stack array. | ||
| int n = GM_FIELD_FLOAT_MAX_FIELDS < ACTIVE_SETTINGS | ||
| ? GM_FIELD_FLOAT_MAX_FIELDS : ACTIVE_SETTINGS; | ||
| for (i=0; i<n; i++) |
There was a problem hiding this comment.
why not use std::min? or even just for(i = 0, i < ACTIVE_SETTINGS, i++) ?
There was a problem hiding this comment.
This was added in a PR by Luca to resolve runtime tests. Without it, because we have added additional state_tags, he maintains the active_settings array would overflow (which made sense).
This worried me because I thought the codebase should be more robust than that so I queried it with him on Discord:
Just get some feedback on a bug you found. In interp_base.hh we have #define ACTIVE_SETTINGS 5
Shouldn't that be #define ACTIVE_SETTINGS GM_FIELD_FLOAT_MAX_FIELDS to prevent future overflows of state tags related code? Or maybe better #defined as the max of the 2 equates?
grandixximo — Yesterday at 11:35 PM
no, that will break the API
compatibility with axis UI and other
If we want a guard against future drift, a one-liner like:
static_assert(ACTIVE_SETTINGS <= GM_FIELD_FLOAT_MAX_FIELDS,
"ACTIVE_SETTINGS slot must exist in fields_float[]");
would catch anyone adding an ACTIVE_SETTINGS entry without growing the tag, without coupling GM_FIELD_FLOAT_MAX_FIELDS with ACTIVE_SETTINGS.
Using max(5, 11) = 11 would re-create the exact bug my PR fixed
I did not understand the static_assert and was going to research it once I got this done and raise it here if necessary.
There was a problem hiding this comment.
static_assert is like assert (it checks an expression and only continues if expression is true) but it "runs" at compile time. So all quantities in the expression need to be known at compile time.
max would be the "wrong dircetion", you want to copy those settings that both structures can accomodate (the minimum).
static_assert like
static_assert(ACTIVE_SETTINGS == GM_FIELD_FLOAD_FEEDRATE);would probably be a good idead nonetheless. Frankly, that whole
state_tagsstructure is a mess and should be converted into a proper struct with proper members, but that's a different story...What is the difference between
GM_FIELD_FLOAT_FEEDand aGM_FIELD_FLOAT_FEEDRATE?
I will leave it to others with more knowledge to edit this code. Luca fixed it twice after I broke it when merging!
re GM_FIELD_FLOAT_FEED and a GM_FIELD_FLOAT_FEEDRATE
I'm not sure. I will review that another day. I do recall Dewey added a feed_upm pin (name from memory) using state tags when they were first released but now I see a number of other feed rate pins. machine units per minute was not particularly useful because if using it you would normally want to compare it with current_vel which is in machine units per second (for plasma cutting you want to disable torch height control if current_vel < original feederate to prevent diving.
I found the packed flags OK to work with but the messy bit is the data has to be repeated in a C++ structure in the interpreter. So we have a classic case of data redundancy. Using a union placing bit mapped fields over the unsigned int might help but that's likely to be just as messy!
The other thing I learnt is that the tag is applied at the start of the segment so to see arc headings in real time, you need to calculate them in motion. Thats also a bit messy as I mentioned in my initial comment.
There was a problem hiding this comment.
static_assert is like assert (it checks an expression and only continues if expression is true) but it "runs" at compile time. So all quantities in the expression need to be known at compile time.
max would be the "wrong dircetion", you want to copy those settings that both structures can accomodate (the minimum).
static_assert like
static_assert(ACTIVE_SETTINGS == GM_FIELD_FLOAD_FEEDRATE);
would probably be a good idead nonetheless. Frankly, that whole state_tags structure is a mess and should be converted into a proper struct with proper members, but that's a different story...
What is the difference between GM_FIELD_FLOAT_FEED and a GM_FIELD_FLOAT_FEEDRATE?
There was a problem hiding this comment.
Is this code even necessary? We copy settings[] to state_tags in interp_write.cc and here we copy the same data back from state_tags to settings[] here in rs274ngc_pre.cc
Are we loading data for the GUI to see? Maybe someone with more knowledge can help here.
3D_chips.pdf exists but is in a .gitignore file I would not have added that! |
|
Local tests pass |
Yes, PDF files are ignored by the |
|
You still have the |
|
Corrected typos in comments. (1 was pre existing)
Thanks for everyone's help |
|
I had to force add Chips as it had been removed so restore failed. |
|
I don't trust Githubs tests, all runtests still pass locally |
|
The problem with purely whitespace changes are that they are detractors from what is being changed. As such, it may be fine to change whitespace, but in a complex change it can become hard to keep track. (/me being pedantic) |
|
Yeh, I know you are being pedantic but as you say this is complex so I don't mind. Helps me learn. I did a git clean and rebooted but they are persisting. It can't be anything in that last commit surely. The only code I touched was adding back in the commented out pin. Any ideas short of recloning? |
|
This is now complete. Changes include:
All runtime tests pass locally. |
|
Last minute I realised I had an error in gcode (not using G20) and this was not picked up because I was not using the corrected radius in interp_conv.cc. |
|
You have in the test g-code: Should these two be reversed? First set the machine into metric and then set the tolerance to 0.1mm? |
|
I will review suggestions tomorrow. |
|
Please find attached heading2.zip that has the merged tests/heading/ content. It also updates the ui script with the points that I mentioned in review. The CI problem is likely caused by your master branch being divergent from LinuxCNC master branch. You added some commits directly to your master branch and those changes should probably have been in your state-tags-arcs branch. You need to save all your changes, then revert/reset your master branch to match LinuxCNC's and then apply your saved changes to your state-tags-arcs branch. Then you can look and find where you made the error. |
|
That should go into |
You were correct about the divergence. It happened in the first batch of commits and it must have finally bitten. Now fixed. I noticed you corrected the tolerance. But I think its now looser on an inch machine thjan a metric machine. Not sure how to fix that. Can you pass a paramter to the ui file? I guess it won't really matter becasue if its off it will be a G20/G21 issue and it will be far more than that! |
I noticed you had fixed that. |
|
Oops Fixed command.c. Was looking in the wrong file (control.c) and found some redundant code anyway |
|
Finally! All cleaned up. Sorry about the stream of commits. I'm very relieved our revised gcode passes all tests Thanks for all your help and suggestions. It will be great to see this committed now. Some users are keen for it as it was a forum post that spurred me on to get this done. I had an attempt about 6 years ago and gave up but it was good to start afresh with this commit as things had changed and I had done even more silly things than I did in this time! |
|
Looks fine to me ;-) |
|
@andypugh you need to squash the commits. |
|
Should I rebase to master at this point? |
|
In principle, you could squash the commits locally into one commit with a new message and then force push the result. It is a bit tricky, so make a backup before you attempt and verify locally before you push. But the squash can also be done in the merge here. |
|
@andypugh Now rebased to master and squashed to a single commit. Ready to merge into master. |
5 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.