Centralize click-handling in the LCD loop

[thinkyhead]

Addressing #5052

• The LCD loop sets the lcd_clicked flag in a single place
• The flag is extern and replaces LCD_CLICKED (direct button pin test)
• M0/M1 is simplified

[ghost]

After small modification for fixing a compilation error (and optimize), I confirmed that click-button works propriety.
https://github.com/esenapaj/Marlin/commit/dcabf4ed7195e59cdfd38a8aa1d30e78fc1f3633

It seems that ignore_click is no longer used anywhere.

[thinkyhead]

@esenapaj Yeah, ignore_click is made totally obsolete by this.

[thinkyhead]

I had to make some more tweaks. Is it still holding up?

[ghost]

I've tested newest one just now, and I confirmed that click-button of REPRAP_DISCOUNT_SMART_CONTROLLER works propriety without any modification.
And new compilation error or warning are none.

But, I found new problem.
With REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER, click-button doesn't works propriety.
Click-button respond all-time, but doesn't turn into menu, only lower half of LCD flickering once.

[thinkyhead]

Try adding the line lcd_clicked = false to the lcd_goto_screen function. That may do the trick.

[ghost]

adding the line lcd_clicked = false to the lcd_goto_screen function

It has solved the problem definitely.
With this modification
https://github.com/esenapaj/Marlin/commit/fbb46b7b64e45da8ff61d6e2985a17580635f730
, I've confirmed that click-button works propriety with both REPRAP_DISCOUNT_SMART_CONTROLLER and REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER.

[thinkyhead]

@esenapaj I have a note from @Kaibob2 that M0 / M1 is still unreliable. I've made some changes, but I can't see any cause for it. The wait_for_user flag is cleared whenever the button is pressed (as long as it's not wait_for_unclick'ing). Can you see what I'm missing?

[Kaibob2]

@thinkyhead I made two videos this morning on the fly before leaving for work.
http://sendvid.com/xyhgz53t
This video (Click issue 1) shows, that short clicking cancels M1, but you have to be lucky to hit the button at the right time. Sometimes it reacts on the second hit, sometimes on the 7th or the 12th or whatever.

http://sendvid.com/351x8x4l
This video (Click issue 2) shows, that holding the button after some failed short click attempts cancels M1 reliable.
The beep is almost inaudible because i forgot to increase duration and frequency, but it only beeps when M1 gets canceled afterwards. The failed clicks produce no beep.

Further: M112 does nothing when in M1 state. After sending M112 the serial monitor continues to report MSG_BUSY_PAUSED_FOR_USER

http://www.filedropper.com/changedfiles
These are the files i've changed. The rest is from https://github.com/thinkyhead/Marlin/tree/rc_break_m1_fix from 26.10.2016

[thinkyhead]

@Kaibob2 I see. So the click is still unreliable in M1, and M112 should also do what M108 does. Good to know. I'll have some folks with diagnostic skills try some things and hopefully this will be cleared up.

I'm pretty sure that M112 has never been processed until the previous command finishes, unless you enable EMERGENCY_PARSER. Of course, that's what EMERGENCY_PARSER is for: to keep certain commands from being blocked, as they normally are.

So otherwise, the menu system works ok for you, however?

[ghost]

I've tested m0, m1, and m112 with updated this PR (09c7305). (EMERGENCY_PARSER is enabled)
It looks like that m112 in m1 state itself works fine.

But I found that m112 has a problem with host software (Repetier-Host).
If hardware reset is conducted in m112 state, m112 state isn't cleared.
Only reset from host software clear m112 state.

Video clip:

A branch that used for test: https://github.com/esenapaj/Marlin/tree/testes

[ghost]

All M112 does is call kill, so do other things that call kill also require software reset?

It looks like that it doesn't.
I cause MAXTEMP Err intensionally with MAX31855 thermocouple by pulling a signal wire,
and done hardware reset after restoring the wiring, It has been reset properly.

[ghost]

Um? when EMERGENCY_PARSER is disabled, behavior is different and strange.
When I order m112 in m1 state, displaying of LCD doesn't transit to 'kill screen', and LCD is still refreshed, encoder works.
But, printer doesn't respond to host software, and then I click the encoder, LCD transit to 'kill screen'.
It's really strange.

But when I order m112 out of m1 state, It looks like behavior is right.

And,

m112 → hardware reset: no problem
m1 → m112 → hardware reset: no problem
m1 → m112 → click a encoder (screen is killed) → hardware reset: no problem

It looks like that m999 isn't needed in case of EMERGENCY_PARSER is disabled.

[thinkyhead]

when EMERGENCY_PARSER is disabled … I order m112 in m1 state, displaying of LCD doesn't transit to 'kill' … LCD is still refreshed, encoder works. But, printer doesn't respond to host software, and then I click the encoder, LCD transit to 'kill screen'.

That's because M112 goes into the command queue and isn't executed until M1 exits. This is the reason EMERGENCY_PARSER was created. The alternative would be to make Marlin a state machine instead of going into loops like while (wait_for_user) idle().

It looks like that m999 isn't needed in case of EMERGENCY_PARSER is disabled.

My guess is that the text "M112" is still in the serial buffer. Apparently we need to flush the buffer if the emergency parser gets triggered.

[thinkyhead]

m1 → m112 → hardware reset: no problem

I see no problem regardless of whether EMERGENCY_PARSER is enabled. But I haven't tested with Repetier, just using the "dumb" serial console in Arduino IDE. Doesn't seem to matter which host software I use.

[thinkyhead]

holding the button after some failed short click

@Kaibob2 Is M1 still not responding to short clicks with the latest code in this PR? What about menu navigation? Does it also not respond to short clicks?

@esenapaj isn't able to reproduce, and I can't see any reason why this might be the case.

The wait loop calls idle() which immediately calls lcd_update() which immediately reads the button states, and immediately clears wait_for_user (unless the button is still being held down from a previous click). So while (wait_for_user) idle() should exit on the very first button click.

What model is your LCD controller?

[Kaibob2]

@thinkyhead Timeshift between continents is really annoying. When i went to bed the first comment came in here. Anyway:
Everything fine here. I updated to the most recent latest code in this PR and the button reacts perefctly as it should when in M1. Great work!

BUT:
Now it behaves like this (with emergency_parser enabled):

M112 working
Printer freshly rebooted
Send M1 via serial
Display says "Waiting for User"
Send M112 via serial
Printer shuts down (i have suicide pin enabled and a SSR in the mains line)

M112 working
Printer freshly rebooted
Send M1 via serial
Display says "Waiting for User"
Send G1 Z1.000 F300
Send M112 via serial
Printer shuts down (i have suicide pin enabled and a SSR in the mains line)

M112 not working
Printer freshly rebooted
Send M1 via serial
Display says "Waiting for User"
Send G1 Z1.000 F300
Send G1 Z1.000 F300
Send M112 via serial
Nothing happens
Click the button
Printer shuts down (i have suicide pin enabled and a SSR in the mains line)

I guess this is related to
That's because M112 goes into the command queue and isn't executed until M1 exits.

This
My guess is that the text "M112" is still in the serial buffer. Apparently we need to flush the buffer if the emergency parser gets triggered.
seems like a good attempt.

[ghost]

I see no problem regardless of whether EMERGENCY_PARSER is enabled.
Doesn't seem to matter which host software I use.

Yes, perhaps it's problem of host software rather than Marlin.
In case of EMERGENCY_PARSER is enabled,

When I connect between PC and printer by Repetier-Host:
m1 → m112 → hardware reset: endless m112 state

When I connect between PC and printer by Pronterface:
m1 → m112 → hardware reset: no problem

Maybe this problem can be solved by altering the Marlin code,
but at the same time, maybe it can be solved by altering the code of host software (Repetier-Host).

[thinkyhead]

Thanks @Kaibob2 and @esenapaj. I'm glad it holds together. I've been wanting to centralize click-handling for a long while.

Yes, perhaps it's problem of host software rather than Marlin.

@repetier Can you offer insight as to why the host would repeatedly send M112? Unfortunately, since it immediately calls kill() it doesn't send an "ok" back to the host. The host should not expect an "ok" reply from M112.

Fortunately this is rare enough that I don't think we have to hack a workaround into Marlin.

[Kaibob2]

@thinkyhead Wouldn't it be necessary to implement a

need to flush the buffer if the emergency parser gets triggered.

before finally merging this?
Because now all commands only work if the buffer is empty when in M0/M1 state?!

[thinkyhead]

Wouldn't it be necessary

I was wrong about that. The buffer on the board is emptied. It's the host causing that issue.

now all commands only work if the buffer is empty when in M0/M1 state

That's a separate issue, and comes down to the buffer being blocked. If you want such commands to work all the time, enable EMERGENCY_PARSER. That's what it's for.

[Kaibob2]

?
I don't get it. Why does this work

Printer freshly rebooted
Send M1 via serial
Display says "Waiting for User"
Send G1 Z1.000 F300
Send M112 via serial
Printer shuts down

and this doesn't

Printer freshly rebooted
Send M1 via serial
Display says "Waiting for User"
Send G1 Z1.000 F300
Send G1 Z1.000 F300
Send M112 via serial
Nothing happens
Click the button
Printer shuts down

[Kaibob2]

enable EMERGENCY_PARSER. That's what it's for.

EMERGENCY_PARSER is enabled all the time while testing this branch!

In fact i don't need all this emergency parcer stuff. The main reason for me was the skipped buton presses. So, i'm happy with this improvement :)

[Kaibob2]

To be honest, i was printing different calibration stuff all day and 50% of the time, on the inital GCODE which has a M1 in it, there was no beep when i pressed the button. But the click itself was recognized and led to proceed.

[ghost]

I've tested with newest RCBugFix just now.

EMERGENCY_PARSER is enabled:

・Pronterface

m1
m112
hardware reset

No problem

m1
g1 z1 f300
m112
hardware reset

No problem

m1
g1 z1 f300
g1 z1 f300
m112
hardware reset

No problem

m1
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
m112
hardware reset

No problem

・Repetier-Host

m1
m112
hardware reset

endless m112 state

m1
g1 z1 f300
m112
hardware reset

endless m112 state

m1
g1 z1 f300
g1 z1 f300
m112
hardware reset

In this case, nothing happens after m112 and kill() screen is displayed after click a encoder.
And in this case, behavior of hardware reset is inconsistent.
Basically it fall into endless m112 state, but sometimes it's reset properly.
And when machine fall into endless m112 state, if I order a m999, next hardware reset works properly.

[ghost]

EMERGENCY_PARSER is disabled:

・Pronterface

m1
m112
hardware reset

No problem

m1
g1 z1 f300
m112
hardware reset

No problem

m1
g1 z1 f300
g1 z1 f300
m112
hardware reset

In this case, nothing happens after m112 and kill() screen is displayed after click a encoder.
But hardware reset works properly.

・Repetier-Host

m1
m112
hardware reset

In this case, nothing happens after m112 and kill() screen is displayed after click a encoder.
But hardware reset works properly.

m1
g1 z1 f300
m112
hardware reset

Same as above

m1
g1 z1 f300
g1 z1 f300
m112
hardware reset

Same as above

m1
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
g1 z1 f300
m112
hardware reset

Same as above

[thinkyhead]

there was no beep when i pressed the button

Interesting, since the quick_feedback function is called for every successful click.

[ghost]

About this block, Is

     #if BUTTON_EXISTS(ENC)
       #define BLEN_C 2
-      #define EN_C (_BV(BLEN_C))
     #endif
     #define EN_A (_BV(BLEN_A))
     #define EN_B (_BV(BLEN_B))
     #define EN_C (_BV(BLEN_C))

possible?

[thinkyhead]

Yes!