raspberrypi: Several rotary encoder fixes

[jepler]

Support swapped pins, Closes: #4422

Turn off interrupts with deinit(), Closes: #4557

Don't say we have an output pin, Closes: #4556

[jepler]

@daveythacher can you please provide more context? I don't follow what you are saying. If you're suggesting a change that is unrelated to the specific issues this PR is intended to address, consider filing your own PR or issue.

[dhalbert]

@daveythacher There is an implementation in the nrf port that is reminiscent of what you posted above:

circuitpython/ports/nrf/common-hal/rotaryio/IncrementalEncoder.c

Lines 37 to 66 in a156243

	static void _intr_handler(nrfx_gpiote_pin_t pin, nrf_gpiote_polarity_t action) {
	rotaryio_incrementalencoder_obj_t *self = _objs[pin];
	if (!self) {
	return;
	}
	// reads a state 0 .. 3 *in order*.
	uint8_t new_state = nrf_gpio_pin_read(self->pin_a);
	new_state = (new_state << 1) + (new_state ^ nrf_gpio_pin_read(self->pin_b));
	uint8_t change = (new_state - self->state) & 0x03;
	if (change == 1) {
	self->quarter++;
	} else if (change == 3) {
	self->quarter--;
	}
	// ignore other state transitions
	self->state = new_state;
	// logic from the atmel-samd port: provides some damping and scales movement
	// down by 4:1.
	if (self->quarter >= 4) {
	self->position++;
	self->quarter = 0;
	} else if (self->quarter <= -4) {
	self->position--;
	self->quarter = 0;
	}
	}

[tannewt]

One, question. Thanks for the fixes!

[tannewt]

Why do we need to save this? I was thinking we'd just need to support both ordering but not change how position changes.

[jepler]

it was part of the original request, as I understood it. Otherwise, the user could just reverse the pins themselves in the constructor.

[daveythacher]

Is the expectation the position will always be a difference to their counter? Is there a need to make direction reversible on the fly? Is there a need to see a roll over point internally? Is there a need to add scaling for revolutions? Can someone implement a new class based on this?

I know jepler's solution had better performance than the solution I was recommending. However there could be different use cases, which I had not considered before. I guess my question is what is the expectation for the user and API?

You could use the position variable as a counter and avoid using it in application logic. If you are porting to different hardware you could edit the constructor. Maybe the user of a GUI wants it one way so the GUI exposes that? However maybe you want to keep counting forwards even though the direction changed?

If kept difference based you could retrofit all of this in application logic. However that logic could be ugly.

[tannewt]

I'd rather not add switching counting direction because you can do it outside of this class. My understanding what just that the pin order could be swapped.

[daveythacher]

@daveythacher There is an implementation in the nrf port that is reminiscent of what you posted above:

circuitpython/ports/nrf/common-hal/rotaryio/IncrementalEncoder.c

Lines 37 to 66 in a156243

	static void _intr_handler(nrfx_gpiote_pin_t pin, nrf_gpiote_polarity_t action) {
	rotaryio_incrementalencoder_obj_t *self = _objs[pin];
	if (!self) {
	return;
	}
	// reads a state 0 .. 3 *in order*.
	uint8_t new_state = nrf_gpio_pin_read(self->pin_a);
	new_state = (new_state << 1) + (new_state ^ nrf_gpio_pin_read(self->pin_b));
	uint8_t change = (new_state - self->state) & 0x03;
	if (change == 1) {
	self->quarter++;
	} else if (change == 3) {
	self->quarter--;
	}
	// ignore other state transitions
	self->state = new_state;
	// logic from the atmel-samd port: provides some damping and scales movement
	// down by 4:1.
	if (self->quarter >= 4) {
	self->position++;
	self->quarter = 0;
	} else if (self->quarter <= -4) {
	self->position--;
	self->quarter = 0;
	}
	}

My post before had a slight error, so I deleted it. I think I figured out a fix for it. However I did not repost since it was not directly referenced in the pull request.

Basically it attempted to use XOR and jump tables to remove look up table. Less lines, but may not be as readable. I am not able to verify the assembly length, however I currently guess it to be similar.

That is a similar approach it follows something mentioned here:
https://bobrathbone.com/raspberrypi/documents/Raspberry%20Rotary%20Encoders.pdf

[daveythacher]

Here it is, just in case it works or is useful. Below is truth table which shows how it is supposed to work through the different steps. This is probably no better than the nrf solution. Assembly wise they are probably close. The look up table is little easier to understand, and maybe the compiler could figure this out. However this and the nrf avoid the table in ROM/RAM.

Edit:
@dhalbert @jepler nrf solution appears to be better than this following closer inspection. My apologies for the tangent.
@jepler Never seen compiler explorer before that is really cool.

new &= 0x3;
int x = new ^ self->last_state;
switch (self->last_state) {
	case 1:
	case 2:
		x = ~x;
		break;
}
switch (x & 3) {
	case 1:
		self->quarter_count -= 1;
		break;
	case 2:
		self->quarter_count += 1;
		break;
}
self->last_state = new;

/*
0000 = 0 = 0 = 0 = 0
0001 = 1 = 1 = 1 = -1
0010 = 2 = 2 = 2 = +1
0011 = 3 = 3 = 3 = 0
0100 = 1 = -2 = 2 = +1
0101 = 0 = -1 = 3 = 0
0110 = 3 = -4 = 0 = 0
0111 = 2 = -3 = 1 = -1
1000 = 2 = -3 = 1 = -1
1001 = 3 = -4 = 0 = 0
1010 = 0 = -1 = 3 = 0
1011 = 1 = -2 = 2 = +1
1100 = 3 = 3 = 3 = 0
1101 = 2 = 2 = 2 = +1
1110 = 1 = 1 = 1 = -1
1111 = 0 = 0 = 0 = 0
*/

[jepler]

As long as we're code-golfing, here's what I came up with:

#define quadrature_const (0x3443c11cu)
int8_t quadrature_fast(uint8_t a, uint8_t b) {
    int idx = (a << 3) | (b << 1);
    return (int)(quadrature_const3 << idx) >> 30;
}

This compiles to 6 thumb instructions and 1 out-of-line constant, for a total of 16 bytes. It works by extracting 2 consecutive bits from the constant, then sign-extending to give -1, +1, or 0. However, it makes an assumption about unsigned-to-signed conversion that is not guaranteed by standard C :( so that's not so great. (it matches the semantics of gcc with -fwrapv specified, though)

It's verifiably identical to

int8_t quadrature_slow(uint8_t a, uint8_t b) {
    const static int8_t quadrature_slow_table[] = {
     0, -1, +1,  0, // 00 => 00 01 10 11
    +1,  0,  0, -1, // 01 => 00 01 10 11
    -1,  0,  0, +1, // 10 => 00 01 10 11
     0, +1, -1,  0, // 11 => 00 01 10 11
    };
    int idx = (a << 2) | b;
    return quadrature_slow_table[idx];
}

which is easy to show through exhaustive testing of the 16 possibilities. (in fact, the constants were generated from such a routine so it's no surprise--any bug from quadrature_slow will reproduce itself in quadrature_fast). This assembles to 4 instructions plus 4 bytes to point to the table, plus 16 bytes for the table: 28 bytes.

I think that more important than getting the best byte size of the code possible, it would be good to provide a "pretty good" shared routine for the multiple ports that need it. Verifying that the nrf52 and atmel-samd implementations actually do the same thing (or that they don't) is tough, but if they share the code then it's easier.

@daveythacher If you're interested in working on this, maybe addressing it in a fresh PR intended to unify encoder handling and close #3875 would be the way to go.

[dhalbert]

@daveythacher If you're interested in working on this, maybe addressing it in a fresh PR intended to unify encoder handling and close #3875 would be the way to go.

Please also see #3875 (comment), where I mention another very simple algorithm I found that only uses one interrupt. It misses a count when reversing, which was the original motivation for #3875 and its corresponding PR.

[jepler]

7 instructions & no constant (so 2 bytes smaller & no worry about undefined behavior under C)

int8_t quadrature_fast3(uint8_t a, uint8_t b) {
    int x = (1 + ((a ^ (a >> 1)) - (b ^ (b >> 1)))) & 3;
    return x - 1; 
}

x ^ (x>>1) converts a 2-bit gray-coded number to binary. The arithmetic (1+p-q) & 3 gives +1, 0, -1 as +2, +1, 0; then subtracting 1 in signed fashion gives the correct answer. Again, exhaustive verification is feasible.

HOWEVER the caveat with this version is that the invalid transitions give +2 instead of 0.

[jepler]

@tannewt I removed support for reversing the encoder when swapping the pins. Please review again.

[tannewt]

Thank you!