gnea / grbl

An open source, embedded, high performance g-code-parser and CNC milling controller written in optimized C that will run on a straight Arduino
https://github.com/gnea/grbl/wiki
Other
4.02k stars 1.6k forks source link

No delay when resuming cycle with spindle off #605

Open ademenev opened 5 years ago

ademenev commented 5 years ago

When Grbl is in Run state, sending '!' will switch it into Hol state. In this state it accepts 0x9E and stops the spindle. Now sending '~' turns the spindle on and resumes the cycle. The problem is that there is no delay between spindle power-on and start of motion. Instead, there should be a delay to allow spindle spin-up.

carneeki commented 5 years ago

Could you confirm which version of Grbl you are running? Have you made any configuration changes (specifically, SAFETY_DOOR_SPINDLE_DELAY, it has a default of 4.0 seconds).

I've taken a brief look at v1.1g and it looks like it should work, but I understand v0.9 handles resuming quite differently.

On Mon, 11 Feb 2019 at 17:27, Andrey Demenev notifications@github.com wrote:

When Grbl is in Run state, sending '!' will switch it into Hol state. In this state it accepts 0x9E and stops the spindle. Now sending '~' turns the spindle on and resumes the cycle. The problem is that there is no delay between spindle power-on and start of motion. Instead, there should be a delay to allow spindle spin-up.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gnea/grbl/issues/605, or mute the thread https://github.com/notifications/unsubscribe-auth/AAdYqFxJ2SQU_Z8Nz6T90hWImvZScUAOks5vMQ1IgaJpZM4azl3a .

-- Adam "carneeki" Carmichael p: +61 415 37 1990 w: http://bigneek.com e: carneeki@carneeki.net i: 2207644

ademenev commented 5 years ago

I am using 1.1g

Delay does work in the following scenario:

  1. Machine is in Run state, door is closed
  2. I send '!', machine enters Hold state
  3. I open the door, machine enters Door state, spindle stops
  4. I close the door and send '~'
  5. Spindle starts, motion resumes after 4 second delay

But in another scenario there is no delay

  1. Machine is in Run state, door is closed
  2. I send '!', machine enters Hold state
  3. Keeping the door closed, I send 0x9E. Spindle stops
  4. I send '~'
  5. Spindle starts, motion resumes immediately

In summary, when resuming from Door state, there is a delay, when resuming from Hold state, there is no delay, even if spindle was stopped

barma1ey commented 5 years ago

I am experiencing the same trouble. there is no spindle delay after it was toggled off in Hold state (default config)

carneeki commented 5 years ago

I think I may have found the problem, but since I don't look inside the Grbl source often, one of the regulars should probably confirm.

Line 351 of protocol.c starts the transition from any spindle override (not just stopped) to restore it.

Line 745 of protocol.c executes cycle start internally and clears the spindle override, and I don't see a pause here either.

My proposed fix is two parts (I've pushed to my own repo and sent a PR,

606) :

1) Renaming (and reusing) SAFETY_DOOR_SPINDLE_DELAY to SPINDLE_SPOOLUP_DELAY to give the spindle time to spool up from any state where it would be stopped, not just the safety door. 2) After line 351 insert a delay using the same method the safety door delay works.

I am unable to test this for a few weeks while I wait for parts to arrive, so I encourage others to look at it, and if they're willing, to test it at their own risk.

On Tue, 12 Feb 2019 at 04:20, barma1ey notifications@github.com wrote:

I am experiencing the same trouble. there is no spindle delay after it was toggled off in Hold state (default config)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/gnea/grbl/issues/605#issuecomment-462414313, or mute the thread https://github.com/notifications/unsubscribe-auth/AAdYqLHRajcCY8zNM7IKBomfwSSHMOKXks5vMaZngaJpZM4azl3a .

-- Adam "carneeki" Carmichael p: +61 415 37 1990 w: http://bigneek.com e: carneeki@carneeki.net i: 2207644

109JB commented 5 years ago

As far as I am concerned this is not an issue and the fact that the spindle starts automatically after only a feed hold is the issue. So what is actually an issue is that the spindle starts at all when the cycle-start is commanded.

On most industrial machines the feed hold and cycle start buttons start and stop the machine axis feeds only. The spindle can be turned off and on while in feed-hold, as can the coolant, but it is up to the operator to turn them back on when hitting cycle start. This is how every industrial machine I have ever worked on operated.

On an industrial machine the door is locked and cannot even be opened while the spindle is running, so I understand how Grbl has to make a logical concession for the safety door with a switch rather than a lock by turning the spindle off if the door is opened during cycle. However, I don't agree that cycle start should re-start the spindle automatically as default. It would be fine as a user configurable option (ie: config.h), but I do not feel that the cycle start should do anything more after a feed hold than restart axis motion to maintain commonality with industrial standards.

The way you open an industrial machine door and then resume the program is as follows:

1 Feed-hold

  1. Coolant off
  2. Spindle off (Unlocks door)
  3. Open Door
  4. Do whatever
  5. Close door
  6. Start spindle
  7. Start coolant
  8. Cycle-start
ademenev commented 5 years ago

@109JB , yes, that is true. For industrial machines.

But how many such machines are are actually controlled by Grbl? The truth is that it is used mostly by hobbyists and small(ish) shops, and vast majority of machines do not have a door at all.

But I still should be able to pause my machine, stop the spindle, do whatever I need, and resume the cycle without breaking my tools.

ademenev commented 5 years ago

My point is: there is no harm in a delay, but it is extremely helpful

carneeki commented 5 years ago

Partially agree @109JB .

One of the things I forgot to add in my first reply to @ademenev was to just issue 0x9E again which is supposed to toggle the spindle back on, and simply wait until spindle has spooled up before pressing ~.

I think we disagree about CS / resume though. I expect a resume to also restore the state prior to the hold, which includes the spindle state. I suspect this is what the intent was when the manual was written. By restoring the spindle state automatically, it won't really matter if you restore it manually in step 7 of the check list. In the limited number of use cases I can think of where one would want to continue motion without the spindle restored, one can simply toggle the spindle override after the restore.

As ademenev said, there's no harm in a delay, but it is helpful. It simplifies your list. I don't run coolant, but I would expect resuming to also restore the coolant state if it was manually toggled off during a hold.

ademenev commented 5 years ago

I think resume should operate consistently regardless of whether a door is used or not. After opening and closing the door and sending ~, the cycle resumes after turning spindle on and waiting 4 seconds. It is natural to expect same behavior if door is not present at all.

вт, 12 февр. 2019 г. в 15:22, Adam notifications@github.com:

Partially agree @109JB https://github.com/109JB .

One of the things I forgot to add in my first reply to @ademenev https://github.com/ademenev was to just issue 0x9E again which is supposed to toggle the spindle back on, and simply wait until spindle has spooled up before pressing ~.

I think we disagree about CS / resume though. I expect a resume to also restore the state prior to the hold, which includes the spindle state. I suspect this is what the intent was when the manual was written. By restoring the spindle state automatically, it won't really matter if you restore it manually in step 7 of the check list. In the limited number of use cases I can think of where one would want to continue motion without the spindle restored, one can simply toggle the spindle override after the restore.

As ademenev said, there's no harm in a delay, but it is helpful. It simplifies your list. I don't run coolant, but I would expect resuming to also restore the coolant state if it was manually toggled off during a hold.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/gnea/grbl/issues/605#issuecomment-462620472, or mute the thread https://github.com/notifications/unsubscribe-auth/ACSb9MIt-LJtdQp_4Bnzdx05JXnpdzLRks5vMk-hgaJpZM4azl3a .

109JB commented 5 years ago

There are usually very good reasons they do things the way they do on industrial machines. Sometimes hobby machines can learn something. However that is why I said this should not be a default configuration but a user configurable option in config.h. To the extent possible, what you want to do on your machine should not be forced on me and what I want for my machine should not be forced on you. In most things Grbl is very good at this.

As far as breaking tools. Just restart the spindle before issuing a cycle start. You had to issue a spindle stop to stop it didn't you?

I agree that resume should be consistent but my vote would be to make it consistent with industrial operation. Also, it has been noted many times in the past that Grbl as much as possible follows how LinuxCNC operates. If this is still the case, Neither Feed hold or resume affect spindle or coolant in LinuxCNC.

ademenev commented 5 years ago

So, what would work for most of us, I think is the following:

carneeki commented 5 years ago

Inverting the current default behaviour as you suggest is likely to be bad for existing installations where the current install base expects the spindle to spool up. However...

Adding a #define and some #ifndefs to config.h to leave spindle state untouched is trivial, flexible, and, permits the behaviour you would like, it just means when you next upgrade, you'll need to flip this yourself.

Perhaps when 1.2 comes out, this behaviour can be inverted to more closely reflect industrial machines, because as with major version numbers, one expects some significant changes, but not with an h to i revision (think like when D11 and D12 were swapped, the version numbers were bumped for ensuring compatibility) - here the spindle restore behaviour can remain consistent until a major version change, yet remains customisable before the version bump.

Schildkroet commented 5 years ago

The current behavior is fine! A hold doesn't stop the spindle and therefore doesn't need to wait for it after resume. Your problem is created by yourself and is no error of grbl.

barma1ey commented 5 years ago

I don't use feed hold often, if ever. So the issue is not a big deal for me. but the documentation clearly states that there must be a 4s delay and it sounds reasonable. Moreover, I think it would be nice to have $ variable to configure a delay time.

ademenev commented 5 years ago

@Schildkroet , there was no problem created.

It is clearly stated that there should be a delay:

When motion restarts via cycle start, the last spindle state will be restored and wait 4.0 seconds (configurable) before resuming the tool path. This ensures the user doesn't forget to turn it back on.

And that is reasonable

Schildkroet commented 5 years ago

@ademenev This is the case for the safety door. There it is correct. But not for a normal user hold. Just add a dwell between spindle and resume. No code changes needed. (Your changes are not reasonable and are wrong in terms of commercial machines)

109JB commented 5 years ago

Actually I'm going to have to change from "my vote would be" to I vehemently oppose incorporating this request. At least incorporating it without a config.h option.

My reasoning is that the feed hold, holds the feed and is what you are resuming from. What you do during the hold is your responsibility to re-activate, or not on a resume.

Incorporating this feature can result in unexpected behavior, namely coolant restarting and spindle restarting. This is particularly true if a user is accustomed to industrial standards. Many user, myself included come from an industrial background, or may move on to an industrial setting. On top of that, we all know how finicky MCU's are with regard to switch inputs and noise errors. I have been fighting this issue on limits on a new machine build. Imagine how issuing a feed hold to check a cutter insert could have someone's hand on a tool, and an errant noise spike could start the spindle causing much more severe injury than simply axis feed resuming.

After thinking about this, I feel that it would be sacrificing safety for a tiny bit of convenience if incorporated without a config.h option for no-spindle and no-coolant on resume.

ademenev commented 5 years ago

Actually I'm going to have to change from "my vote would be" to I vehemently oppose incorporating this request. At least incorporating it without a config.h option.

Anyway, at least documentation must be corrected.

After thinking about this, I feel that it would be sacrificing safety for a tiny bit of convenience if incorporated without a config.h option for no-spindle and no-coolant on resume.

I am totally fine with this. Make it consistent between door/no-door setups. Make it configurable, with whatever default behavior you think is most reasonable, and let the user decide what they need.

SailWithChips commented 5 years ago

Hello, I would like to say sorry if it's an stupid question or provably wrong place and sorry in advanced for my low understanding but I would like to ask here for instance working with GUI like "Universal Gcode Sender", how can I send a CMD_FEED_HOLD command ("!" or 0x83)?. Is the only way via some digital input (hardware?). I would like to be able to send a real time command to STOP the machine but I don't know how to do it via software - GUI.

carneeki commented 5 years ago

Could someone check to see if https://github.com/gnea/grbl/compare/master...carneeki:master satisfies all parties?

@SailWithChips not quite the right place for this discussion - however no special hardware is needed. The GUI should issue realtime commands to do this for you. If you are writing a GUI in C, 0x83 is just the hexadecimal representation for a char to be sent.

char hold = '!'; // send a exclamation mark to feed hold
char cmd = 0x99; // set spindle to 100%

UGS is written in Java, so there might be a Unicode conversion required. Checkout here for more info on this. I recommend popping over to the UGS people for specifics there on implementing controls there. Incidentally, UGS already has Pause and Stop buttons in the interface already at the top.

109JB commented 5 years ago

I can check it here in a bit.

109JB commented 5 years ago

@carneeki

Could someone check to see if master...carneeki:master satisfies all parties?

The above code changes don't appear to work correctly in all cases. Here is what I see

With defines enabled to restore spindle and coolant on cycle start

SpindleOn - Hold - Resume    Works as expected with spindle remaining on the whole time
SpindleOff - Hold - Resume    Works as expected with spindle remaining off the whole time
SpindleOn - Hold - SpindleOff - SpindleOn - Resume   Works as expected
SpindleOn - Hold - SpindleOff - Resume   Does not work and resets Grbl upon sending "~"

With defines NOT enabled (commented out) to restore spindle and coolant on cycle start

SpindleOn - Hold - Resume    Works as expected with spindle remaining on the whole time
SpindleOff - Hold - Resume    Works as expected with spindle remaining off the whole time
SpindleOn - Hold - SpindleOff - SpindleOn - Resume   Works as expected
SpindleOn - Hold - SpindleOff - Resume   Does not work. Grbl remains in the hold state
carneeki commented 5 years ago

Thanks for the detailed test results! I'll take a look in the morning - my guess is there is probably some combination of illegal states causing the reset. I'll also see if have a spare Arduino and some LEDs to knock together a test bench so the next commit should pass all test criteria you laid out.

carneeki commented 5 years ago

@109JB I was trying to recreate the second test case for the defines enabled (SpindleOff, Hold, Resume) when I noticed I had to use M5 rather than toggling the spindle during a motion.

It turns out this override is specifically blocked during motion (with a comment stating to permit the toggle only in hold).

Is this expected behaviour on machines you have used? I'm not sure I've ever had to toggle a spindle during motion before, and I can see reasons to both permit and block this toggle in idle, run and jog states.

109JB commented 5 years ago

@carneeki

I can't say I have ever had an occasion to turn a spindle off during run mode. Usually if this is the case something bad has gone wrong and you are looking to stop everything and more likely to use an E-stop. I have had occasion to adjust rpm during run, which is covered by the spindle speed override commands. I think it is fine to have the toggle (0x9E) disabled during run.

To tell you the truth, I used the "Spindle" button on my gui when running the tests I performed. This button is programmed to use M3/M4/M5 as appropriate when in "Idle", and it uses (0x9E) when in "Hold". I also used the GUI's feed hold button which just sends a "!", and cycle start button which just sends "~". I didn't actually stream any code as grbl doesn't really know if code is streaming or just being typed a line at a time. For the SpindleOff-Hold_Resume test I simply typed a single line of code (M3S1000), and then M5 to make sure a S value was stored by Grbl. The I insured that the spindle was off, selected feed hold and then resume and made sure the spindle didn't start.

As for when it would make sense from my perspective to be able to use the spindle toggle I would say that limiting it to the Hold state is fine. It is needed for hold because g-code input is blocked during this state so M3/M4/M5 wouldn't get through. During Idle M3/4/5 works so that can be used there.

I can't really see a need to use the toggle anywhere else but a would like to hear you ideas of situations where it might be useful in other modes.

109JB commented 5 years ago

Forgot to mention that on at least some of the machines I've used you could stop the spindle during run because the spindle speed override could be turned all the way down to 0% thereby stopping the spindle. I can't say for sure if the spindle buttons would work during run as I don't ever remember doing it.

carneeki commented 5 years ago

The more I think about it, the more I think that there's no real need to permit 0x9e toggle in anywhere but hold - and that's mostly out of good habit forming.

The first case: Operator sees the spindle isn't moving in the lead up to taking a cut, they could press toggle. But the smart thing would be to press abort and inspect the G-code to see if an M3 was not included. Toggling the spindle here could result in an undesired speed.

Second case: during jogs. If an operator is using an MPG pendant. But each of these jog motions will likely be very short, on the order of .001" / .02mm. By the time an operator takes their hand off the encoder, the jog move will have well and truly finished, and the machine would be back to idle.

Third case: machine is idle. M3 / M4 / M5 makes the most sense from a habit forming perspective (and encouraging GUI designers to pick up on whether they should send a toggle or an M code as appropriate).

I was using UGS, and I sort of expected toggle to magically change the spindle state, and spent about 2 hours double checking my actions, my wiring and then source before I found the comment indicating the block. I guess this one is just where the GUI led me down the wrong path. Thanks for helping me sound it out. I'll leave toggle state as is and get back to it tomorrow. If someone finds that Haas / Tormach / Mazak will toggle in other states, we can revisit it then, but it seems like an edge case at this point not worthy of much time.

On Wed, 13 Feb 2019 at 18:59, 109JB notifications@github.com wrote:

Forgot to mention that on at least some of the machines I've used you could stop the spindle during run because the spindle speed override could be turned all the way down to 0% thereby stopping the spindle. I can't say for sure if the spindle buttons would work during run as I don't ever remember doing it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gnea/grbl/issues/605#issuecomment-463097040, or mute the thread https://github.com/notifications/unsubscribe-auth/AAdYqPd9_1elMjp2uXIrbItyXQXKpYwrks5vM8XPgaJpZM4azl3a .

-- Adam "carneeki" Carmichael p: +61 415 37 1990 w: http://bigneek.com e: carneeki@carneeki.net i: 2207644

carneeki commented 5 years ago

With defines enabled to restore spindle and coolant on cycle start

SpindleOn - Hold - SpindleOff - Resume   Does not work and resets Grbl upon sending "~"

The crash is caused by the delay I put in in the first commit. I had the delay in the wrong place, moving it results in the following test cases:

SpindleOn - Hold - Resume - PASS - Motion resumes
SpindleOff - Hold - Resume - PASS - Cannot toggle spindle, motion resumes
SpindleOn - Hold - SpindleOff - SpindleOn - Resume - PASS - Toggle works as expected.
SpindleOn - Hold - SpindleOff - Resume - PASS - Motion resumes after delay.

In the last case, motion was started after the desired four second delay.

Of interest, the spindle state cannot be changed during a feed hold if the spindle is M5. I cannot think of a case where this would be an issue, except perhaps if one were to want to do something dangerous (like a belt change on a CNC lathe / mill that has no VFD). To that end, it is probably best to leave this behaviour as is (and I suspect this is existing behaviour anyway).

FEED_HOLD_RESTORE disabled
SpindleOn - Hold - Resume - PASS - Motion resumes
SpindleOff - Hold - Resume - PASS - Cannot toggle spindle
SpindleOn - Hold - SpindleOff - SpindleOn - Resume - PASS - Motion resumes instantly.
SpindleOn - Hold - SpindleOff - Resume - FAIL - motion stays off until spindle toggled on manually, though perhaps this is desired? It forces the user to manually start the spindle.

I'm running into a bit of a mental wall with the complexity of protocol.c and I'm tempted to break the protocol_rt_exec methods into smaller methods to make it easier to understand. (notably the many nested if() and #ifdef statements).