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 53 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. |
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. |
…and G20/G21
1. Corrected units where gcode is not in machine units in interp_convert
2. Cleaned up tests so it no longer looks at line number so now universal
3. Changed tests so there is now seperate tests heading-mm and heading-in
for metric and imperial device units
4. Added additional gcode so it now tests G20 and G21 on
metric and imperial machines
5. If a test fails, the result file will included detailed DISPLAY output
I did originally try to fold metric and imperial configs into the one folder,
but had no luck.
Merge upstream/master into local master branch
Merge master to upstream
|
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. |
| *(emcmot_hal_data->interp_arc_center_y) = 0.0; | ||
| *(emcmot_hal_data->interp_straight_heading) = 0.0; |
There was a problem hiding this comment.
Isn't the z-value missing here?
There was a problem hiding this comment.
these lines are uperflous as they are all set (including z) further down at around line 2249.
Removed
There was a problem hiding this comment.
Now fixed. Was originally looking in the wrong file (control.c) and found superflous code there!
| state.fields_float[GM_FIELD_FLOAT_STRAIGHT_HEADING] = (block == NULL) ? 0.0f : (float)block->arc_heading; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_RADIUS] = (block == NULL) ? 0.0f : (float)block->arc_radius; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_CENTER_X] = (block == NULL) ? 0.0f : (float)block->arc_center_x; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_CENTER_Y] = (block == NULL) ? 0.0f : (float)block->arc_center_y; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_CENTER_Z] = (block == NULL) ? 0.0f : (float)block->arc_center_z; | ||
| state.fields_float[GM_FIELD_FLOAT_NORMAL_HEADING] = (block == NULL) ? 0.0f : (float)block->normal_heading; | ||
| state.flags[GM_FLAG_IS_CIRCLE] = (block == NULL) ? false : block->iscircle; |
There was a problem hiding this comment.
These are explicitly cast to float. You should let the compiler take care of that. Also, I think that the state.fields_float should actually be a double, as all the rest of LCNC is double.
Wouldn't it be more natural to write this like:
if (null != block) {
state.fields_float[GM_FIELD_FLOAT_STRAIGHT_HEADING] = block->arc_heading;
state.fields_float[GM_FIELD_FLOAT_ARC_RADIUS] = block->arc_radius;
...
} else {
state.fields_float[GM_FIELD_FLOAT_STRAIGHT_HEADING] = 0.0;
state.fields_float[GM_FIELD_FLOAT_ARC_RADIUS] = 0.0;
...
}| lastrecnum = 0 | ||
| ctr = 0 | ||
|
|
||
| def is_arc_valid(sample, tolerance=0.11): |
There was a problem hiding this comment.
You never call with empty tolerance. Therefore, you should remove the default from the argument.
(also, the default tolerance of 0.11 is a bit huge, don't you think?)
There was a problem hiding this comment.
Removed and tested
As said elsewhere the tolerance of 0.001 mm used now is quite a lot tighter than 0.001 inches. Is that an issue?
|
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. |
| joint = &joints[joint_num]; | ||
| joint_data = &(emcmot_hal_data->joint[joint_num]); | ||
|
|
||
There was a problem hiding this comment.
On this line you (only) add trailing whitespace.
I discovered that you have trailing whitespace on lines multiple places in your code. You should check for that and remove it.
There was a problem hiding this comment.
I will review in a seperate commit from code fixes
| if(nullptr != block){ | ||
| state.fields_float[GM_FIELD_FLOAT_STRAIGHT_HEADING] = (float)block->arc_heading; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_RADIUS] = (float)block->arc_radius; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_CENTER_X] = (float)block->arc_center_x; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_CENTER_Y] = (float)block->arc_center_y; | ||
| state.fields_float[GM_FIELD_FLOAT_ARC_CENTER_Z] = (float)block->arc_center_z; | ||
| state.fields_float[GM_FIELD_FLOAT_NORMAL_HEADING] = (float)block->normal_heading; | ||
| state.flags[GM_FLAG_IS_CIRCLE] = block->iscircle; |
There was a problem hiding this comment.
Do not cast these values to float. At best the compiler must solve this issue automatically and, actually, types will be matched after #4020. You must not artificially reduce accuracy by casting.
| *(emcmot_hal_data->interp_arc_center_y) = 0.0; | ||
| *(emcmot_hal_data->interp_straight_heading) = 0.0; |
|
Oops Fixed command.c. Was looking in the wrong file (control.c) and found some redundant code anyway |
| * | ||
| * 1) Since these are used as array indices, they have to start at 0, | ||
| * be monotonic, and the MAX_FIELDS enum MUST be last in the list. | ||
| * be monotonic, and the GM_FIELD_MAX_FIELDS enum MUST be last in the list. |
There was a problem hiding this comment.
This corrects an ambiguity in the original comment
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.