search

LinuxCNC / linuxcnc

LinuxCNC controls CNC machines. It can drive milling machines, lathes, 3d printers, laser cutters, plasma cutters, robot arms, hexapods, and more.
http://linuxcnc.org/
GNU General Public License v2.0
1.81k stars 1.16k forks source link

Updating `state-tags-master` branch for acceptance into LinuxCNC #134

Closed zultron closed 2 years ago

zultron commented 8 years ago

I'm creating this issue to track work on the state-tags-master branch (originally introduced on emc-developers), with the goal of acceptance into LinuxCNC.

In offline communication, @cradek pointed to the following problems he found in that branch:

He also stated there are more tests left: "[I] did not get to testing the restore-on-abort functionality which is where I am more wary."

zultron commented 8 years ago

I was able to reproduce the problem in SF issue 431. The problem in SF issue 432 is demonstrated with this sequence:

zultron commented 8 years ago

@robEllenberg, I'm finding files in the state-tags-master branch licensed as GPLv2 (only). Can I change the license to GPLv2 or later?

jepler commented 8 years ago

Note that by project policy, new submissions to linuxcnc need to be covered by GPLv2+. GPLv2(only) is not OK.

robEllenberg commented 8 years ago

Sure, I see no reason for them not to be GPL 2+. For legal purposes, should I make the commit that updates them?

On Wed, Aug 10, 2016, 3:26 PM John Morris notifications@github.com wrote:

@robEllenberg https://github.com/robEllenberg, I'm finding files in the state-tags-master branch licensed as GPLv2 (only). Can I change the license to GPLv2 or later?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/LinuxCNC/linuxcnc/issues/134#issuecomment-238976485, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJ6PHR7hrRvHOyUnckmlUlIDWdyfeQFks5qeiXSgaJpZM4Jfg-S .

zultron commented 8 years ago

@robEllenberg, I think that your permission is sufficient. Thanks for replying, and thanks for the state tags work!

zultron commented 8 years ago

I have prepared a branch based on the latest 2.7 with the latest set of updates. It fixes the issues @cradek reported in the two above-mentioned SF issues, and also fixes #138, where interp and canon states can be out of synch after abort, which is related to @cradek's additional concern about restore-on-abort.

I'd really appreciate reviews by @robEllenberg, @cradek and @SebKuzminsky, if possible, and I'll submit a PR soon. Thanks!

robEllenberg commented 8 years ago

Looks good to me. I quickly verified in simulation that SF issues 431 and 432 are fixed, and restore-on-abort seems to work as expected for coordinate systems (The active G code matches canon / motion coordinates). Also, I added a bit of code cleanup here, no functional changes though.

cradek commented 8 years ago

[zultron and I talked about this in person] After aborting a program during active G92, the DRO still shows the G92 offset, but if you MDI g0x0y0 it will go to the origin as if G92 is not applied. I think the fix is to add G92.2/G92.3 (aka something like "G92 offset is applied") as a new modal group, add it to the Active GCodes readout, and add it as a new state flag, perhaps like

state.flags[GM_FLAG_G92_APPLIED] = settings->parameters[5210] != 0.0;

cradek commented 8 years ago

@zultron @SebKuzminsky please review glo/statetags

I think it's ready to merge. (I added G92.x handling I was complaining about above)

cradek commented 8 years ago

@zultron @robEllenberg testing glo/statetags some more, I see a new problem. When I abort while deep in subroutines, I get an interpreter error: Unknown oword number

To reproduce, build glo/statetags, run sim/axis/axis, F1 F2 home all, open flowsnake.ngc, touch off Z if necessary to let it run, run, wait until it gets to the XY motions, hit ESC. Expected behavior is no error message, actual behavior is the above error message.

Bisecting shows the first bad commit as: 2a39e3a statetag: added additional flags to prevent restoring interp state if aborted mid-remap

zultron commented 8 years ago

Thanks @cradek, I'll take a look.

jepler commented 7 years ago

I have a concern about G64—it looks like programming G64 alone resets the P- and Q- values, and that state tags will do exactly this. I'm not entirely sure how to test this given that #177 is also affecting whether P- and Q- are retained. See IRC conversation: http://tom-itx.no-ip.biz:81/~tom-itx/irc/logs/%23linuxcnc-devel/2016-11-15.html

robEllenberg commented 7 years ago

This sounds like a potential issue for end users too. It would be useful to have a g-code command to change the blending mode without changing tolerance values. If we had that, then the state restore could use it. Another possibility is to store the tolerance as part of the tag (then restore it). That way, it would work even if the user changed tolerances during the program.

zultron commented 7 years ago

I'm looking at solving the G64 problem with Rob's state tags work. As I said over at #177, I have a unit test that demonstrates the problem. It's lengthier than necessary, since I'm also using it while developing the fix.

The fix I'm working on follows Rob's suggestion to save/restore the tolerances in the tag.

Reviewing this issue, it looks like G64 and @cradek's Unknown oword number are the final two outstanding issues here. I'll try to get these resolved in the next week or so.

zultron commented 7 years ago

PR #283 addresses the G64 issue. Unknown oword number issue still outstanding....

zultron commented 7 years ago

As for @cradek's problem, commit 2a39e3a found by the bisect is at the heart of the issue (Unknown oword number is a red herring; this can be addressed later).

After that commit, a state tag is flagged as not restorable if it comes from within a sub or remap. At least in my regression test, the error originates in:

Although the error message is printed (with more sane error when state tag and interp debug flags enabled), it's not a fatal error.

I understand why a tag should be unrestorable at least in a remap. It seems @robEllenberg thinks aborting in a remap or sub is a corner case where restoring the state tag can be skipped. I don't understand the consequences yet, but will report back after some thought. In the meantime, I'd love to hear what others think.

zultron commented 6 years ago

Is there still interest in this work? I haven't updated this issue in a while because I added support for state tags in non-motion canon commands, which solves problems like incorrect status buffer at M0/M1 program pause, and that was merged in Tormach's production branches only a few months ago after shaking out various issues. At this point, the updates are looking pretty solid, so it could be time to revisit the possibility of getting state tags merged into LCNC.

c-morley commented 6 years ago

There is interest yes.

Chris M

-------- Original message -------- From: John Morris notifications@github.com Date: 2018-07-30 11:36 AM (GMT-08:00) To: LinuxCNC/linuxcnc linuxcnc@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [LinuxCNC/linuxcnc] Updating state-tags-master branch for acceptance into LinuxCNC (#134)

Is there still interest in this work? I haven't updated this issue in a while because I added support for state tags in non-motion canon commands, which solves problems like incorrect status buffer at M0/M1 program pause, and that was merged in Tormach's production branches only a few months ago after shaking out various issues. At this point, the updates are looking pretty solid, so it could be time to revisit the possibility of getting state tags merged into LCNC.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/LinuxCNC/linuxcnc/issues/134#issuecomment-408966178, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AHBrNSyYi8kOkZodrI6GW_3SiBMpwGWSks5uL1IigaJpZM4Jfg-S.

rene-dev commented 5 years ago

any update? There is still interest.

andypugh commented 2 years ago

Did we merge this?

zultron commented 2 years ago

I can't seem to find a related PR, but maybe developers are pushing directly to mainline branches. This related file is in master branch:

https://github.com/LinuxCNC/linuxcnc/blob/master/src/emc/motion/state_tag.h

I'm closing this.