ginge / marlin-builder

Ginge's reimplemented from scratch and memory of the Daid's marlin builder web app.
GNU General Public License v2.0
15 stars 7 forks source link

Ulticontroller click encoder rotation direction is reversed #3

Open RedwoodForest opened 10 years ago

RedwoodForest commented 10 years ago

With Marlin built from Daid's builder to increase print speed I would turn the encoder clockwise. With Marlin from this builder I have to turn it counter-clockwise. The same direction swap applies to menu navigation and everything else I've tried. The old way seemed much more intuitive to me.

https://groups.google.com/forum/#!topic/ultimaker/eg2Td3DcF7s

ginge commented 10 years ago

OK Now I am confused.

If I define ULTIMAKERCONTROLLER only, everything works but the knob is inverted. If I define ULTIPANEL only, then it doesn't work at all.

Daid's default configuration.h:

define ULTIMAKERCONTROLLER //as available from the ultimaker online store.

//#define ULTIPANEL //the ultipanel as on thingiverse

clearly the same config option enabled!

There is a significant difference between the LCD code in Ultimaker/Marlin or daid/Marlin and ErikZalm/Marlin branches. Changes were merged in ErikZalm 3 months ago: https://github.com/ErikZalm/Marlin/commit/6a81291c570c356dc0a4154d75efc204cf9f6b04

At this point I am going to go with it's a bug in ErikZalm's branch. The daid/ultimaker branch is a few months behind and uses older code. The dead giveaway is ErikZalms latest patch that changes the pitch of the buzzer to be less annoying (Although I do miss the old chirpy noise)

Can someone check my logic before I send a patch

ginge commented 10 years ago

I have created a pull request in ErikZalm's branch.

https://github.com/ErikZalm/Marlin/pull/587

The previous patch has been modified to this:

if anyone has an oldstyle LCD, just define ULTIMAKERCONTROLLER_DEV in Configuration.h Support for the limited release -dev ulticontrollers will likely not be added to the builder page.

After testing this works for me.

RedwoodForest commented 10 years ago

I just tested and the current version works for me too. I'll let you close the issue once you've resolved the LCD code discussion with Daid.

Thanks!

gr5 commented 10 years ago

This is now fixed in the latest build. I had also created a pull request (not realizing ginge created one too). Erik pulled my merge. I tested the fix and it now works on my ulticontroller (was backwards without the fix). Please close this issue and refer to pull request #594.

ginge commented 10 years ago

Confirmed as working with the patch from #594.

I will await lawrencejohnston's input before closing.

Good work all round!

RedwoodForest commented 10 years ago

I have no problem closing it based on your testing. Thanks to both of you for fixing the issue.

gr5 commented 10 years ago

The saga goes on – just spent 3 hours looking this over again. Lol. But I stand by the original pull request...

https://github.com/ErikZalm/Marlin/pull/594

ginge commented 10 years ago

Agreed. I also just spent 2.5 hours sanity checking Marlin code, your commit and finally verifying with 3 machines I have access to. I stand by your changes.

Thumbs up here.

gr5 commented 10 years ago

The issue isn't "does it work on ulticontroller". The issue is did I break something for other controllers.

The same code is used for 4 or more different types of controllers.

ginge commented 10 years ago

To clarify, the code change works for UltiController and a Paneloulu. The latter using NEWPANEL encoder support.