Strange behaviour when reading digital pins

Hello,

I have a encoder connected to 6 digital inputs. The encorder outputs a Greycode signal, 6 bits.

During the development I have used several Serial.print-statements to debug the conversion from Greycode to binary. And when I was happy with the result I removed the Serial.print-statements. This is when it all went wrong. The Grecode-to-binary conversion suddenly yielded a different result. I then added a couple of Serial.print-statements and the result was all of a sudden correct again. This baffeled me a bit :)

I scratched my head and tried to search with diferent keywords. I came accross a thread here which suggested using delay(1).

I then added delay(1) without Serial.print-statements, and the result was correct. I tried moving the delay(1) around to see where the need occoured, but it does not matter where I put it, as long as it apears AFTER I read the digital inputs. Putting it in front gives the wrong result. Can anyone explain why, what og where? :)

I understand that Serial.print creates a delay, so I understand why delay(1) works just as good as Serial.print, but I wonder why?

byte greycode;

if (digitalRead(PIN_SENSOR_WAVC)) { bitSet(greycode, 5); } else { bitClear(greycode, 5); }
if (digitalRead(PIN_SENSOR_WAVD)) { bitSet(greycode, 4); } else { bitClear(greycode, 4); }
if (digitalRead(PIN_SENSOR_WAVE)) { bitSet(greycode, 3); } else { bitClear(greycode, 3); }
if (digitalRead(PIN_SENSOR_WAVF)) { bitSet(greycode, 2); } else { bitClear(greycode, 2); }
if (digitalRead(PIN_SENSOR_WAVG)) { bitSet(greycode, 1); } else { bitClear(greycode, 1); }
if (digitalRead(PIN_SENSOR_WAVH)) { bitSet(greycode, 0); } else { bitClear(greycode, 0); }
delay(1);

The code above runs two times pr second. Any suggestions are apreciated. :)

Here is the thread I got the delay(1) tips from: http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1272024322

It would be useful to see what happens after this code snippet. Where/how does greycode get used?

Why is greycode not explicitly initialized before this block of code?

The byte variable greycode is just a temp variable used localy while converting to a binary byte, which is a global variable.

Here is the whole code-block which reads this spesific encoder:

if (millis() - _SENSOR_WAV_LASTREAD > 500) {
  byte greycode;
  int tempvalue;

  if (digitalRead(PIN_SENSOR_WAVC)) { bitSet(greycode, 5); } else { bitClear(greycode, 5); }
  if (digitalRead(PIN_SENSOR_WAVD)) { bitSet(greycode, 4); } else { bitClear(greycode, 4); }
  if (digitalRead(PIN_SENSOR_WAVE)) { bitSet(greycode, 3); } else { bitClear(greycode, 3); }
  if (digitalRead(PIN_SENSOR_WAVF)) { bitSet(greycode, 2); } else { bitClear(greycode, 2); }
  if (digitalRead(PIN_SENSOR_WAVG)) { bitSet(greycode, 1); } else { bitClear(greycode, 1); }
  if (digitalRead(PIN_SENSOR_WAVH)) { bitSet(greycode, 0); } else { bitClear(greycode, 0); }
  delay(1);
  _SENSOR_WAV_VALUE = int(5.61 * int(GrayToBin(greycode))); //Convert the greycode to natural binary. Convert the natural binary to integer and multiply it with 5.61(number of degrees for each step).
  tempvalue = _SENSOR_WAV_VALUE; //Create a temp value to convert the integer value to BCD.
  _SENSOR_WAV_VALUE1 = 0x00; //Make sure that the variable is 0x00.
  _SENSOR_WAV_VALUE2 = 0x00; //Make sure that the variable is 0x00.

  // Check if there is a fouth digit.
  if (tempvalue > 999) {
    _SENSOR_WAV_VALUE1 = byte(int(tempvalue / 1000)); //Convert int to byte.
    tempvalue = tempvalue - (int(tempvalue / 1000) * 1000); //Remove the digit which we have converted.
    _SENSOR_WAV_VALUE1 = _SENSOR_WAV_VALUE1 << 4; // Shift the bytes 4 bits down.
  }

  // Check if there is a third digit.
  if (tempvalue > 99) {
    _SENSOR_WAV_VALUE1 |= byte(int(tempvalue / 100)); //Convert int to byte, and add this to the beginning of variable byte.
    tempvalue = tempvalue - (int(tempvalue / 100) * 100); // Remove the digit which we now converted.
  }

  // Check if there is a second digit.
  if (tempvalue > 9) {
    _SENSOR_WAV_VALUE2 = byte(int(tempvalue / 10)); // Convert int to byte.
    tempvalue = tempvalue - (int(tempvalue / 10) * 10); // Remove the digit which we now converted.
    _SENSOR_WAV_VALUE2 = _SENSOR_WAV_VALUE2 << 4; // Shift the byte 4 bits down.
  }

  // There is always one digit.
  _SENSOR_WAV_VALUE2 |= byte(int(tempvalue)); //Convert int to byte and add this to the beginning of the byte.
  _SENSOR_WAV_LASTREAD = millis();  //Store millis when last executed.
}

This code reads a wind vane that has a encoder inside which outputs a greycode specifies the wind direction. The end result is that the direction(degress from 0 to 359) is converted to BCD format in _SENSOR_WAV_VALUE1(bit 7 to 4 for digit 4 and bit 3 to 0 for digit 3) and in _SENSOR_WAV_VALUE2(bit 7 to 4 for digit 2 and bit 3 to 0 for digit 1).

The result is correct as long as the delay(1) line is in the code. If I remove it or moves it in front of the input-reading the result is wrong.

Edit 1: Added comments to the code.
Edit 2: Changed some comments.

The reason that I asked about the explicit initialization of greycode is that none of the bitclear commands is necessary, if you explicitly set greycode to 0 to start with (all the bits will be clear).

What is GrayToBin doing? The comment says "Convert the greycode to binary", but greycode is clearly binary to start with (since you set it one bit at a time).

"Convert the binary to integer". This is a useless comment. An integer is a whole number. Whether that whole number is expressed in binary, octal, or decimal notation, the internal storage on the Arduino is exactly the same. Certain notations make it easier to input the value to be stored, or to interpret the contains of that memory space.

Multiplying an int by 5.61 and storing the result in an int seems like a strange thing to do. Not wrong, just strange. The comments would be more useful if they explained why the value of 5.61 is used, and why the result of the multiplication is stored in an int.

None of this has anything to do with why the delay is necessary. The need for the delay does not appear to be in this code, though.

Thank you for your time/help so far :)

Yes, the bitClear is redundant. At first the variable was global, so it retained it's value for each execution. Then I changed it with a private variable because I don't need the greycode-value once the code-block is done.

Although greycode is binary, it's not natural binary code but reflected binary code. To get a value I can use I need to convert it to natural binary. That is what the GreyToBin-function does. I will change the comment to reflect that :)

My comments may seem badly frased, and I might convert bits and bytes unnessesary. This is my first code with real bit modifying, so I blame the lack of knowledge.

Multiplying the int with 5.61 is a bit strange I know. The documentation for the encoder says that each step in the greycode is 6 degrees. While working with the output I saw that 6 degress where wrong and replaced it with 5.61. And the reason I use int in stead of float or double is because I need whole numbers, not floating numbers.

Yes, the need for delay(1) is strange. I can put the delay(1) anywhere in the code as long as it is within the code(even in the GreyToBin-function) and after digitalRead.

5.61 is a strange value for a six bit encoder - why not 5.625?

The manual for the encoder says 6 degrees for each step. Then I convert the reflected binary to natural binary and end up with something like 9 or 60(which should be number of steps in total). Then I multiply this with 6(degrees for each step) and the result is correct only when the natural binary is 1. When I multiply 6 with ex 63(64 is the highest step number) I get 378, which is wrong because the highest number of degrees are 354.

So, to correct this i divided 354 with 64 and ended up with 5.53, which I manually corrected to 5.61 :)

To illustrate the reason for 5.61 when multiplying:
Step 1 = 0
Step 2 = 6
Step 3 = 11
Step 4 = 17

Step 64 = 354

Here is the manual for the sensor/encoder: http://www.vaisala.com/Vaisala%20Documents/User%20Guides%20and%20Quick%20Ref%20Guides/WAV151_User_Guide_in_English.pdf

On page 26 there is a table illustrating the values I should end up with.

And, it seems that I mixed up something on the way. The manual says 5.6 resolution. I guess I must have read 6 degrees resolution from some product page or similar.

Anywho, that code works pretty well. Besides the need for delay(1) :slight_smile:

Wel,, the way I figured it was a 6bit encoder has 64 states, so 360(degrees) / 64 = 5.625.

Yes, but step 64 should return 354, not 360 :)

Step 0 = 0
Step 1 = 6
Step 2 = 11
Step 3 = 17

Step 64 = 354

Are you sure both of the highlighted steps are present, i.e that there are 65 distinct steps? :-?

Good eye :)

Corrected.

Exactly how much delay is needed? What if you replace delay(1) with delayMicroseconds(1000) and keep reducing it?

I changed delay(1) with delayMicroseconds(1000), which should be the same, and the result was wrong again. Increased to 1500(which should be more than delay(1), same result(wrong).

Changed back to delay(1) and the result where correct again.

I'm stuck :)

delay(1) is rather imprecise as it can give 1-2 ms delay depending on where in the counting cycle timer0 was. Does delayMicroseconds(2500) help? In what way are the results "wrong"?

Ok. Finally found the problem :)

I commented out most of the code, created to global byte vars containing the greycode and natural binary code. I then added a function that prints the bits in these bytes with Serial.print(in main loop, which won't interfere with the original problem). What I then discovered was that the MSB in the converted byte always where 1 when the delay(1) where removed, and 0 when delay(1) where in the code. Strange.

I've not posted that part of the code before. It's the GreyToBin function. Here it goes(modified):

byte newgray = 0; // changed this from "byte newgray;"
int lastbit;

for (int x=5; x >= 0; x--) {
  if (x == 5) {
    bitWrite(newgray, x, bitRead(gray, x));
    lastbit = bitRead(gray, x);
  } else {
    if (lastbit == bitRead(gray, x)) {
      bitClear(newgray, x);
      lastbit = 0;
    } else {
      bitSet(newgray, x);
      lastbit = 1;
    }
  }
}

return newgray;

Thank you for all your help and time people. I still don't understand why the MSB in a new byte is 1 without delay, and 0 with.

You might have problems with the code if the greycode reading changes fast enough.

Because you are doing digitalreads on each pin, at the time in-between pin reads, you could potentially have a change in the values being presented to the pins.

It would be better to instead read the pins as a group, using direct port access (of course, the pins used would all have to be on a single port).

That way, you could read all the pins at one instance in time. From there, you would then want to use an interrupt on a pin-change on that port, and store the value in your ISR to a global variable to use in the rest of your code.

:)

Are you saying that "newgray" sometimes has MSB set when running that code, even when you initialize it to 0 at the beginning? Does that bug disappear if you put Serial.prints inside GreyToBin? None of the bitset/bitclear/bitwrite seems to be guilty since x is never 7.

The last code there only changed bit 0 to 5(6 bits). Bit 6 and 7 where never touched as you said.

In the original code, which had the problem, I only created the byte var newgray. Then, when I had the problems(without delay(1)-line), I noticed that bit 7 where set to 1. With the delay(1)-line that specific bit where 0, as I assumed it would be since I only changed bit 0 to 5.

So I changed to code to set the byte var newgray to all 0, and then the problem where solved.