drivers/rotary_encoder : WIP #5168

dkm · 2016-03-24T14:36:34Z

This is a WIP for handling rotary encoders. Tested on lm4f120, so basic feature should be mostly OK.
There are still lots of things to take care, in particular how to handle the different hw you can find.

dkm · 2016-03-24T14:38:32Z

(this is a follow-up of the "Encoder drivers" mail on riot-devel@ )

miri64 · 2016-03-24T15:42:27Z

For reference I provided a link to devel's archive: https://lists.riot-os.org/pipermail/devel/2016-March/003725.html

OlegHahm · 2016-05-31T19:32:00Z

Any progress?

dkm · 2016-05-31T20:41:17Z

it works more or less, but i'm willing to merge other changes before. This PR was here because some ppl wanted to have a look and test it... It still work, I'm using it on a daily basis...

OlegHahm · 2016-05-31T20:59:25Z

Thanks

kYc0o · 2016-07-22T14:49:09Z

WIP and new feature -> next release.

miri64 · 2016-10-31T13:19:45Z

Postponed due to Feature Freeze.

PeterKietzmann · 2017-01-13T15:54:55Z

but i'm willing to merge other changes before

Did you, can you give a quick update?

dkm · 2017-01-13T16:08:27Z

Other changes are not merged and will probably never be merged because nobody can test (except me obviously) them (even if the board is "maintained"...). As stated before, I was willing to contribute as much as possible, but it looks like RIOT team can't handle these kind of minor contributions (affecting only a handfull of isolated ppl), which is understandable given the size of the team and all the works in progress in more useful part :)

Quick update: nothing has changed since last time and work is currently stalled.

miri64 · 2017-01-24T17:10:50Z

As stated before, I was willing to contribute as much as possible, but it looks like RIOT team can't handle these kind of minor contributions (affecting only a handfull of isolated ppl), which is understandable given the size of the team and all the works in progress in more useful part :)

This is not a denial of your work, I'm just curious, but may I ask why, if it only affects a handful of people (and are not able to provide the hardware for us to test) you want to contribute the code for it to RIOT? You always can publish the code for the driver on your own terms: CPUs can be integrated by changing the RIOTCPU macro in you application's Makefile, same goes for boards with RIOTBOARD. Modules can be integrated from external directories using the EXTERNAL_MODULES variable.

kaspar030 · 2017-01-24T17:22:31Z

You always can publish the code for the driver on your own terms

@dkm I appretiate your effort of trying to get the code merged here!

miri64 · 2017-01-24T18:19:03Z

@dkm I appretiate your effort of trying to get the code merged here!

Don't get me wrong, me too! Just wondering if you were aware about the external integration.

dkm · 2017-01-24T19:51:05Z

@miri64 because you have partial support for this board, it is free software, you have a nice community working with PR and I wanted to make the existing support better. I like the idea of having my changes getting merged and being useful to other (even if only a few) (which is harder if I publish changes elsewhere). Initially, I thought these changes could be a start for contributing to RIOT.
Now, I don't plan on pushing all my changes (I also have a working PWM driver), but I would have liked the part where I spent few hours rebasing several times getting finally merged.
It's true that I am "not able to provide the hardware for us[RIOT] to test", but I've also never been asked. I did offer to provide waves for testing the SPI though, to get better confidence in my external tests. I don't know how the team works, but few months ago, I asked for the status of the board and if it was possible to have some board support hosted/reviewed by RIOT but not necessarily tested («community maintained») and was answered that this board IS maintained.

I guess my mistake is thinking that if the code is not yet removed (obsoleted), it is still active and possible to contribute.

[I know that everything is ready for having part of RIOT out of the tree, I've been using this on my side.]

Back to this PR. I did a quick and dirty hack and someone on the mailing list was interested by it. I was instructed to open this PR, but I don't think I'll take the time needed to clean it: it can be closed with no regret :)

aabadie · 2017-04-05T11:54:53Z

Maybe @astralien3000 could be interested by a rotary encoder driver in RIOT.

I took a quick look at your code and there are quite some things missing, requiring changes before getting it merged.

See this page for coding guidelines commonly used in RIOT.

I won't comment inline and just summarize some of the problems I saw here:

Use C style comments /* */ instead of //
Use 4 spaces indentations
Use the following pattern for header guards : HEADER_GUARD_H
A @defgroup line is needed in the doxygen block of rotary.h
There are missing doxygen documentation (TIP: run make static_test in the RIOTBASE to get - and fix ! - all problems locally before pushing)
Use double quotes include for RIOT related includes : #include "periph/gpio.h", and <> for system includes
Wrap your header for C++ compatibility
Put return types and function names on the same line. Example:

int my_func(void* arg)
{
    /* some code */
}

Add a basic test applications in the tests directory with a README.md explaining how to use.

It can be a bit of work, but I think this PR is nice addition in general for RIOT and I would be happy to press the green button one day.

dkm · 2017-04-05T12:01:19Z

@aabadie I'm still using this piece of code but didn't bother polishing it as I was already struggling with other changes. I would be happy to make the necessary effort for getting it merged in RIOT. Expect some rebase in the coming days :)

astralien3000 · 2017-04-05T12:26:36Z

@aabadie actually, you can be interested as well ^^
This piece of code should be able to read the encoders you have on RiBot.
I may also have some (bad) encoders to test it.

dkm · 2017-04-05T12:32:10Z

The original code can handle different kind of encoder. To simplify the porting, I've constrained it to the kind I'm using (cheap aliexpress device). I'm still not clear exactly what kind it is. I've tested with some other encoder (ripped from a logitech kbd) and it was clearly not a good match.

astralien3000 · 2017-04-05T12:45:57Z

This PR is about rotary encoder with quadratic signal, right ?

dkm · 2017-04-06T11:25:30Z

I'm not really sure, I should check what I'm using :)

Also, I was in the process of cleaning the code, and started by having correct copyright. The original code is released under the GPL v3. RIOT is (mainly?) licensed under LGPL v2.1. I'm not sure that it is a good idea to mix these (the result, if using this code, would have to be distributed under GPL clauses, not LGPL), even if I don't really see how the Lesser version is used in RIOT as i thought resulting executables were always static. Do you have more insight on this ?

A solution would be to rewrite the drivers from scratch, but I'm not sure I'll find the time...

aabadie · 2017-04-06T11:39:23Z

I guess you are talking of this project : https://github.com/buxtronix/arduino/tree/master/libraries/Rotary

The GPLv3 is indeed a problem if one copy-pastes it as is in RIOT code. Looking at the code in your PR, except the state tables that remains very similar, the code is already kind of a rewrite of the initial library : the GPIO API is different, you are using interrupts instead of polling, etc

@emmanuelsearch, @OlegHahm any thoughts ?

dkm · 2017-04-06T11:51:37Z

Yes, that's the code I'm referring to...

lebrush

Preliminary code review. Please add documentation in header files. Remove commented out code and use /* */ comment style.

lebrush · 2017-04-10T07:12:18Z

drivers/include/rotary.h

+ */
+
+/**
+ * @ingroup     drivers_rotary_encoder


@defgroup

lebrush · 2017-04-10T07:14:36Z

drivers/include/rotary.h

+ */
+
+#ifndef __ROTARY_ENC_H
+#define __ROTARY_ENC_H


ROTARY_ENC_H

lebrush · 2017-04-10T07:14:54Z

drivers/include/rotary.h

+#include <periph/gpio.h>
+
+typedef struct {
+  gpio_t pin1, pin2;


2 spaces instead of 4. Did you run uncrustify?

lebrush · 2017-04-10T07:16:15Z

drivers/rotary_encoder/rotary.c

+}
+
+int
+rotary_init(rotary_t *dev, gpio_t pin1, gpio_t pin2){


no new line between type and function definition. Space between arguments and {

lebrush · 2017-04-10T07:28:38Z

drivers/rotary_encoder/rotary.c

+  rotary_t *dev = (rotary_t*) rot_dev;
+  unsigned int b1_v = gpio_read(dev->pin1) ? 1 : 0;
+  unsigned int b2_v = gpio_read(dev->pin2) ? 1 : 0;
+  unsigned char pinstate = ( b1_v? 2 : 0) | (b2_v ? 1 : 0);


b1_v and b2_v don't need to be set to 0 or 1. Simply use

int b1_v = gpio_read(dev->pin1); int b2_v = gpio_read(dev->pin2);

or get rid of them :-)

Additionally, I would also make pinstate an int since it's only a support variable. If you remove the other 2 variables there's no size impact (and sometimes avoids extra casting introduced by the compiler).

int pinstate = (gpio_read(dev->pin1) ? 2 : 0) | (gpio_read(dev->pin2) ? 1 : 0);

That's right. I'll keep the 2 var as the compiler is able to remove them when asked and also he keeps them when debuging with -O0 :)

lebrush · 2017-04-10T07:34:06Z

drivers/include/rotary.h

+// Clockwise step.
+#define DIR_CW 0x10
+// Anti-clockwise step.
+#define DIR_CCW 0x20


if it's meant to be used by some other module please make an enum of it

lebrush · 2017-04-10T07:35:33Z

drivers/include/rotary.h

+void rotary_register(rotary_t *dev, unsigned int pid);
+
+
+#define ROTARY_EVT 1


Please rename to ROTARY_MSG_TYPE or similar

lebrush · 2017-04-10T07:36:03Z

drivers/rotary_encoder/rotary.c

+}
+
+void
+rotary_register(rotary_t *dev, unsigned int pid){


please use kernel_pid_t

lebrush · 2017-04-10T07:36:14Z

drivers/include/rotary.h

+
+typedef struct {
+  gpio_t pin1, pin2;
+  unsigned int listener;


please use kernel_pid_t

lebrush · 2017-04-10T07:37:37Z

drivers/rotary_encoder/rotary.c

+
+  dev->state = ttable[dev->state & 0xf][pinstate];
+
+  switch(dev->state & 0x30){


The current state tables has values from 1 to 5 or from 1 to 6. So masking with 0x30 will always return 0.

The mask is used to test upper bits for values stored in the table (ie. if the value has DIR_CW or DIR_CCW bit set). dev->state equals one line of the table, it is not an index of the table.

dkm · 2017-04-10T11:26:39Z

I'll add doc in header later. But as I'm still not convinced that this GPL3 code can be used easily in RIOT...

astralien3000 · 2017-06-23T16:01:04Z

If I understand well, the fact that your code is under GPL implies that any user app or driver using your code (USEMODULE += rotary_encoder and/or #include "rotary.h") will fall under the GPL License. It is not a problem for any other app or module that is not using your driver.
Am I wrong ?

After that it is a matter of policy, which seems to be no GPL in the repos.
The other solution to add your code is to create an external package, which can more easily accept GPL.

@dkm From your side, you seems to consider that your code is not different enough from this lib to have an other license. The only way to publish it as LGPL is to ask the author of the lib if he considers what you wrote as a derived work of his and if he agrees to publish it under an other license.

dkm · 2017-06-23T16:11:06Z

@astralien3000 There is no «being different enough». I've based the code from a copy of his code, so it's clearly a derived work and must comply with its license.
I think you're correct on the usage of this module wrt to licensing: using it forces the publication of all the software under terms compatible with LGPL.
I've asked the original author when we discussed this here (mid april) about a possible re-licensing, but I did not get any answer.
I haven't seen any work in RIOT to handle this kind of licensing issue (like «enabling» some modules with clear warning about the consequences).

Best move would be to reject this PR and simply make it live outside with a clear statement in the README. I can do that if you think it's worth the effort :)

astralien3000 · 2017-06-23T17:42:59Z

Well, what is the point of starting from scratch in your case ? The algorithm is like 70 lines long. If you start from scratch and implement the same idea, you will end up with the same algorithm, maybe with slightly different names. Does the fact that you have seen the other code before writing yours makes yours a derived work ? For me it's yes because patents concern ideas, not only lines of code. If you think you would have done something different by starting from scratch, why didn't you do that in the first place ?

Well I guess this PR will be rejected if there is no solution with the license.
About the effort (that you are not sure is worth) of doing an external package, I don't know how much people will use that.
But in my case, I will always use the hardware counter that is present on the microcontroller.

dkm · 2017-06-23T18:39:24Z

In theory, I should not be the one re-implementing it. But in this case, for a 70 lines long code, I guess nobody would complain :)

As for 'why didn't you do it in the first place', easy answer: I tend to not rewrite existing code when possible :)

miri64 · 2018-02-15T16:21:04Z

No significant progress for half a year. Closing as memo for now.

emmanuelsearch added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Mar 24, 2016

emmanuelsearch added this to the Release 2016.07 milestone Mar 24, 2016

emmanuelsearch assigned haukepetersen Mar 24, 2016

dkm force-pushed the pr/rotary branch from b2f726e to a6460a4 Compare April 8, 2016 11:27

kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 22, 2016

miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016

PeterKietzmann removed this from the Release 2017.01 milestone Jan 13, 2017

miri64 assigned aabadie and lebrush Apr 5, 2017

lebrush suggested changes Apr 10, 2017

View reviewed changes

drivers/rotary_encoder : WIP

9ea379b

dkm force-pushed the pr/rotary branch from a6460a4 to 9ea379b Compare April 10, 2017 10:41

aabadie modified the milestone: Release 2017.07 Jun 23, 2017

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng removed this from the Release 2018.01 milestone Jan 12, 2018

miri64 closed this Feb 15, 2018

miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Feb 15, 2018

		void rotary_register(rotary_t *dev, unsigned int pid);


		#define ROTARY_EVT 1


		dev->state = ttable[dev->state & 0xf][pinstate];

		switch(dev->state & 0x30){

drivers/rotary_encoder : WIP #5168

drivers/rotary_encoder : WIP #5168

Conversation

dkm commented Mar 24, 2016

dkm commented Mar 24, 2016

miri64 commented Mar 24, 2016

OlegHahm commented May 31, 2016

dkm commented May 31, 2016

OlegHahm commented May 31, 2016

kYc0o commented Jul 22, 2016

miri64 commented Oct 31, 2016

PeterKietzmann commented Jan 13, 2017 • edited

dkm commented Jan 13, 2017

miri64 commented Jan 24, 2017

kaspar030 commented Jan 24, 2017

miri64 commented Jan 24, 2017

dkm commented Jan 24, 2017

aabadie commented Apr 5, 2017 • edited

dkm commented Apr 5, 2017

astralien3000 commented Apr 5, 2017

dkm commented Apr 5, 2017

astralien3000 commented Apr 5, 2017

dkm commented Apr 6, 2017

aabadie commented Apr 6, 2017

dkm commented Apr 6, 2017

lebrush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkm commented Apr 10, 2017

astralien3000 commented Jun 23, 2017

dkm commented Jun 23, 2017

astralien3000 commented Jun 23, 2017

dkm commented Jun 23, 2017

miri64 commented Feb 15, 2018

PeterKietzmann commented Jan 13, 2017 •

edited

aabadie commented Apr 5, 2017 •

edited