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

Rotary Encoder #10

Open
HaxorGate opened this issue May 6, 2019 · 9 comments
Open

Rotary Encoder #10

HaxorGate opened this issue May 6, 2019 · 9 comments
Assignees
Labels
enhancement New feature or request new component idea

Comments

@HaxorGate
Copy link

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
Copy link
Owner

gilmaimon commented May 8, 2019

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.

@gilmaimon gilmaimon added enhancement New feature or request new component idea labels May 8, 2019
@gilmaimon gilmaimon self-assigned this May 8, 2019
@HaxorGate
Copy link
Author

HaxorGate commented May 8, 2019 via email

@gilmaimon
Copy link
Owner

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
Copy link
Author

HaxorGate commented May 9, 2019 via email

@gilmaimon
Copy link
Owner

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
Copy link
Author

HaxorGate commented May 9, 2019 via email

@gilmaimon
Copy link
Owner

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
Copy link
Author

HaxorGate commented May 11, 2019 via email

@gilmaimon
Copy link
Owner

gilmaimon commented May 11, 2019

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new component idea
Projects
None yet
Development

No branches or pull requests

2 participants