Fixing Rigidbot LCD Panel Support

[japzi]

The LCD panel of Rigidbot printer is broken/not working in current RC. This PR fixes that.
Fixes are based on mikenz's MarlinFirmware/MarlinDev#184

[japzi]

@KiteLab's comment from the closed PR.

 void lcd_buttons_update() {
 #if ENABLED(NEWPANEL)
   uint8_t newbutton = 0;
   if (READ(BTN_EN1) == 0) newbutton |= EN_A;
   #if BTN_EN1 > 0
     if (READ(BTN_EN1) == 0) newbutton |= EN_A;
   #endif
   #if BTN_EN2 > 0
     if (READ(BTN_EN2) == 0) newbutton |= EN_B;
   #endif
   #if ENABLED(RIGIDBOT_PANEL)
     millis_t now = millis();
     if (now > next_fake_encoder_update_ms) {
       if (READ(BTN_UP) == 0) {
         encoderDiff = -1 * ENCODER_STEPS_PER_MENU_ITEM;
       }
       if (READ(BTN_DWN) == 0) {
         encoderDiff = ENCODER_STEPS_PER_MENU_ITEM;
       }
       if (READ(BTN_LFT) == 0) {
         encoderDiff = -1 * ENCODER_PULSES_PER_STEP;
       }
       if (READ(BTN_RT) == 0) {
         encoderDiff = ENCODER_PULSES_PER_STEP;
       }
       next_fake_encoder_update_ms = now + 300;
     }
   #endif

is a bit less time and space consuming.

Calling mills() inside the interrupt is a bad idea.
Why is next_fake_encoder_update_ms used, why not the already existent next_button_update_ms?
Why is ENCODER_STEPS_PER_MENU_ITEM used insted of a consequent ENCODER_PULSES_PER_STEP? If you want to make one menu item per step use #define ENCODER_STEPS_PER_MENU_ITEM 1.

[japzi]

@KiteLab I've changed the code according to your comments in commit 89ab33d98241c2462a7687782ed75b185c4852af .

Code worked with next_button_update_ms instead of next_fake_encoder_update_ms variable. Furthermore, reduced number of mills() calls to one.

Had to bring next_fake_encoder_update_ms = now+30 inside every if condition, to make sure responsiveness (otherwise there is a 300ms delay even w/o a button press).

[Wackerbarth]

It appears that you can still have two calls to millis() in that routine.

And you changes need to be rebased to the current RCBugFix

I'm also concerned the some of your proposed changes will affect others who do not have this particular hardware. For example, your change in cardreader,cpp, would affect anyone who selected the SDEXTRASLOW option.

[japzi]

@Wackerbarth I rebased this to the current RCBugFix
Furthermore, I have introduced a new variable SDULTRASLOW instead of slowing down SDEXTRASLOW variable (in e6cc232). As you've mentioned, that wouldn't affect other printers using the SDEXTRASLOW variable. Does that sound okay? BTW, If I'm not mistaken SDEXTRASLOW was introduced to this specific hardware.

About mills() I could do this:

  #if BTN_ENC > 0 || ENABLED(RIGIDBOT_PANEL)
     millis_t now = millis();
  #endif 

  #if ENABLED(RIGIDBOT_PANEL)
    if (now > next_button_update_ms) {
      if (READ(BTN_UP) == 0) {
        encoderDiff = -1 * ENCODER_STEPS_PER_MENU_ITEM;
        next_button_update_ms = now + 300;
      }
      else if (READ(BTN_DWN) == 0) {
        encoderDiff = ENCODER_STEPS_PER_MENU_ITEM;
        next_button_update_ms = now + 300;
      }
      else if (READ(BTN_LFT) == 0) {
        encoderDiff = -1 * ENCODER_PULSES_PER_STEP;
        next_button_update_ms = now + 300;
      }
      else if (READ(BTN_RT) == 0) {
        encoderDiff = ENCODER_PULSES_PER_STEP;
        next_button_update_ms = now + 300;
      }
    }
  #endif

  #if BTN_ENC > 0
    if (now > next_button_update_ms && READ(BTN_ENC) == 0) newbutton |= EN_C;
  #endif

I just want to confirm with you whether something like above is okay? Not sure #if || works though.

[Wackerbarth]

Yes, the millis() is a better approach.

As for the slowdown, let me suggest that we change the whole thing.

Why do we need SDEXTRASLOW, et. al.

Why not just use one of these in your configuration file when you need the slowdown.

#define SPI_SPEED SPI_HALF_SPEED
#define SPI_SPEED SPI_QUARTER_SPEED
#define SPI_SPEED SPI_SIXTEENTH_SPEED

then, in the cardreader, or Conditionals

#ifndef SPI_SPEED
     #define SPI_SPEED SPI_FULL_SPEED
#endif

[japzi]

Sure, let me do those changes.
BTW, shall I change that in all example configurations?

[KiteLab]

If next_button_update_ms exceeded you still test the buttons in every interrut. next_button_update_ms = now + 300; is needed only once - when if (now > next_button_update_ms) { and should be independent of the result of the buttons tests.

[japzi]

@Wackerbarth
number of millis() calls are reduced to 1.
Got rid of SDSLOW, SDEXTRASLOW and newly introduced SDULTRASLOW. Configurations has to define following if slow speed required. Otherwise full speed assumed.
#define SPI_SPEED SPI_HALF_SPEED

@KiteLab, I think if next_button_update_ms exceeded it does not test the buttons in every interrupt. It checks in every 300ms. Furthermore, when I take that out following button stops working. Is there a way around it?

[AnHardt]

Ah. Now i see. next_button_update_ms is used here as a kind of speed limiter/ debouncer.
The buttons part looks ok to me now.

[AnHardt]

Maybe SET_INPUT is preferable over pinMode. We already have enough problems where Arduino an FastIO pin mapping is confused.

[japzi]

@AnHardt Thank you. I have changed the code to use SET_INPUT instead of pinMode.

[japzi]

@AnHardt, @Wackerbarth & @KiteLab If changes I made are okay, could this be merged please? Thank you.

[AnHardt]

Looks OK to me now.
Sorry - i gave back my commit rights.