Marlin: Menu navigation is slow on Anet A8

Created on 30 Oct 2017 · 13Comments · Source: MarlinFirmware/Marlin

Bug Report

Description: menu navigation is slow on Anet A8 (with 2004 display and ADC keypad)
Expected behaviour: to scroll the menu items faster
Actual behaviour: even if you hold the up/down buttons, it takes ~0.5s to scroll through each menu item.
How to fix it: find the "inline bool handle_adc_keypad()" definition in ultralcd.cpp, and comment the "lcd_quick_feedback();" line.
It seems lcd_quick_feedback(); is trying to beep using the buzzer on each "step", and this adds 500ms delay, making it impossible to scroll faster.

    inline bool handle_adc_keypad() {
      static uint8_t adc_steps = 0;
      if (buttons_reprapworld_keypad) {
        if (adc_steps < 20) ++adc_steps;
//         lcd_quick_feedback();
        lcdDrawUpdate = LCDVIEW_REDRAW_NOW;
        if (encoderDirection == -1) { // side effect which signals we are inside a menu
          if      (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_DOWN)  encoderPosition -= ENCODER_STEPS_PER_MENU_ITEM;
          else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_UP)    encoderPosition += ENCODER_STEPS_PER_MENU_ITEM;
          else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_LEFT)  menu_action_back();
          else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_RIGHT) lcd_return_to_status();
        }

The Anet A8 does not have a buzzer anyway.

Confirmed ! LCD & Controllers

Source

lokster

Most helpful comment

It works fine even when the call to lcd_quick_feedback() is disabled - I don't see any side effects (debouncing problems, etc.). See the video https://www.youtube.com/watch?time_continue=24&v=L-OXU_XCHUI
But if it's really needed, I think 100ms should be a good trade off - it's still very responsive that way. 300 seems slow. Compare it to the keyboard repeat rate on a regular PC - it's much faster.
As I said - the Anet A8 display does not have a buzzer, so I'm not sure why this function is called at all.
Why not make it

#ifndef ANET_KEYPAD_LCD
lcd_quick_feedback()
#endif

Another issue, which might or might not be related, is that when clicking the buttons (not just holding them pressed) the navigation is also slow.
In fact, if I start clicking, for example, the DOWN button fast enough it stops doing anything. The menu does not move. And I'm not clicking it that fast - just 2-3 times per second.
It seems like debouncing issue, but I am not yet that deep into the code, so I'm not exactly sure.
If you can point me in the right direction, I will test it.
Just for a perspective: Repetier running on the same mainboard / LCD has very responsive menu, no matter how fast you click.

lokster on 31 Oct 2017

👍3

All 13 comments

I've made a patch making lcd_quick_feedback only fire on screen changes. However, it also sets next_button_update_ms to wait 300ms to match what other keypads do. Try tweaking that delay until you find one that is reliable. Maybe no delay is needed…?

inline bool handle_adc_keypad() {
  #define ADC_MIN_KEY_DELAY 300
  static uint8_t adc_steps = 0;
  if (buttons_reprapworld_keypad) {
    if (adc_steps < 20) ++adc_steps;
    lcdDrawUpdate = LCDVIEW_REDRAW_NOW;
    if (encoderDirection == -1) { // side effect which signals we are inside a menu
      if      (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_DOWN)  { encoderPosition -= ENCODER_STEPS_PER_MENU_ITEM; next_button_update_ms = millis() + ADC_MIN_KEY_DELAY; }
      else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_UP)    { encoderPosition += ENCODER_STEPS_PER_MENU_ITEM; next_button_update_ms = millis() + ADC_MIN_KEY_DELAY; }
      else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_LEFT)  { menu_action_back(); lcd_quick_feedback(); }
      else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_RIGHT) { lcd_return_to_status(); lcd_quick_feedback(); }
    }
    else {
      if (buttons_reprapworld_keypad & (EN_REPRAPWORLD_KEYPAD_DOWN|EN_REPRAPWORLD_KEYPAD_UP|EN_REPRAPWORLD_KEYPAD_RIGHT)) {
        const int8_t step = adc_steps > 19 ? 100 : adc_steps > 10 ? 10 : 1;
             if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_DOWN)  encoderPosition += ENCODER_PULSES_PER_STEP * step;
        else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_UP)    encoderPosition -= ENCODER_PULSES_PER_STEP * step;
        else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_RIGHT) encoderPosition = 0;
        next_button_update_ms = millis() + ADC_MIN_KEY_DELAY;
      }
    }
    #if ENABLED(ADC_KEYPAD_DEBUG)
      SERIAL_PROTOCOLLNPAIR("buttons_reprapworld_keypad = ", (uint32_t)buttons_reprapworld_keypad);
      SERIAL_PROTOCOLLNPAIR("encoderPosition = ", (uint32_t)encoderPosition);
    #endif
    return true;
  }
  else if (!thermalManager.current_ADCKey_raw)
    adc_steps = 0; // reset stepping acceleration

  return false;
}

thinkyhead on 30 Oct 2017

#ifndef ANET_KEYPAD_LCD
lcd_quick_feedback()
#endif

lokster on 31 Oct 2017

👍3

Just tested your patch. 300 is indeed slow. 100 much better if not the same as when the call to lcd_quick_feedback() is disabled.
There's still the issue with the slow clicking though.

lokster on 31 Oct 2017

I figured 300 was probably slow. At 100 we'll get 10 menu lines per second, which is the kind of keyboard repeat I'm used to.

The debounce issue makes sense. I'll check how the debouncer works for other keypads and see if it was simply overlooked for this one, since it has its own set of custom routines.

Good to know the ANET has no buzzer. I think the better condition to test would be #if PIN_EXISTS(BEEPER) ... lcd_quick_feedback() in case users connect a piezo to a different pin or have one built into their board.

thinkyhead on 31 Oct 2017

Yes, I agree - 10 per sec sounds just about right.
The PIN_EXISTS(BEEPER) makes sense.
I've checked the pins_ANET_10.h and I see the BEEPER_PIN is defined only when using ANET_FULL_GRAPHICS_LCD, which is correct - only the full graphics lcd has beeper.

lokster on 31 Oct 2017

👍1

See if this is any better…

inline bool handle_adc_keypad() {
  #define ADC_MIN_KEY_DELAY 100
  static uint8_t adc_steps = 0;
  if (buttons_reprapworld_keypad) {
    if (adc_steps < 20) ++adc_steps;
    lcdDrawUpdate = LCDVIEW_REDRAW_NOW;
    if (encoderDirection == -1) { // side effect which signals we are inside a menu
      if      (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_DOWN)  { encoderPosition -= ENCODER_STEPS_PER_MENU_ITEM; }
      else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_UP)    { encoderPosition += ENCODER_STEPS_PER_MENU_ITEM; }
      else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_LEFT)  { menu_action_back(); lcd_quick_feedback(); }
      else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_RIGHT) { lcd_return_to_status(); lcd_quick_feedback(); }
    }
    else {
      if (buttons_reprapworld_keypad & (EN_REPRAPWORLD_KEYPAD_DOWN|EN_REPRAPWORLD_KEYPAD_UP|EN_REPRAPWORLD_KEYPAD_RIGHT)) {
        const int8_t step = adc_steps > 19 ? 100 : adc_steps > 10 ? 10 : 1;
             if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_DOWN)  encoderPosition += ENCODER_PULSES_PER_STEP * step;
        else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_UP)    encoderPosition -= ENCODER_PULSES_PER_STEP * step;
        else if (buttons_reprapworld_keypad & EN_REPRAPWORLD_KEYPAD_RIGHT) encoderPosition = 0;
      }
    }
    #if ENABLED(ADC_KEYPAD_DEBUG)
      SERIAL_PROTOCOLLNPAIR("buttons_reprapworld_keypad = ", (uint32_t)buttons_reprapworld_keypad);
      SERIAL_PROTOCOLLNPAIR("encoderPosition = ", (uint32_t)encoderPosition);
    #endif
    next_button_update_ms = millis() + ADC_MIN_KEY_DELAY;
    return true;
  }
  else if (!thermalManager.current_ADCKey_raw)
    adc_steps = 0; // reset stepping acceleration

  return false;
}

With this change I'm going on the theory that this piece of hardware has its own key debouncing. See what happens when you hold down the select button. Does it go too fast, or…?

thinkyhead on 31 Oct 2017

Also, please test value editing and axis movement in the sub-menus to make sure it's not too easy to miss the target.

thinkyhead on 31 Oct 2017

OK, will test tomorrow.

lokster on 31 Oct 2017

Looking forward to it! Definitely want to get this right before we merge.

thinkyhead on 1 Nov 2017

Odd... I downloaded, configured & compiled https://github.com/thinkyhead/Marlin/tree/96fbce60ace16d56632d6fc0c96874ccb2893b05, but my SD card stopped working. It works OK with the 1.1.6 release.
So I just applied the changes in ultralcd.cpp to 1.1.6 manually and tested it.
Scrolling / editing values / positioning while holding the buttons looks OK to me.
However, now that everything it more responsive and faster, it makes this unneeded:

const int8_t step = adc_steps > 19 ? 100 : adc_steps > 10 ? 10 : 1;

It works better when steps is allways 1.
One more thing, not sure if it's related (it was always like that): when I move some axis in 0.1mm increments, it moves on every step. Which is OK, and what I expect.
However, when I'm moving it in 1mm or 10mm increments, it starts to move only after I release the key.

lokster on 1 Nov 2017

However, when I'm moving it in 1mm or 10mm increments, it starts to move only after I release the key.

That is by design, and needed because we are abusing the planner by allowing direct injection of moves from the LCD. The old behavior was much worse, as it would just ignore your input if the planner got full.

thinkyhead on 3 Nov 2017

I just loaded this last night, I think "select" should be excluded from auto repeat. I'ts a bit twitchy especially when you have babystepping via double click turned on.