search

MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.03k stars 19.12k forks source link

[BUG] VIKI LEDs don't work. #19809

Open nophead opened 3 years ago

nophead commented 3 years ago

LCD_HAS_STATUS_INDICATORS is set here: https://github.com/MarlinFirmware/Marlin/blob/92076c65601de5fdfe0facf32dd60c57aab05747/Marlin/src/lcd/HD44780/ultralcd_HD44780.h#L50

But isn't visible here: https://github.com/MarlinFirmware/Marlin/blob/e8177735a00d1d7bb20b439544d3324fe01cfe7e/Marlin/src/lcd/ultralcd.h#L304

or here: https://github.com/MarlinFirmware/Marlin/blob/8e1ea6a2fa1b90a58b4257eec9fbc2923adda680/Marlin/src/lcd/ultralcd.cpp#L892

Because ultralcd_HD44780.h is not included. So the status LEDs don't get updated.

ellensp commented 3 years ago

Please attach your configuration files.

nophead commented 3 years ago

Why does that make any difference to a scope bug? I expect if I add LCD_HAS_STATUS_INDICATORS to my configuration it will work around the problem but that makes the line in ultralcd_HD44780.h pointless.

Marlin.zip

Looks like it was broken 14 months ago and will affect Panelolu2 as well.

sjasonsmith commented 3 years ago

Why does that make any difference to a scope bug?

For anybody hoping to help fix this, the configuration files allow them to start in exactly the same scenario you are using. Most people looking at this will probably say "What is a Viki?" and have to start fumbling to figure out how to configure it.

With configuration files provided, it is trivial to use your files, or to diff them against the default configs to know what you changed.

sjasonsmith commented 3 years ago

If you already know a solution I'd encourage you to file a pull request to the bugfix-2.0.x branch to resolve it.

ellensp commented 3 years ago

Completely wrong, ignore this post.

nophead commented 3 years ago

I tried that earlier and it didn't work. How could it when that file isn't included in ultralcd.h and ultralcd.cpp? And VS code showed me it wasn't active in the ultralcd.h. The TERN macro makes it impossible to see in ultralcd.cpp and confuses VS code. When I removed the TERN macro it said update_indicators() was undefined which made me look at ultralcd.h.

Adding just #define LCD_HAS_STATUS_INDICATORS in configuration.h does work but I don't think that is the intention of the code.

ellensp commented 3 years ago

try adding the following to ultralcd.h line 34

#if ENABLED(LCD_I2C_TYPE_MCP23017)
  #include "../lcd/HD44780/ultralcd_HD44780.h"
#endif
nophead commented 3 years ago

Yes that does work.

The bed light blinks when the LCD is updated. That is the one that shares the B port with the LCD signals, so I expect there is a bug somewhere that doesn't merge in the top bit.

The up / down and left buttons don't do anything but the right button does the same as click. Are the VIKI I2C buttons supposed to be implemented or not?

ellensp commented 3 years ago

Had a good look through the code, It doesn't seem to have any code to do any actions for the other buttons.

But it looks like it should be simple enough to add.

NB This is untested, other than compiling, as I don't have any compatible hardware to try it on.

Find this code in Marlin/src/lcd/ultralcd.cpp for me it is at line 1242, but your might be different.

      #if HAS_ADC_BUTTONS
        if (keypad_buttons == 0) {
          const uint8_t b = get_ADC_keyValue();
          if (WITHIN(b, 1, 8)) keypad_buttons = _BV(b - 1);
        }
      #endif

Add this following block of code after the above. 

      #if ENABLED(LCD_I2C_VIKI)
        const int8_t pulses = epps * encoderDirection;
        if (buttons & B_UP) //up 
          encoderDiff = (ENCODER_STEPS_PER_MENU_ITEM) * pulses;
        if (buttons & B_DW) //down 
          encoderDiff = -(ENCODER_STEPS_PER_MENU_ITEM) * pulses;
        if (buttons & B_LE) //left 
          encoderDiff = -pulses;
        if (buttons & B_RI) //right 
          encoderDiff = pulses;
      #endif

Now I suspect this will conflict with current right button code. So we need to stop the right button acting like a click. In Marlin/src/lcd/HD44780/ultralcd_HD44780.cpp find this code

      #if ENABLED(LCD_I2C_VIKI)
        if ((slow_bits & (B_MI | B_RI)) && PENDING(millis(), next_button_update_ms)) // LCD clicked
          slow_bits &= ~(B_MI | B_RI); // Disable LCD clicked buttons if screen is updated
      #endif

And change to

      #if ENABLED(LCD_I2C_VIKI)
        if ((slow_bits & B_MI) && PENDING(millis(), next_button_update_ms)) // LCD clicked
          slow_bits &= ~B_MI; // Disable LCD clicked buttons if screen is updated
      #endif
nophead commented 3 years ago

Thanks, that works with a couple of tweaks. The direction is reversed. I.e up should be the negative one and down positive. The click has to be very short or it moves multiple positions. Adding next_button_update_ms = now + 300; like some of the code above fixes that.

I would prefer right to be as it was (select) and left to go up one menu level because that is cumbersome with the wheel. Using arrows to move around the menus and the wheel to adjust values makes more sense to me. It doesn't seem to be how other buttons are handled though.

Another bug seems to be that the LEDs don't initialise correctly due to ledsprev being initialised to 0 here: https://github.com/MarlinFirmware/Marlin/blob/e8177735a00d1d7bb20b439544d3324fe01cfe7e/Marlin/src/lcd/HD44780/ultralcd_HD44780.cpp#L1073 It should be an impossible value, i.e. bigger than 7, so that it always updates the leds the first time it runs.

It looks nobody else uses VIKI or Panelolu2 with Marlin 2.0. I don't know if it worked properly with older Marlins because I haven't tried it until now but Panelolu2 did. That doesn't have the extra buttons though but it does have the I2C LEDs.

ellensp commented 3 years ago

most VIKI or Panelolu2 would have been on 644p's based controllers. Which is to small for Marlin2... so I suspect most are still running old firmware or in a parts draw... (I will add a VIKI lcd to my collection, if the opportunity presents itself)

No other controller has 4 buttons and a encoder with button that I know of... so there is no 'standard' definition for what buttons should do what.

So lets see if we can get left working as a back button. (standard disclaimer, complies, cannot test without hardware)

In Marlin/src/lcd/ultralcd.h around line 222 is

#if BUTTON_EXISTS(BACK) || EITHER(HAS_TOUCH_XPT2046, IS_TFTGLCD_PANEL) 
  #define BLEN_D 3
  #define EN_D _BV(BLEN_D)
  #define LCD_BACK_CLICKED() (buttons & EN_D)
#else
  #define LCD_BACK_CLICKED() false
#endif

Change it to

#if BUTTON_EXISTS(BACK) || EITHER(HAS_TOUCH_XPT2046, IS_TFTGLCD_PANEL) 
  #define BLEN_D 3
  #define EN_D _BV(BLEN_D)
  #define LCD_BACK_CLICKED() (buttons & EN_D)
#elif ENABLED(LCD_I2C_VIKI)
  #define LCD_BACK_CLICKED() (buttons & B_LE)
#else
  #define LCD_BACK_CLICKED() false
#endif

make sure to remove these lines of code we added before to Marlin/src/lcd/ultralcd.cpp

        if (buttons & B_LE) //left 
          encoderDiff = -pulses;
        if (buttons & B_RI) //right 
          encoderDiff = pulses;

and check the right button is set as a select again in Marlin/src/lcd/HD44780/ultralcd_HD44780.cpp

      #if ENABLED(LCD_I2C_VIKI)
        if ((slow_bits & (B_MI | B_RI)) && PENDING(millis(), next_button_update_ms)) // LCD clicked
          slow_bits &= ~(B_MI | B_RI); // Disable LCD clicked buttons if screen is updated
      #endif
nophead commented 3 years ago

Yes that works, many thanks.

Our Mendel90 kits had Melzis with 1280P in them. I don't think Panelulo2 would fit in a 644P even with old Marlin.

This machine is actually the first prototype Mendel90 that hasn't been run for 8 years with a bed and cabinet recycled from my Sell's Mendel and a RAMPS board. I was pleased to find Marlin supports chamber heaters and lighting. The code seems in much better shape and doesn't seem any bigger than the old version with the same features enabled.

nophead commented 3 years ago

I noticed my wheel was working backwards and when I reversed that I had to put the signs of up and down back to your original. Very confusing.

nophead commented 3 years ago

The reason the bed LED flickers and also gets turned off when it should be on is because inti_lcd() is called more than once. For example here:

https://github.com/MarlinFirmware/Marlin/blob/8e1ea6a2fa1b90a58b4257eec9fbc2923adda680/Marlin/src/lcd/ultralcd.cpp#L573 I fixed it by setting NO_LCD_REINIT in configuration.h. Perhaps it needs to be set when the LCD has LEDs. Either that or the LiquidTWI2.cpp library would need to be modified to preserve the LED status when begin() is called and only initialise _backlightBits in the constructor.

ellensp commented 3 years ago

In Marlin/src/inc/Conditionals_post.h is

#if ANY(HAS_GRAPHICAL_TFT, LCD_USE_DMA_FSMC, HAS_FSMC_GRAPHICAL_TFT, HAS_SPI_GRAPHICAL_TFT) || !PIN_EXISTS(SD_DETECT)
  #define NO_LCD_REINIT 1  // Suppress LCD re-initialization
#endif

So we just need to add LCD_I2C_VIKI into the list

nophead commented 3 years ago

And PANALOLU2 presumably and any other I2C boards with LEDs, if there are any.

Seems a bit odd to have a bodge to get around poor wiring enabled by default anyway. I have never experienced the display getting corrupted but I don't run the cables near the stepper or bed wiring.

ellensp commented 3 years ago

not touching PANALOLU2 till have access to one to test. one thing at a time...

nophead commented 3 years ago

The LEDs and the LCD are connected identically on the Panelolu2 and driven through the same library. The difference is it has the encoder inputs connected to the I2C in place of the direction buttons of the VIKI and the VIKI has direct connections for the encoder.

I can probably make a PR and test against both but it will have to wait until next April because I will be away from my machines until then.

boelle commented 3 years ago

I can probably make a PR and test against both but it will have to wait until next April because I will be away from my machines until then.

that is why i added the no locking label... just my 0.02$

sjasonsmith commented 3 years ago

that is why i added the no locking label... just my 0.02$

I was wondering why you did that. It will still close as stale if inactive, no-locking would just prevent it from being eventually locked such that you'd have to repot a new bug to re-awaken it.

boelle commented 3 years ago

oh, i thought it would work as a kind of hands off for the bot :-/

sjasonsmith commented 3 years ago

No, @thinkyhead has stated that he doesn't want any issues exempt from the stale-bot, they need at least an occasional poke to keep them open. All it takes is any comment once a month to stave off the bot.

boelle commented 3 years ago

just a comment to stave off the bot

ellensp commented 3 years ago

I have the required i2c device in transit... this is still in progress . Sadly I suspect it has gotten lost, as so many things sent from china get lost when trying to get here..

ellensp commented 3 years ago

order 3 different lots of i2c devices, different suppliers, different sites, Nothing from aliexpress got through. however the one from ebay did, a few days ago. now to find some time to get back into this.

ellensp commented 3 years ago

still working on working on it.