Reading encoder gives very strange results using the encoder library

I am using the Encoder Library from PJRC Encoder Library, for Measuring Quadarature Encoded Position or Rotation Signals, which works wonderful. The library is the first one mentioned on the Arduino Reading Rotary Encoders page. I use the HONEYWELL HLC2701 IR optic encoders with A connected to an interrupt and B connected to a non interrupt input. There is no bouncing or any other interference.

I have a class PID, which takes as one of its arguments a pointer to an encoder instance. The PID is very similar to the Arduino PID_v1 library. Instead of taking its input from a variable, it reads the encoder. The following C++ code works great:

PID.h
-----------------------------------------------------------
class PID
{
 public:
  PID(Encoder* encoder, Motor* motor, double* setpoint, double kp, double ki, double kd, int direction);
  bool Compute();
  ...
 private:
  Encoder *_input;
  Motor *_output;
  double *_setpoint;			  
  ...


PID.cpp
-----------------------------------------------------------
PID::PID(Encoder* encoder, Motor* motor, double* setpoint,
	 double Kp, double Ki, double Kd, int direction)
{
  _output = motor;
  _input = encoder;
  ...

bool PID::Compute()
{
  ...
  double input = 0;
  double error = 0;
  ...
  if (dt >= _sampletime) {
    // Read encoder and immediately reset it to 0.
    input = float(_input->read()); 
    _input->write(0);
    setpoint = *_setpoint;
    error = setpoint - input;
    ...

Every time the PID loop is executed, the encoder is read as input and the error is computed. Immediatly after reading the encoder, it is reset to 0 for counting the pulses for the next loop. This gives good results.

Problem is, the encoder is reset every time and I can not use the encoder readings directly to calculate the distance the motor travelled so far. So I inserted the following code:

bool PID::Compute()
{
  ...
  double input = 0;
  double error = 0;
  double output = 0;
  long encoder = 0;

  if (dt >= _sampletime) {
    // Read encoder and subtract previous encoder reading to calculate input.
    encoder = _input->read();
    input = float(encoder - _lastencoder);
    _lastencoder = encoder;
    ...

The _lastencoder variable is a long and initialized to zero on startup.
Now the encoder returns weird numbers when I turn the motor just a little bit, maybe one encoder pulse or so:

encoder, _lastencoder, input :3	3	0.00
encoder, _lastencoder, input :1073807398	3	1073807360.00
encoder, _lastencoder, input :3	1073807398	-1073807360.00
encoder, _lastencoder, input :1073807398	3	1073807360.00
encoder, _lastencoder, input :3	1073807398	-1073807360.00
encoder, _lastencoder, input :3	3	0.00

So, could anybody please tell me what goes wrong?

TIA

PS
The encoder library is a little weird because all code is in the header file and not in the cpp file. The reader function looks like this:

#ifdef ENCODER_USE_INTERRUPTS
	inline int32_t read() {
		if (interrupts_in_use < 2) {
			noInterrupts();
			update(&encoder);
		} else {
			noInterrupts();
		}
		int32_t ret = encoder.position;
		interrupts();
		return ret;
	}
	inline void write(int32_t p) {
		noInterrupts();
		encoder.position = p;
		interrupts();
	}
#else
	inline int32_t read() {
		update(&encoder);
		return encoder.position;
	}
	inline void write(int32_t p) {
		encoder.position = p;
	}
#endif

So, could anybody please tell me what goes wrong?

No. What is that output supposed to be telling anybody? A bunch of names followed by a bunch of values? It would make more sense to output name = value pairs, AND show the code where the name = value stuff gets printed.

Using names like _input and input in the same code shows a clear lack of design. No variable should EVER be named input. Doing so means that you haven't a clue WHAT the input is, which means that haven't a hope of solving a real-world problem. Sorry to be so harsh, but sometimes life just is that way.

There is no need to resort to names like _input and input, for any reason. If you REALLY think you need to, then call them local_input and class_input, just to illustrate that you can't think of better names.

Rather than start a flame war I'll address the question.

The large numbers are at or near 2^30-2^16 which is suggestive. Without seeing the code
for update() its hard to see why this should be. The 2^16 error would be indicative of the
noInterrupts() failing perhaps, but the 2^30 error is strange, but from the log its clearly
coming from the read() function, which calls update().

Are all the necessary volatile declarations present?

Dear MarkT,

Yes, the large number comes from the read() function. I used both volatile and non-volatile declarations for the encoder variable, but that made no difference.
This is the update() code. I'll attach the original files as well.
Appreciate your help very much!

private:
	static void update(Encoder_internal_state_t *arg) {
#if defined(__AVR__)
		// The compiler believes this is just 1 line of code, so
		// it will inline this function into each interrupt
		// handler.  That's a tiny bit faster, but grows the code.
		// Especially when used with ENCODER_OPTIMIZE_INTERRUPTS,
		// the inline nature allows the ISR prologue and epilogue
		// to only save/restore necessary registers, for very nice
		// speed increase.
		asm volatile (
			"ld	r30, X+"		"\n\t"
			"ld	r31, X+"		"\n\t"
			"ld	r24, Z"			"\n\t"	// r24 = pin1 input
			"ld	r30, X+"		"\n\t"
			"ld	r31, X+"		"\n\t"
			"ld	r25, Z"			"\n\t"  // r25 = pin2 input
			"ld	r30, X+"		"\n\t"  // r30 = pin1 mask
			"ld	r31, X+"		"\n\t"	// r31 = pin2 mask
			"ld	r22, X"			"\n\t"	// r22 = state
			"andi	r22, 3"			"\n\t"
			"and	r24, r30"		"\n\t"
			"breq	L%=1"			"\n\t"	// if (pin1)
			"ori	r22, 4"			"\n\t"	//	state |= 4
		"L%=1:"	"and	r25, r31"		"\n\t"
			"breq	L%=2"			"\n\t"	// if (pin2)
			"ori	r22, 8"			"\n\t"	//	state |= 8
		"L%=2:" "ldi	r30, lo8(pm(L%=table))"	"\n\t"
			"ldi	r31, hi8(pm(L%=table))"	"\n\t"
			"add	r30, r22"		"\n\t"
			"adc	r31, __zero_reg__"	"\n\t"
			"asr	r22"			"\n\t"
			"asr	r22"			"\n\t"
			"st	X+, r22"		"\n\t"  // store new state
			"ld	r22, X+"		"\n\t"
			"ld	r23, X+"		"\n\t"
			"ld	r24, X+"		"\n\t"
			"ld	r25, X+"		"\n\t"
			"ijmp"				"\n\t"	// jumps to update_finishup()
			// TODO move this table to another static function,
			// so it doesn't get needlessly duplicated.  Easier
			// said than done, due to linker issues and inlining
		"L%=table:"				"\n\t"
			"rjmp	L%=end"			"\n\t"	// 0
			"rjmp	L%=plus1"		"\n\t"	// 1
			"rjmp	L%=minus1"		"\n\t"	// 2
			"rjmp	L%=plus2"		"\n\t"	// 3
			"rjmp	L%=minus1"		"\n\t"	// 4
			"rjmp	L%=end"			"\n\t"	// 5
			"rjmp	L%=minus2"		"\n\t"	// 6
			"rjmp	L%=plus1"		"\n\t"	// 7
			"rjmp	L%=plus1"		"\n\t"	// 8
			"rjmp	L%=minus2"		"\n\t"	// 9
			"rjmp	L%=end"			"\n\t"	// 10
			"rjmp	L%=minus1"		"\n\t"	// 11
			"rjmp	L%=plus2"		"\n\t"	// 12
			"rjmp	L%=minus1"		"\n\t"	// 13
			"rjmp	L%=plus1"		"\n\t"	// 14
			"rjmp	L%=end"			"\n\t"	// 15
		"L%=minus2:"				"\n\t"
			"subi	r22, 2"			"\n\t"
			"sbci	r23, 0"			"\n\t"
			"sbci	r24, 0"			"\n\t"
			"sbci	r25, 0"			"\n\t"
			"rjmp	L%=store"		"\n\t"
		"L%=minus1:"				"\n\t"
			"subi	r22, 1"			"\n\t"
			"sbci	r23, 0"			"\n\t"
			"sbci	r24, 0"			"\n\t"
			"sbci	r25, 0"			"\n\t"
			"rjmp	L%=store"		"\n\t"
		"L%=plus2:"				"\n\t"
			"subi	r22, 254"		"\n\t"
			"rjmp	L%=z"			"\n\t"
		"L%=plus1:"				"\n\t"
			"subi	r22, 255"		"\n\t"
		"L%=z:"	"sbci	r23, 255"		"\n\t"
			"sbci	r24, 255"		"\n\t"
			"sbci	r25, 255"		"\n\t"
		"L%=store:"				"\n\t"
			"st	-X, r25"		"\n\t"
			"st	-X, r24"		"\n\t"
			"st	-X, r23"		"\n\t"
			"st	-X, r22"		"\n\t"
		"L%=end:"				"\n"
		: : "x" (arg) : "r22", "r23", "r24", "r25", "r30", "r31");
#else
		uint8_t p1val = DIRECT_PIN_READ(arg->pin1_register, arg->pin1_bitmask);
		uint8_t p2val = DIRECT_PIN_READ(arg->pin2_register, arg->pin2_bitmask);
		uint8_t state = arg->state & 3;
		if (p1val) state |= 4;
		if (p2val) state |= 8;
		arg->state = (state >> 2);
		switch (state) {
			case 1: case 7: case 8: case 14:
				arg->position++;
				return;
			case 2: case 4: case 11: case 13:
				arg->position--;
				return;
			case 3: case 12:
				arg->position += 2;
				return;
			case 6: case 9:
				arg->position += 2;
				return;
		}
#endif
	}

Encoder.h (25.1 KB)

Encoder.cpp (251 Bytes)

Whoa, what a complex library for such a simple task! Still even the assembler in update() seems to make sense and
DTRT, but I suspicious of it causing issues (linker and assembler version dependent).

There is at least one discrepancy between the assembler and C versions of update, but only affects the case of both
quadrature signals changing simultaneously (which is undefined anyway).

It seems the read() function works without a problem as long as it is not passed along to another class instance or function as I did. The xxxx->read() construction does not work reliable. I rewrote the code with calling the read() from the same sketch the encoder instance is defined and all works great.