Rotary Encoder

[HaxorGate]

In using the ArduinoComponents library, I found that I needed a RotaryEncoder component. I wrote one that makes use of TactileButton. I don't know if you'd want to include it. But here are the .h & .cpp files in case you want them (they're meant to be in the Components subfolder).
RotaryEncoder.zip

[gilmaimon]

Hi!

Thank you for using the library, I had no idea there are people who use it other than me 😄
I would love to include your code, but it can be great of you help me refactor it a little bit and understand it better.

Can you share some info about:

• The rotary encoder you are using
• The general algorithm you wrote (basically how you implemented)
• What onRotate does

Also, the best way of contributing to GitHub libraries is by cloning the repository and opening a pull-request. If you don't want to (you rather just submit your code here, like you did) it's fine!

Thank you.

[HaxorGate]

Thanks for the reply. The encoder I used was the cheapest one I could find at my local electronics store ( https://vetco.net/products/rotary-encoder-w-push-button-switch/vupn7453). In theory this code will work with any incremental encoder. [image: RotaryEncoderComponent_wiring_diagram.png] The center pin is Common, the other two are A and B (which one is A and which one is B don't really matter). The code essentially uses the TactileButton.onPress to determine which direction the encoder is rotated. The signals on both A and B are taken into account to determine the state of the encoder. Basically if A is pressed and B is not pressed it is rotating to the left. If A is pressed and B is pressed it is rotating to the right. The onRotate() method simply prints out the rotation value along with the direction of rotation (either left or right). It is called within the onPress() method for pin A with a positive or negative value depending upon the isPressed() value of pin B. Sorry for being so unclear in my original email. I honestly wasn't even sure if you were monitoring the github or not. Also, thanks for the library. I find it quite useful. I'm currently working on a RFIDReader component (I have made one, but it still makes use of the Adafruit_PN532 library--a dependency I would like to eliminate). Brian Tugade 425 591 8392

…

On Wed, May 8, 2019 at 7:02 AM Gil Maimon ***@***.***> wrote: Hi! Thank you for using the library, I had no idea there are people who use it other than me 😄 I would love to include your code, but it can be great of you help me refactor it a little bit and understand it better. Can you share some info about: - The rotary encoder you are using - The general algorithm you wrote (basically how you implemented) - onRotate Also, the best way of contributing to GitHub libraries is by cloning the repository and opening a pull-request <https://help.github.com/en/articles/creating-a-pull-request-from-a-fork>. If you don't want to (you rather just submit your code here, like you did) it's fine! Thank you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG5IY2M5PGDXL7HSFXVAV4DPULMQHANCNFSM4HLD5PNQ> .

[gilmaimon]

First of all, you're welcome! I had no idea that there is anyone else who use this library. I was very happy when I saw your issue. And I am glad that you found the library useful, I am always happy to receive any comment on my code and libraries.

Personally I don't own a rotary encoder yet, so I will not feel comfortable releasing a version with your code without testing it a bit myself, I hope you understand. I ordered some just now, so as soon as they will arrive I will be able to publish a new version after testing the code.

Meanwhile, I've commited your files into the repository so I can start working on it. But, I would like to first ask your permission to edit and refactor your code, as there are quite few things I would like to change and fix (I can elaborate, if you wish).

Regarding dependencies: I am not sure how to approach components that relay on external libraries. I would not like to make new users install any other libraries only to use a LED and a Button. Also I dont think the library should include components that are not commonly used. A solution might be to distribute every contributed components as a seperated library. But that's a topic for another issue.

Thank you again for your contribution,
Gil.

[HaxorGate]

You can refactor as you see fit. If you'd rather I do it for you, I can do that too. It's your choice. Brian Tugade

…

On Thu, May 9, 2019 at 6:51 AM Gil Maimon ***@***.***> wrote: First of all, you're welcome! I had no idea that there is anyone else who use this library. I was very happy when I saw your issue. And I am glad that you found the library useful, I am always happy to receive any comment on my code and libraries. Personally I don't own a rotary encoder yet, so I will not feel comfortable releasing a version with your code without testing it a bit myself, I hope you understand. I ordered some just now, so as soon as they will arrive I will be able to publish a new version after testing the code. Meanwhile, I've commited your files into the repository so I can start working on it. But, I would like to first ask your permission to edit and refactor your code, as there are quite few things I would like to change and fix (I can elaborate, if you wish). Regarding dependencies: I am not sure how to approach components that relay on external libraries. I would not like to make new users install any other libraries only to use a LED and a Button. Also I dont think the library should include components that are not commonly used. A solution might be to distribute every contributed components as a seperated library. But that's a topic for another issue. Thank you again for your contribution, Gil. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG5IY2OXJQXPUIR2OESMRFLPUQT5XANCNFSM4HLD5PNQ> .

[gilmaimon]

Well, there are few things that should be adressed in your code:

• The code does not compile on Arduino (not esp8266 or esp32 boards)
• Use PinNumber instead of int8_t
• There should be no Serial communication in library code
• You should choose meaningful names. There is no need to call a variable dr when what you mean is direction
• What does rotation represents? You should probably choose a better name.
• Make sure all you variables are initialized. rotation is not.
• onRotate should accept a callback so the user could make action upon rotation

This is to name a few.
I am very picky with Code Reviews, if you wish to take it as exercise to refactor the code, go ahead, I will continue from where you left once I will receive the parts.

Gil.

[HaxorGate]

I am using the code for my current project at work as we speak. It definitely does compile. Did you forget to add the include directive to your header file? The file structure should be: [ArduinoComponents] [src] [Components] ... RotaryEncoder.h RotaryEncoder.cpp And in your file ArduinoComponents.h, you need to include the line: #include "ArduinoComponents/Components/RotaryEncoder.h" Also, PinNumber is sort of wasteful as it is written. It should be a typedef for uint8_t not int. That reduces the size of the code when using something like an ATmega2560 with 70 pins. This is why I used uint8_t for the pins corresponding to leg A and leg B of the rotary encoder. The variable, "rotation", is so named because it represents the rotation of the rotary encoder shaft. It could be initialized in the constructor, but it is already guaranteed to be initialized to zero by the compiler. That is a good point about Serial communication. I sort of shortcut the production process, which I shouldn't have done. The direction parameter of the onRotate method would probably be better served by being a boolean, to be honest. Having it as a signed character is a holdover from when I was using it as a multiplier so that Left was always negative and Right was always positive. When you do get your parts, you can use the optional "clicks" argument to the constructor to constrain the rotation value to the number of detents (if any) in the encoder that you bought. Brian Tugade

…

On Thu, May 9, 2019 at 2:04 PM Gil Maimon ***@***.***> wrote: Well, there are few things that should be adressed in your code: - The code does not compile on Arduino (not esp8266 or esp32 boards) - Use PinNumber instead of int8_t - There should be no Serial communication in library code - You should choose meaningful names. There is no need to call a variable dr when what you mean is direction - What does rotation represents? You should probably choose a better name. - Make sure all you variables are initialized. rotation is not. - onRotate should accept a callback so the user could make action upon rotation This is to name a few. I am very picky with Code Reviews, if you wish to take it as exercise to refactor the code, go ahead, I will continue from where you left once I will receive the parts. Gil. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG5IY2NELUZ2JDRI3YDIFBTPUSGUXANCNFSM4HLD5PNQ> .

[gilmaimon]

What compiler/board are you using currently? From my experiance some compilers include the fixed integer types () automatically and some don't. The compiler/board I am using probably does not include them implicitly.

I agree, PinNumber is wasteful and should not be an int. But consider the added readablity when reading:

PinNumber a;

vs

int8_t a;

Also, lets say this component will be needed in a system where there are more than 256 pins! so pin numbers are shorts. This is of-course imaginary a bit, but the nice thing about using a typedef is that it can be changed at any point and there will be no need to go through each file.

it is already guaranteed to be initialized to zero by the compiler

This is actually not always right. It might be correct with the current compiler and/or version that you are using, but in C++, primitive memebers are not gaurenteed to be initialized

It's also in the C++ Core Guidlines: ES.20: Always initialize an object

An enum will be a better approach than boolean, because it is easier to read. Consider:

enum Direction {
   Direction_Left = false,
   Direction_Right = true
};

encoder.onRotation([](Direction direction){
   if (direction == Direction_Left) {
      // Do something on left rotation
   } else if (direction == Direction_Right) {
      // Do something on right rotation
   }
});

Without this, the user must read the implementation or some comments/docs in order to understand that true means Right and false means Left.

I would rename clicks to encoderClicksCount.

Overall my approach is that I always prefer explicit over implicit. This is right for method/variable names, initialization, interfaces and everything else.

Gil.

[HaxorGate]

What compiler/board are you using currently? From my experiance some compilers include the fixed integer types () <https://en.cppreference.com/w/cpp/types/integer> automatically and some don't. The compiler/board I am using probably does not include them implicitly.

Since this is an Arduino library, I am using--presumably--the same compiler as you, avr-gcc in the case of the ATmega328P board (like the one in the wiring diagram I gave you previously). In the case of any Arduino library that includes "Arduino.h", we are guaranteed to have <intypes> included. Also, lets say this component will be needed in a system where there are

more than 256 pins!

In over ten years of engineering, I have yet to come across such a board.

This is actually not always right. It might be correct with the current compiler and/or version that you are using, but in C++, primitive memebers are not gaurenteed to be initialized <https://stackoverflow.com/a/3127482/4132303> It's also in the C++ Core Guidlines: ES.20: Always initialize an object <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object>

You need to be careful trying to translate software programming experience over to embedded systems programming. We will always be dealing with a very specific environment. In the case of AVR chips (i.e. most Arduino boards), an explicitly initialized variable (even if it is initialized to zero) will be placed in the .DATA section of the code rather than in the .BSS section where uninitialized variables (those that the compiler will implicitly initialize to zero) go. This causes variables to consume more space in flash than they are required to. This can become quite noticeable after a while. An enum will be a better approach than boolean, because it is easier to

read.

Fair point. I might go so far as to suggest a scoped enum deriving from uint8_t so as to minimize the flash footprint. Something like: enum class Direction : uint8_t { Left, Right }; This would also obviate the need for clunky looking prefix "Direction_", as the values must be scoped ( e.g. Direction::Left ) Brian Tugade

…

On Fri, May 10, 2019 at 12:17 AM Gil Maimon ***@***.***> wrote: What compiler/board are you using currently? From my experiance some compilers include the fixed integer types () <https://en.cppreference.com/w/cpp/types/integer> automatically and some don't. The compiler/board I am using probably does not include them implicitly. I agree, PinNumber is wasteful and should not be an int. But consider the added readablity when reading: PinNumber a; vs int8_t a; Also, lets say this component will be needed in a system where there are more than 256 pins! so pin numbers are shorts. This is of-course imaginary a bit, but the nice thing about using a typedef is that it can be changed at any point and there will be no need to go through each file. it is already guaranteed to be initialized to zero by the compiler This is actually not always right. It might be correct with the current compiler and/or version that you are using, but in C++, primitive memebers are not gaurenteed to be initialized <https://stackoverflow.com/a/3127482/4132303> It's also in the C++ Core Guidlines: ES.20: Always initialize an object <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object> An enum will be a better approach than boolean, because it is easier to read. Consider: enum Direction { Direction_Left = false, Direction_Right = true }; encoder.onRotation([](Direction direction){ if (direction == Direction_Left) { // Do something on left rotation } else if (direction == Direction_Right) { // Do something on right rotation } }); Without this, the user must read the implementation or some comments/docs in order to understand that true means Right and false means Left. I would rename clicks to encoderClicksCount. Overall my approach is that I always prefer explicit over implicit. This is right for method/variable names, initialization, interfaces and everything else. Gil. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG5IY2PICSI3UESJ72OPNMDPUUORBANCNFSM4HLD5PNQ> .

[gilmaimon]

In over ten years of engineering, I have yet to come across such a board.

Well, you obviously have far more experiance than me 😅. But yet, the first and primary argument of readablity still stands.

As far as I know, and please correct me if I am wrong about this, you can't know where a class member will be initialized as it depends on where the instance itself will be declared.*
* unless it is a static member

This would also obviate the need for clunky looking prefix "Direction_",

Yup, the prefix is dirty but it is useful when not using enum class (there is not really a reason to not use it tho). Your solution looks much better.

Also, I see you are using the method isPressed for tactile button, but it is not defined. Is it something you added locally?