Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] LPC1768 ZONESTAR ADC KEYPAD SKR V1.3 #14552

Closed
istyszy opened this issue Jul 9, 2019 · 46 comments
Closed

[BUG] LPC1768 ZONESTAR ADC KEYPAD SKR V1.3 #14552

istyszy opened this issue Jul 9, 2019 · 46 comments

Comments

@istyszy
Copy link

istyszy commented Jul 9, 2019

So after hitting a brick wall with I2C Display (for now) i figured out i try with Anet A8 LCD (aka zone-star) Got the wires hooked up on EXP1 (SKR V1.3) :

  • As i know the P1_30 in not 5V tolerant so i added a 3.3V regulator to the ADC keypad power source.
 #if ENABLED(ZONESTAR_LCD)
  #define LCD_PINS_RS       P1_19
  #define LCD_PINS_ENABLE   P1_18
  #define LCD_PINS_D4       P1_20
  #define LCD_PINS_D5       P1_21
  #define LCD_PINS_D6       P1_22
  #define LCD_PINS_D7       P1_23
  #define ADC_KEYPAD_PIN    P1_30
#endif // ZONESTAR_LCD

The result is that the MCU reboots in like 5 seconds when the ADC keypad function in activated in the firmware. Below are some compilation errors .
Capture
does anybody have an idea on this topic?

@p3p
Copy link
Member

p3p commented Jul 9, 2019

Update
The pin assignment issue has been fixed, so you should now use the pin number not the adc channel number


Looks like you need to specify the ADC channel rather than the pin, it's a remnant of how Arduino has analog pins and digital pins numbered the same and should probably be cleaned up, P1_30 would be analog channel 4.

Glad you knew about the ADCs not being 5V tolerant or you would have damaged your board but I'm not sure how well this will function.

@istyszy
Copy link
Author

istyszy commented Jul 9, 2019

Thanks for the info, i have donne some arduino programming, but marlin is a big project, lots of code and i get lost quickly. I will dig in to your perspective. Thanks!

@philocritus
Copy link

Same problem here.
Trying to find a 5v analog pin available for that ADC keypad but there's none available.
@istyszy I realize you connected a 3.3v regulator, so if your changes on the firmware work, let me know so that I can test it aswell

@p3p
Copy link
Member

p3p commented Jul 9, 2019

Trying to find a 5v analog pin available for that ADC keypad but there's none available.

The LPC176x is 5V tolerant on all pins unless they are configured as ADC so there are no 5V analogue pins on any LPC176x board, connecting 5V while in ADC mode will break it.

The pins files use the channel number not the pin number.

Pin Analogue Channel
P0_02 7
P0_03 6
P0_23 0
P0_24 1
P0_25 2
P0_26 3
P1_30 4
P1_31 5

@istyszy
Copy link
Author

istyszy commented Jul 9, 2019

So i did not see the:

#define TEMP_BED_PIN       0   // A0 (T0) - (67) - TEMP_BED_PIN
#define TEMP_0_PIN         1   // A1 (T1) - (68) - TEMP_0_PIN
#define TEMP_1_PIN         2   // A2 (T2) - (69) - TEMP_1_PIN

i tried something like #define TEMP_0_PIN 4 so that i can see on the LCD if the MCU reads the analog values, and it does. there are variation in "temperature" as i press the keys.

is there any way i can output to serial to see what is going on in this part of the code:
Capture

i believe we are making progress.
Thanks for your help!

@p3p
Copy link
Member

p3p commented Jul 9, 2019

So now we have verified the reset was caused by the invalid pin, (I'l have to look at mitigation for that), to make the buttons work you are probably going to have to reduce the adc low pass filter https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/HAL/HAL_LPC1768/HAL.h#L136, (try a value of 2) or it will change slowly between states (I'm going to change the default level of filtering soon)

is there any way i can output to serial to see what is going on in this part of the code

The SERIAL_ECHO should output data over serial?

@istyszy
Copy link
Author

istyszy commented Jul 9, 2019

no change, with the lowpass

i wan to point out that if no key is pressed you get 3.3V on the analog pin.

  #if HAS_ADC_BUTTONS
    static unsigned int raw_ADCKey_value =0 ;
    static bool ADCKey_pressed = false;
  #endif

that 0 should be the max value for the ADC no?

@gloomyandy
Copy link
Contributor

Hi, sorry to interrupt the discussion but @istyszy can you describe exactly how the SKR V1.3 rebooted before you made the changes to remove the reference to pin P1_30 as the ADC_KEYPAD_PIN? Did the system just halt or did it reboot over and over again? I've been talking to @p3p about this sort of issue earlier today and it would help us to better understand a problem seen by some other Marlin users.

@p3p
Copy link
Member

p3p commented Jul 9, 2019

that 0 hould be the max value for the ADC no?

The value the variable is initialised to shouldn't be an issue.

@istyszy
Copy link
Author

istyszy commented Jul 10, 2019

@gloomyandy @p3p Sorry for the delay had some things to do.
So i reproduced the problem i had and i made a video it is easier to see how the board behaves.
i have noticed that all analog reads are canceled. (see the temp for extruder / bed 0/0, it should be -14/0). i also copied in the txt attached all the error during compile.
VIDEO_20190710_181258.zip
CompileLOG.txt
Hope this helps.
if you have any ideas how to fix this please let me know.

@Pavel4e5
Copy link
Contributor

Pavel4e5 commented Jul 10, 2019

Hi! It looks like initialization is missing for the adc keypad pin, I made necessary changes in my repository,
Pavel4e5@76026a9 ,
but not ready for PR yet.
If keypad connected to the pin P1_30, then definition in the file 'pins_BIGTREE_SKR_V1.3.h' should look like this:

#if ENABLED(ZONESTAR_LCD)
...
#define ADC_KEYPAD_PIN 4
...

This configuration works for me.
@istyszy can you test it and tell the result?

Things to do: ADC low pass filter needs to be disabled or adjusted somehow for the ADC keypad, otherwise it makes keys sluggish. For ADC_LOWPASS_K_VALUE I tried values from 1 to 6 with no noticeable difference.

@istyszy
Copy link
Author

istyszy commented Jul 11, 2019

I will try toight. Thanks

@istyszy
Copy link
Author

istyszy commented Jul 11, 2019

@Pavel4e5 You are right, started working but it has delays and then multiple presses... works strange.
i will play around with the settings.
Thanks Man

@Pavel4e5
Copy link
Contributor

These delays and multiple presses occur because the low pass filter makes signal transients more gradual.
I will try to figure out how the filter code works, if I have enough time.
Maybe @p3p would tell us the best way to deal with that code.

@p3p
Copy link
Member

p3p commented Jul 11, 2019

@Pavel4e5 The only thing that needs to change is ADC_LOWPASS_K_VALUE as you say you have, make sure you do a clean and then build just in case.

@Pavel4e5
Copy link
Contributor

@p3p I checked these settings once again with a clean build, as you suggested. Here are the results: With ADC_LOWPASS_K_VALUE = 6 keypad unusable, with 1 it works almost ok, but from time to time it misses fast keypress. If I set ADC_LOWPASS_K_VALUE and ADC_MEDIAN_FILTER_SIZE to 0, then keypad works perfectly, like on the 8bit boards, but this turns off filtering altogether.

@p3p
Copy link
Member

p3p commented Jul 11, 2019

I would only turns the LPF off, the median filter is considerably more important. These filters were added to try and compensate for people coming from AVR based printers with.. lets say less than acceptable wiring (there was a lot of reports). As long as your printers temperature readings are fine when printing there is no problem with disabling it so you can use this keypad.

There is a hardware issue with the LPC176x mcu themselves that means some level of median filtering is required so you should not completely disable that, its unlikely to cause 2 consecutive bad reads so a ADC_MEDIAN_FILTER_SIZE value of 5 may be okay.

I'm currently testing changing the LPF to 2 by default, unfortunately I don't have a printer with dodgy wiring ;) so it's hard to test.

@Pavel4e5
Copy link
Contributor

I will try tinkering with the settings. @p3p, thanks for explaining things to the new contributors!
Unfortunately, I also don't have a printer dodgy enough, so can't help with testing :)

@istyszy
Copy link
Author

istyszy commented Jul 11, 2019

I will test for you but i have some other problems. Maybe in the weekend.

@istyszy
Copy link
Author

istyszy commented Jul 15, 2019

@p3p @Pavel4e5 Works pretty well:
#define ADC_MEDIAN_FILTER_SIZE (5)
#define ADC_LOWPASS_K_VALUE (2)
THANKS!

@powertomato
Copy link

I started working on enabling ZONESTAR_LCD for my SKR v1.3 board as well. Only after I made the same mistake as @istyszy (specifying the pin instead of ADC channel) I came across this issue.

I have a slightly different pin assignment. I like it because it's the same as the LCD pinout, just mirrored horizontally. In my opinion, it made creating an adapter cable much easier:
powertomato@caf46bd

                 _____
             5V | · · | GND
        D4 1.23 | · · | 1.22
        D5 1.21 | · ·   1.20 LCD_RS
        D6 1.19 | · · | 1.18 LCD_EN
        D7 0.28 | · · | 1.30 ADC_KEYPAD
                 -----
                 EXP1
                 _____
             D7 | · · | ADC_KEYPAD
             D6 | · · | LCD_EN
             D5 | · ·   LCD_RS
             D4 | · · | NC
            +5V | · · | GND
                 -----
              ZONESTAR LCD

@MikRoscope
Copy link

  • As i know the P1_30 in not 5V tolerant so i added a 3.3V regulator to the ADC keypad power source.

Could you please provide more details on how to do this? Which pin is the ADC power source?

I have a LCD from Anet A8 which I am trying to connect to SKR v1.3. So far I only get dim flashes on the lcd.

@powertomato
Copy link

@MikRoscope Here is a schematic of the display: https://i.imgur.com/DIEwA4i.png
On the right side you see the buttons. When no button is pressed 5V go through a 4.7k resistor to the button pin, when a button is pressed it forms a voltage divider. To get the same values you need to cut that 5V connection and add a 3.3V regulator like this:

 /|\ +5V      /|\ +5V ___           
  |            +-----[_ _]---- GND  
  x cut                |   
  |            +-------+ 3.3V out   
[4k7]        [4k7]          
  |            |        

@MikRoscope
Copy link

MikRoscope commented Jul 20, 2019

@MikRoscope Here is a schematic of the display: https://i.imgur.com/DIEwA4i.png
On the right side you see the buttons. When no button is pressed 5V go through a 4.7k resistor to the button pin, when a button is pressed it forms a voltage divider. To get the same values you need to cut that 5V connection and add a 3.3V regulator like this:

 /|\ +5V      /|\ +5V ___           
  |            +-----[_ _]---- GND  
  x cut                |   
  |            +-------+ 3.3V out   
[4k7]        [4k7]          
  |            |        

Thanks a lot! I added a voltage regulator (and a switch to maintain 5V mode when needed https://imgur.com/a/R2ed44E). I also made a terrifying adapter (https://imgur.com/a/58mBi5B) to mirror the connection since my Anet A8 LCD had pin-out similar to yours. The LCD is now working and shows the usual text (https://imgur.com/a/CrQD6S5). However, pressing the buttons has no effect. I cant find where the changes in ADC_KEYPAD_PIN voltage is coded to represent menu actions.

Edit: Using
#define ADC_MEDIAN_FILTER_SIZE (5)
#define ADC_LOWPASS_K_VALUE (2)
the keypad is working, however missed lots of key press.

@MikRoscope
Copy link

Ok, I got it working. To summarize, I added
#if HAS_ADC_BUTTONS
HAL_ANALOG_SELECT(ADC_KEYPAD_PIN);
#endif
to /src/module/temperature.cpp as @Pavel4e5 mentioned. Physically mirroring the connection as @powertomato explained and this pin configuration

#if ENABLED(ZONESTAR_LCD)
#define LCD_PINS_RS P1_20
#define LCD_PINS_ENABLE P1_18
#define LCD_PINS_D4 P1_23
#define LCD_PINS_D5 P1_21
#define LCD_PINS_D6 P1_19
#define LCD_PINS_D7 P0_28
#define ADC_KEYPAD_PIN 4
#endif

in \src\pins\pins_BIGTREE_SKR_V1.3.h. Then modifying
#define ADC_MEDIAN_FILTER_SIZE (2)
#define ADC_LOWPASS_K_VALUE (2)
in \src\HAL\HAL_LPC1768\HAL.h.

The menu does not have any SDCard item. Is that normal? How can I print from stored GCode?

@istyszy
Copy link
Author

istyszy commented Jul 21, 2019 via email

@atreubig49
Copy link

Ok, Where did you place

#if HAS_ADC_BUTTONS
HAL_ANALOG_SELECT(ADC_KEYPAD_PIN);
#endif
in temperature.cpp ????

@istyszy
Copy link
Author

istyszy commented Jul 21, 2019

#14552 (comment)
U can find the info on pavel's git fork.

@powertomato
Copy link

powertomato commented Jul 22, 2019

@atreubig49 @istyszy it is actually already merged upstream, if you download the latest nightly (or pull the changes if you cloned the repository) it should already be there.


I finally got around to fiddle with the filter settings and can confirm

#define ADC_MEDIAN_FILTER_SIZE (5)
#define ADC_LOWPASS_K_VALUE (2)

works for me.

@boelle
Copy link
Contributor

boelle commented Jul 22, 2019

nice to see another one solved, @istyszy can you confirm?

@istyszy
Copy link
Author

istyszy commented Jul 22, 2019

Yes, zonestar lcd is working !

@boelle
Copy link
Contributor

boelle commented Jul 22, 2019

oki, will close then :-D

@boelle boelle closed this as completed Jul 22, 2019
@p3p
Copy link
Member

p3p commented Jul 22, 2019

As happy as I am that the ADC keypads work when converted to 3.3V, I'm worried people will see that they are "supported" and start just plugging them in which will cause damage to the LPC176x MCU ADC channels.

I'm not sure where this can be documented so users see the compatibility issue before trying it out and look into it after its too late.

@powertomato
Copy link

@p3p I was thinking about that, as I originally planned to create a pull request, but decided against it for that exact reason.

These were my thoughs on how one could fool-proof it a bit:

LPC176xs ADC pins are only 5V intolerant if configured as ADC. So the damage will only occur if the users actively configure the pin as ADC i.e. define ADC_KEYPAD_PIN otherwise a compile error would occur. One could catch that from happening and show a more meaningful compile error. Something like:

--- a/Marlin/src/module/temperature.cpp
+++ b/Marlin/src/module/temperature.cpp
@@ -1685,6 +1685,12 @@ void Temperature::init() {
     HAL_ANALOG_SELECT(FILWIDTH_PIN);
   #endif
   #if HAS_ADC_BUTTONS
+    #ifndef ADC_KEYPAD_PIN
+      #error WARNING: ADC-keypads REQUIRE a hardware modification on 3.3V devices and are \
+             not officialy supported. As ADC_KEYPAD_PIN not defined, most likely you're on \
+             a 3.3V device. Only proceed if you know what you are doing or you will damage \
+             your board!
+    #endif
     HAL_ANALOG_SELECT(ADC_KEYPAD_PIN);
   #endif

One can only hope that people know the risks if they start fiddling with the pin-settings by themself. But even for those who don't, HAS_ADC_BUTTONS could be redefined to something like
CURRENT_HAS_ADC_BUTTONS && (I_KNOW_I_HAVE_TO_MODIFY_MY_ADC_KEYPAD || NOT_A_3V3_BOARD)
and prevent the pin from beeing configured as ADC.

In any way I would label this issue as "wont fix", that implies there is some work required to get these displays to work.

@istyszy
Copy link
Author

istyszy commented Jul 23, 2019

  • Maybe we can define this board with different name? Zonestar 3.3V verion ?

Your approach is also great but i think people who thinker around to rewire the display
should have the minimum knowledge to fix this problem. (mod the board).
This is a difficult choice.

  • Or we can leave it out complacently from pins_BIGTREE_SKR_V1.3.h and people who know what they are doing can add it manually.

Anyway there should be a place where you can find mods and documentation on this subject.
With pictures and stuff. ( the main problem was fixed, if you want is simple to integrate into marlin this display on this MCU)

@mmrvelj
Copy link

mmrvelj commented Jul 24, 2019

I am having same issue, trying make Anet A8 display work on SKR V1.3. Display is functional but keys are not.
Now that you broke the ice and already solved this, I would like to see how this can be done with minimal changes on hardware.

I am thinking of two approaches:

  1. desoldering R7 resistor from LCD and replacing it with external circuit that would include a voltage regulator (module?) (such as AMS1117) and then R7 resistor. It would hook to the two R7 soldering points + ground, and thus no need to cut the connection.

  2. letting keys work on 5V (no changes on the display module) and then reducing the output (from ADC_KEYPAD pin) proportionally. I am not sure what kind of circuit would work here - perhaps even simple additional resistor divider?

I am curious would these approaches work, and which one would be better?

@powertomato
Copy link

@mmrvelj I went with #1 without desoldering R7. On my unit (might be different on others) the 5V line is quite long and wide. Enough to cut it and scratch away the soldermask on the two ends then solder Vin and Vout of an SMD voltage regulator. As for GND I chose a regulator that has a heatsink connected to GND (LM7833), I scratched away some of the soldermask on the GND-plane and solderd it to that.
Technically thats using it out of spec, because it needs capcacitors on Vin and Vout, but there is even room for those, and I don't feel like an ADC keypad needs really stable voltage: I'm ok with occasional ghost inputs or missed key presses.

@mmrvelj
Copy link

mmrvelj commented Jul 25, 2019

@powertomato thanks for the feedback.

I went also with option 1 as it looked safer. Here is the picture of my setup and I also confirm that the approach works. It requires changes mentioned by @MikRoscope and others
in /src/module/temperature.cpp ,
in /src/pins/pins_BIGTREE_SKR_V1.3.h and
in /src/HAL/HAL_LPC1768\HAL.h

Module is the one I had laying around identical to this one.

After I removed black plastic distancer from the module and bent pins in order to make everything flatter, I also desoldered LED that would otherwise be on all the time. I had to put different SMD resistor (on the leg OUT), because I lost the original one (it flew away somewhere :) ), the replacement I had is a bit smaller in size (also 4.7k).

@milkpirate
Copy link
Contributor

Another option, adding a voltage devicder to the output, maybe simpler:

IMG_20191124_152103

@p3p
Copy link
Member

p3p commented Nov 24, 2019

I should probably mention here that the original pin assignment issue has been fixed, so you should now use the pin number (Px_xx) not the adc channel number when setting the ADC_KEYPAD_PIN.

@JunctionRunner
Copy link

JunctionRunner commented Dec 22, 2019

@milkpirate @powertomato Hey guys, so I'm pretty darn tired writing this, but I just ran into this issue trying to get my anet a plus back up and running.

It looks like the a8 plus uses a different board design to the one that you guys have, and has quite a few resistors unpopulated.

One thing I'm not totally clear on @milkpirate , are those two surface mount resistors you added on the only change that needs to be made with the pin assignments now changed? Ideally I'd like to use the original cables as I have no IDC connectors on hand to make modifications.

IMG_20191222_054702~01

edit: So one thing I think is different to your boards is that you guys have the five buttons, however I have the jogwheel that you can click, along with a reset button.

edit 2 actually I think this thread isn't applicable to me at all, and I just have to do a bunch of rewiring (unfortunately seems like more work)

@antonvier
Copy link

Hello! @milkpirate I made the change that you suggested with the voltage divider, but I can't make anything appear on the display, the printer is ok and apparently the problem is in the cable, because I changed all the parameters mentioned. Could any of you give me a "how to for dummies"?
thank you all.

@milkpirate
Copy link
Contributor

@XOIIO @antonvier modification on the board itself: yes (higher values are better, keep the ratio). sadly this isnt all I had to do. the cable pinout needed a modification as well: I had to swap every other pin on one connector:

IMG_20200417_202432
IMG_20200417_202452

But I still had a lot of trouble with missed button presses... so I stopped and went for another display board.

@antonvier
Copy link

thank's a lot @milkpirate!

@Warden765
Copy link

I did the manipulation but I have an error in marlin :
{
"resource": "/c:/Users/phili/Desktop/Marlin-bugfix-2.0.x/Marlin/src/gcode/queue.cpp",
"owner": "cpp",
"severity": 4,
"message": "'char* strcpy(char*, const char*)' accessing 1 byte at offsets 0 and [0, 64] may overlap 1 byte at offset 0 [-Wrestrict]",
"startLineNumber": 220,
"startColumn": 9,
"endLineNumber": 220,
"endColumn": 9
} someone what is that?

@github-actions
Copy link

github-actions bot commented Jul 2, 2020

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests