Unexpected results from Encoder::GetRate()

[Bigcheese]

Here's 4x decoding data with the same changes to Encoder.cpp as listed above.

http://spreadsheets.google.com/ccc?k...E1zX 2c&hl=en

And here's the relevant code for getting the data.

void  updateCSV(void* stream)
{
	CompetitionRobot1771* r = CompetitionRobot1771::GetInstance();
	std::ofstream& s = *reinterpret_cast<std::ofstream*>(stream);

	static double left_previous_distance = r->m_PMU.m_leftEncoder.GetDistance();
	static double right_previous_distance = r->m_PMU.m_rightEncoder.GetDistance();
	static double previous_time = static_cast<double>(GetFPGATime() - r->m_BumpTestStartTime) * 1e-6;

	double current_time = static_cast<double>(GetFPGATime() - r->m_BumpTestStartTime) * 1e-6;

	double left_distance = r->m_PMU.m_leftEncoder.GetDistance();
	double right_distance = r->m_PMU.m_rightEncoder.GetDistance();

	double left_derived_rate = (left_distance - left_previous_distance) / (current_time - previous_time);

	double right_derived_rate = (right_distance - right_previous_distance) / (current_time - previous_time);

	double left_rate = r->m_PMU.m_leftEncoder.GetRate();
	double right_rate = r->m_PMU.m_rightEncoder.GetRate();

	s << current_time << "," << r->m_CO << ","
	  << left_distance << "," << left_rate << "," << left_derived_rate << ","
	  << right_distance << "," << right_rate << "," << right_derived_rate << std::endl;

	left_previous_distance = left_distance;
	right_previous_distance = right_distance;
	previous_time = current_time;

	r->GetWatchdog().Feed();
}

void CompetitionRobot1771::Autonomous(void) 
{
	GetWatchdog().SetEnabled(true);
	while(IsAutonomous() && IsEnabled())
	{
		GetWatchdog().Feed();
		std::ofstream csv("bump_test.csv", std::ios::trunc);
		Notifier csv_update(updateCSV, &csv);

		csv << "Time,CO,Left Distance,Left PV,Left Derived PV,Right Distance,Right PV,Right Derived PV\n";
		m_CO = 0;
		m_BumpTestStartTime = GetFPGATime();

		m_PMU.m_leftEncoder.Reset();
		m_PMU.m_rightEncoder.Reset();
		
		csv_update.StartPeriodic(0.005);

		foreach(CANJaguar* c, m_LeftJags)
		{
			c->Set(-m_CO);
		}
		foreach(CANJaguar* c, m_RightJags)
		{
			c->Set(m_CO);
		}
		Wait(0.1);
		
		m_CO = m_BumpTestCO;
		foreach(CANJaguar* c, m_LeftJags)
		{
			c->Set(-m_CO);
		}
		foreach(CANJaguar* c, m_RightJags)
		{
			c->Set(m_CO);
		}
		Wait(m_BumpTestTime);
		
		m_CO  = 0;
		foreach(CANJaguar* c, m_LeftJags)
		{
			c->Set(-m_CO);
		}
		foreach(CANJaguar* c, m_RightJags)
		{
			c->Set(m_CO);
		}
		Wait(m_BumpTestTime);

		csv_update.Stop();
	}
}

I agree that the problem is with the period returned by the FPGA.

[jhersh]

Originally Posted by vamfun

Edit: Looking at the counter constructor for the quad encoder...seems that if
InitCounter(kExternalDirection);
is present then
SetDownSourceEdge(inverted, true);
has a special meaning. It sets direction externally with chan B rather than actually change the down count with chan B edges as the name implies. If this is the case, I really have the urge to yell at an overworked WPI programmer for this style of coding.

This is directly related to the register interface for the counter IP in the FPGA where bits that are not used in a given mode are overloaded to control other parts of the logic. The real failure here is that I didn't clearly document what wicked things I was doing with those bits in the library code.

Allow me to do that here...

If you select "ExternalDirection" mode, then the following overloads apply:
"DownRisingEdge" specifies invert the direction or not.
"DownFallingEdge => true" enables quadrature decoding mode.
In quadrature decoding mode, "UpFallingEdge" selects 1x or 2x decoding and "UpRisingEdge" must be true... more literally, what these two mean in quadrature mode is:
"UpRisingEdge => count A if B is high"
"UpFallingEdge => count A if B is low"

Obvious, right? Again sorry for the confusion (I realize you probably aren't any less confused at this point).

Originally Posted by vamfun

Also still waiting for Joe or other expert to explain how B channel works to set count direction in WPI libs .
post http://www.chiefdelphi.com/forums/sh...3&postcount=24

Here are the much anticipated logic equations used in the 1x and 2x quadrature decoding mode of the counters.

Count = (ARising | AFalling) & ((BHigh & UpRisingEdge) | (BLow & UpFallingEdge))
CountUp = Count & !(BHigh ^ DownRisingEdge ^ ARising)
CountDown = Count & (BHigh ^ DownRisingEdge ^ ARising)

With this, there will hopefully be no more mystery.

See you in Atlanta!
-Joe

[vamfun]

Originally Posted by Bigcheese

Here's 4x decoding data with the same changes to Encoder.cpp as listed above.

http://spreadsheets.google.com/ccc?k...E1zX 2c&hl=en

And here's the relevant code for getting the data.



I agree that the problem is with the period returned by the FPGA.

Thanks for the code... both our encoders are broken so may not get to it for a while.

Question: What encoders did you use on left and right and what are the corresponding distances per pulse? The right encoder has a little less noise than the left.

I did a statistical analysis of your data and will post it soon.

If you still have the same set up and a little extra time, I'd love to see the 2x and 4x cases with the get rate averaged over 2 and 4 pulses respectively rather than the default single pulse. Please use the corresponding JAG speeds as in the original data sets. In case you aren't familiar with this:

The 2x is set in the counter.cpp InitCounter() line :
m_counter->writeTimerConfig_AverageSize(1, &status);
by changing to
m_counter->writeTimerConfig_AverageSize(2, &status);

Similarly, the 4x is set in the encoder.cpp InitEncoder() line:
m_encoder->writeTimerConfig_AverageSize(1, &status);
by changing to
m_encoder->writeTimerConfig_AverageSize(4, &status);

This would help mitigate the rate noise if people insist on using 2x and 4x encoder modes. I would like this to be the default configurations. If you run the cases Ill add it to the statistical analysis and post it also. It would be a nice reference for future First teams making this decision.

If you can't do it, I certainly understand. Thanks

By the way, last year Joe H and I had a thread on moving average : http://forums.usfirst.org/showthread...8143#post28143. Our team added a function to the Encoder.cpp that allows you to do this in your robot init code similar to SetDistancePerPulse(). Eventually, I hope the WPI guys incorporate this capability.

void SetMovingAveragePulseCount(int max_count)
{ //check if 1=<max_count<=127 TODO: should throw an error flag here with message if max_count invalid
if (max_count<1) max_count = 1;
else if (max_count>127) max_count = 127;
if(m_counter)
m_counter->m_counter->writeTimerConfig_AverageSize(max_count, &status); // Counter object within Encoder -> tCounter within Counter
else
m_encoder->writeTimerConfig_AverageSize(max_count, &status);
}

[Bigcheese]

Originally Posted by vamfun

Question: What encoders did you use on left and right and what are the corresponding distances per pulse? The right encoder has a little less noise than the left.

They are both the standard ones from andymark. 0.0054105234681583 feet per pulse. I have no idea why one is a lot noisier than the other.

Originally Posted by vamfun

If you still have the same set up and a little extra time, I'd love to see the 2x and 4x cases with the get rate averaged over 2 and 4 pulses respectively rather than the default single pulse. Please use the corresponding JAG speeds as in the original data sets. In case you aren't familiar with this:

The 2x is set in the counter.cpp InitCounter() line :
m_counter->writeTimerConfig_AverageSize(1, &status);
by changing to
m_counter->writeTimerConfig_AverageSize(2, &status);

Similarly, the 4x is set in the encoder.cpp InitEncoder() line:
m_encoder->writeTimerConfig_AverageSize(1, &status);
by changing to
m_encoder->writeTimerConfig_AverageSize(4, &status);

This would help mitigate the rate noise if people insist on using 2x and 4x encoder modes. I would like this to be the default configurations. If you run the cases Ill add it to the statistical analysis and post it also. It would be a nice reference for future First teams making this decision.

Thanks for the info I didn't know about that. We just packed up everything for Atlanta, so I won't be able to test it until after.

Originally Posted by vamfun

By the way, last year Joe H and I had a thread on moving average : http://forums.usfirst.org/showthread...8143#post28143. Our team added a function to the Encoder.cpp that allows you to do this in your robot init code similar to SetDistancePerPulse(). Eventually, I hope the WPI guys incorporate this capability.

void SetMovingAveragePulseCount(int max_count)
{ //check if 1=<max_count<=127 TODO: should throw an error flag here with message if max_count invalid
if (max_count<1) max_count = 1;
else if (max_count>127) max_count = 127;
if(m_counter)
m_counter->m_counter->writeTimerConfig_AverageSize(max_count, &status); // Counter object within Encoder -> tCounter within Counter
else
m_encoder->writeTimerConfig_AverageSize(max_count, &status);
}

Do the WPI people know about it? I would hope they add it. Also they need to apply that patch I posted. I need to figure out who to send it to...

[jhersh]

Originally Posted by Bigcheese

Do the WPI people know about it? I would hope they add it. Also they need to apply that patch I posted. I need to figure out who to send it to...

http://firstforge.wpi.edu/sf/tracker...wpilib/tracker

If you log in with a generic FIRST Forge user account, you can post bug reports.

-Joe

[vamfun]

Michael, I posted the rate statistics for segments of your data here.

http://www.chiefdelphi.com/forums/sh...519#post953519

Joe: Thanks for the info and good luck in Atlanta. Our factor of 2 GetPeriod() problem awaits your attention.

[jhersh]

Originally Posted by vamfun

Joe: Thanks for the info and good luck in Atlanta.

Seeing the logic for the decoder do you now see why you can't leave the B line static?

Originally Posted by vamfun

Our factor of 2 GetPeriod() problem awaits your attention.

I'll be looking into it after Atlanta. Please report each issue you are aware of on FIRST Forge. It will help us and all of you keep track of each issue and the current status of each of them.

-Joe

[vamfun]

Originally Posted by jhersh

"UpRisingEdge => count A if B is high"
"UpFallingEdge => count A if B is low"

Here you lost me. I would have been ok if you'd said
"DownRisingEdge => countup A if B is high"
"DownFallingEdge => countdown A if B is low"

Here are the much anticipated logic equations used in the 1x and 2x quadrature decoding mode of the counters.


Count = (ARising | AFalling) & ((BHigh & UpRisingEdge) | (BLow & UpFallingEdge))
CountUp = Count & !(BHigh ^ DownRisingEdge ^ ARising)
CountDown = Count & (BHigh ^ DownRisingEdge ^ ARising)

With this, there will hopefully be no more mystery.

Ok, I'm going to think outloud and see if you agree I see the light.
Lets try an example for 1x: UpRisingEdge = true , DownRisingEdge=false
Assume B is static high so Bhigh=true
Now A channel gets a rising pulse so ARising = true , AFalling= false
Evaluate 1)
Count = true;
(BHigh ^ DownRisingEdge ^ ARising)=(true^false^true) = false;
so
CountUp = true & !(false) = true
CountDown = true&false = false
Hence we count up ....
Now A channel gets a falling pulse so Arising = false, AFalling = true
Evaluate 2)
Count = true:
(BHigh ^ DownRisingEdge ^ ARising)=(true^false^false) = true;
so
CountUp= true &!(true)=false
CountDown = true&true = true
Hence we count down ....

This repeats so with a stuck B channel, it oscillates.

In the case where B is not static , it inhibits the count when AFalling = true because Bhigh= false during that transition.

So I guess I'm with you.

Since this logic requires a transition to count 1x properly, it seems the code should throw a flag when the B is static since this is an invalid input. Otherwise why require it.

[jhersh]

Originally Posted by vamfun

...So I guess I'm with you.

Excellent!

Originally Posted by vamfun

Since this logic requires a transition to count 1x properly, it seems the code should throw a flag when the B is static since this is an invalid input. Otherwise why require it.

What it is doing is by definition quadrature decoding. If it counted A without B transitioning, it would just be a simple counter, not be quadrature (which you can do if you select that mode on the counter open).

We shouldn't single out B being static... it's perfectly valid for the encoder to measure a change in direction as well as many changes in direction one after another.

-Joe

[jhersh]

Originally Posted by vamfun

Joe, I would expect that for 2x and 4x but not 1x, since with 1x only A channel leading edge determines the count and the B channel is always the same value on the leading edge. Unless I'm missing something.

Originally Posted by vamfun

Ok, lets get specific. When we write our own interrupt encoder routines, the B channel is just looked at when a TEU pulse occurs on the A chan to get the direction. However, the tcounters in CRIO are a bit of a mystery to me still and I need some more tutoring.

I think the fundamental misunderstanding here is that what you are doing in your microcontroller-based 1x decoder is take a shortcut that is not a valid quadrature decoder.

To create a valid 1x decoder, you must be sensitive to a different edge of A based on the value of B, not always be sensitive to one edge of A and choose direction based on B. Naturally the correct way to implement this is not simple on a microcontroller because interrupt configuration cannot be tied to a digital input, so the mistake is often made because it is seen as the only option. The fact that it almost works only reinforces to the misunderstanding.

In some cases is it a concious decision to use this optimization (which has half the interrupt traffic that would be required to implement it correctly) with the acceptance and acknowledgement that it will generate invalid counts any time the direction changes.

-Joe

[vamfun]

Originally Posted by jhersh

I think the fundamental misunderstanding here is that what you are doing in your microcontroller-based 1x decoder is take a shortcut that is not a valid quadrature decoder.

To create a valid 1x decoder, you must be sensitive to a different edge of A based on the value of B, not always be sensitive to one edge of A and choose direction based on B.

The scheme that I'm describing is a valid quadrature decoder in that it uses the B channel to determine direction. When going CW , the uprising A edge occurs when say B is High and when going CCW, the uprising A edge occurs when B is low. So when going a given direction, B is essentially static when looked at during the uprising edge.

Naturally the correct way to implement this is not simple on a microcontroller because interrupt configuration cannot be tied to a digital input, so the mistake is often made because it is seen as the only option. The fact that it almost works only reinforces to the misunderstanding.

Seems simple to me... in the A rising edge interrupt handler, we just read B digital port immediately and either countup or countdown pending value. The only requirement is that B not be changing for those few useconds. It does give half the interrupt traffic so we have been using this with MPLAB Vex for years primarily to conserve the interrupt ports.

]

[Alan Anderson]

Originally Posted by vamfun

The scheme that I'm describing is a valid quadrature decoder in that it uses the B channel to determine direction.

No, it's just a counter with a direction input. It happens to work with a quadrature encoder if you don't have "noisy" rotation. If the encoder happens to oscillate a bit back and forth at the A channel transition, you'll get false counts.

That exact situation happened on our "Triple Play" robot. We had quadrature encoders on miniature omniwheel-ish rollers to track our motion on the carpet. We could watch the wheels not spin appreciably while the code told us they were making several revolutions. I fixed it by implementing a true quadrature decoder. Something almost exactly like my code was independently developed by Kevin Watson and incorporated into his Encoder.c library for the IFI controller.

[vamfun]

Originally Posted by Alan Anderson

No, it's just a counter with a direction input. It happens to work with a quadrature encoder if you don't have "noisy" rotation. If the encoder happens to oscillate a bit back and forth at the A channel transition, you'll get false counts.

That exact situation happened on our "Triple Play" robot. We had quadrature encoders on miniature omniwheel-ish rollers to track our motion on the carpet. We could watch the wheels not spin appreciably while the code told us they were making several revolutions. I fixed it by implementing a true quadrature decoder. Something almost exactly like my code was independently developed by Kevin Watson and incorporated into his Encoder.c library for the IFI controller.

I call it quadrature only since it is uses the quadrature information, even though it might be a weaker algorithm. Perhaps it takes some liberty with industry conventions.

You make a good point Alan, regarding the sensitive nature of my scheme to vibration that can cause oscillation about an edge. It will certainly do what you said... but it is the price that must be paid if you want to use fewer interrupts. If this is not a constraint, then clearly the use of both rising edge and falling edges will help as you discovered.

These are the mappings as I see them:
my 1x scheme
1x , 1 interrupt per cycle, sensitive to edge oscillation,
rate sensitive to rising edge phase errors of A channel
A (rising) B (low) increment
A (rising) B (high) decrement

2x , 2 interrupts per cycle, not sensitive to oscillations
rate sensitive to rising and falling edge phase errors of A channel
A (rising) B (low) increment
A (falling) B (high) increment
A (rising) B (high) decrement
A (falling) B (low) decrement

4x , 4 interrupts per cycle, not sensitive to oscillations
rate sensitive to rising and falling edge phase errors of both A and B channels

A (rising) B (low) increment
B (rising) A (high) increment
A (falling) B (high) increment
B (falling) A (low) increment
A (rising) B (high) decrement
B (rising) A (low) decrement
A (falling) B (low) decrement
B (falling) A (high) decrement

Making a 1x insensitive to oscillations could be done a number of ways:
One simple mapping is to enable count when B (low) and count when B(high)
This requires B transitions for counting.
So..
1x, 2 interrupts per cycle, not sensitive to oscillations
rate sensitive to rising edge phase errors of A channel
A (rising) B (high) increment , reset enable
A (falling) B (high) decrement, reset enable
A (rising) B (low) enable count
A (falling) B (low) enable count

Ill have to go back and review how Kevin did his. Its been a while.

[vamfun]

Originally Posted by vamfun

Making a 1x insensitive to oscillations could be done a number of ways:
One simple mapping is to enable count when B (low) and count when B(high)
This requires B transitions for counting.
So..
1x, 2 interrupts per cycle, not sensitive to oscillations
rate sensitive to rising edge phase errors of A channel
A (rising) B (high) increment , reset enable
A (falling) B (high) decrement, reset enable
A (rising) B (low) enable count
A (falling) B (low) enable count

Ill have to go back and review how Kevin did his. Its been a while.

I took a look at Kevins code while watching the Einstein Final...
Congrats 67/177/294

In the same mapping style, it translates to:
A(falling) B(high)...................set Backward state
A(falling) B(low) ...................set Forward state
A(rising) B(low) Backward........decrement
A(rising) B(high) Forward.........increment

This works well to prevent edge oscillation error counts since it requires a B transition to count also.
Kevin used the single interrupt scheme on the first two encoder channels and this two interrupt scheme on the three and four channels. He had the foresight to know that sometimes you need fewer interrupts with high velocity encoders and more interrupts with high precision distance encoders.

Note to Joe H: For your 1x, I don't like your algorithm because the count oscillates when stuck with an oscillating A edge over a BHigh or Blow state. I believe this can really aggravate the GetRate() noise.

Count = (ARising | AFalling) & ((BHigh & UpRisingEdge) | (BLow & UpFallingEdge))
CountUp = Count & !(BHigh ^ DownRisingEdge ^ ARising)
CountDown = Count & (BHigh ^ DownRisingEdge ^ ARising)

Revisiting the 1x example with DownRisingEdge=false,UpRisingEdge= true this simplifies to:

CountUp = BHigh&ARising
CountDown = Bhigh&!Arising --> Bhigh&Afalling if counting.

You count up or down on the A edge transition if Bhigh so you oscillate between +1 and -1 increments. My 1x non-oscillating scheme and Kevin's will not do this. Mine will allow a single proper count in the normal direction and no change on the oscillatory reversal. Kevin's will not allow any count until B changes sign.

[jhersh]

Originally Posted by vamfun

Note to Joe H: For your 1x, I don't like your algorithm because the count oscillates when stuck with an oscillating A edge over a BHigh or Blow state. I believe this can really aggravate the GetRate() noise.

...

You count up or down on the A edge transition if Bhigh so you oscillate between +1 and -1 increments. My 1x non-oscillating scheme and Kevin's will not do this. Mine will allow a single proper count in the normal direction and no change on the oscillatory reversal. Kevin's will not allow any count until B changes sign.

What's a proper count? If the lines did it, it gets decoded. It seems unreasonable for the decoder to make assumptions about what the sensor meant to encode. The way I've implemented it allows you to read the true position at any point independent of the rate calculation. I then address the concern you have for the rate measurement independently by ignoring any count that has changed direction for the purposes of calculating rate.

Because I'm using an FPGA, I can afford to implement the decoder correctly without regard for shortcuts in interrupt handling. The simple fact that your interrupt handler can't check the state of the B line at the exact instance that the A line edge is detected means that you have a race condition to count the wrong way anyway... and it's a pretty big hole with most interrupt handlers, especially on the PIC18F where you don't get hardware context saving and it's just generally a very slow processor. Without hardware support, you can't really trust the decoding in the face of noise.

-Joe