Behavior of the "Encoder" module is unexpected - two call per notch

Hello,
I tried to use the Encoder module with a rotary encoder (http://www.amazon.com/dp/B00HSX9ZB2).

var step = 0;
require("Encoder").connect(A1,A2,functio­n (direction) {
    step += direction;
    print(step);
});

step will be incremented/decremented by 2 on every notch. This is not expected when looking at the api but understandable when looking at the implementation. The state machine 00 → 01 → 11 → 11 → .. calls onChange() also in the instable states 01 and 10.

This is academically correct for general gray codes. But I think that is not the intended behavior for supporting an incremental rotary encoder because client code will now have to skip every second callback. What is your opinion? Who is using this module? How do you deal with these circumstances? Do you use the "Encoder" module for something other than rotary encoders?

I made an adaption with a more "rotary encoder" behavior - it's attached. If desired I could make a pull request with some additional debounce logic. Debouncing is now possible because of the internal "forbidden" states.

BTW there is a small bug in the initialization code of the original module. The first step will not be detected correctly. I fix it if the attached new code should not be used.

1 Attachment

• Encoder.js

Hmm, it's difficult. When I did it, I'd assumed that my particular encoder was non-standard in that it effectively jumped two positions for each 'click', but I haven't yet seen an encoder that doesn't do that :)

Of course my problem is that the module is now in use by people, who presumably have worked around that behaviour.

I think it's probably correct to change it though - and hopefully users can just remove the /2 from their code. Or maybe instead of returning +1 and -1 it could actually return +2 and -2? It might then work fine for everyone?

Your code looks great! I just tried it on an encoder here, and it still calls the callback twice for each 'click'. Is that the same for you?

The old one called it 4 times though!

After you mentioned an encoder with four callback per click I looked for different types of encoders: The topic is more complicated than I thought.

1. There are encoders without any notch/click. The current implementation would be the best for these types.
   
2. There are encoders with click and the switches toggle BETWEEN the stable positions. This is the most common case I think.
   
3. There are encoders where one switch is at the stable click position. Here the pins are not symmetric and must be handled differently. Perhaps your last mentions encoder is of this type? Do you know the name of your 4-steps-per-click encoder?
   
4. ??? Maybe there are even more?
   
   

Espruino is such a great platform so we should supported each type out of the box with simple parameterization and not choose one in favor of the others. The principle of a rotary encoder is the same only the state machine differs.

I have created a small pull request to initialize the current module correctly.
https://github.com/espruino/EspruinoDocs­/pull/187 → Solution for 1)

Next I will play a little bit with different rotary encoders. I will try to merge the current module code with the attached version and generalize the module. The current module behavior (+/-2) could then be the default one. Existing clients will not be broken. And the documentation should point to the parameterization possibilities.

What do you think? Should I try this?

Thanks for the PR!

This is the first one I found when I just googled for rotary encoder: http://uk.rs-online.com/web/p/products/7­377767/

I think it's like the one I have.

It'd be great if you could look at it - your code was much tidier! Perhaps (to be backwards compatible) we could have:

// original behviour
require("Encoder").connect(A1,A2,functio­n (direction) { ... });

// your behaviour
require("Encoder").connect(A1,A2,{clicks­:2, callback:function (direction) { ... }});

// other behaviour
require("Encoder").connect(A1,A2,{clicks­:4, callback:function (direction) { ... }});

Of course there might be a better name for it, but allowing the function to be changed to an object with extra parameters would be really good.

The fixed issue was that last was initialized with a 'valid' value that created invalid values. From a coding point of view though I'd preferred to leave the initializer in the constructor but choose a different initialization value, for example, -1, which results as well in correct behavior. Last but not least I'd left it there for clarity and follow the style of Espruino's goody over that standard JS to complain about not initialized values. - just 1-1/2 cents. ;-)