0
\$\begingroup\$

I am trying to generate a Clock synchronized with data, where the frequency of the clock should be twice the frequency of the data. I wrote a code for this purpose and it works fine for 32bits of data, anything more than 32 it will generate a bunch of 1's. Here is what I got for 64bit data=(0XAAAAAAAAAAAAAAAA):

enter image description here

Here is the code I wrote:

  void DATA(unsigned long  int number, int iteration){   //number: data to be send serially
                                                       // iteration: number of bits to send
  unsigned long  int x=0;         //An integer to hold each bit from the data                      
  char even = (char)0x1; 
  TIMER1->ICR |= (1<<0);
  for(int y=0; y<=iteration;) {
     while((TIMER1->RIS & 0x00000001) != 1){}   //wait until Timer1 times out
      TIMER1->ICR |= (1<<0);     //Reset Timer1 flag
      odd ^= (char)0x1;         
      GPIOC->DATA ^= (1<<7);     //Toggle the clock at PC7
      if (odd) continue;        // data change every even cycle
      x = number & (0x01UL << y);      //Get each bit individually  
      if(x==(0x01UL<<y))           //If bit is 1
          GPIOC->DATA |= (1<<6); //Make C6 High 
      else if(x==0X0)            //If bit is 0
          GPIOC->DATA &= ~(1<<6);//Make C6 Low
      y++;
  }
  }

  int main(){ 
     DATA(0XAAAAAAAAAAAAAAAA,64);//
   }

Note: I have tried to make the number in my function to be long long int, and that didn't solve the problem too. Is there any explanation for getting those 1's after the first 32 bits ?

\$\endgroup\$
  • \$\begingroup\$ Have you stepped through the code in the debugger? \$\endgroup\$ – kkrambo Sep 4 '16 at 22:23
  • \$\begingroup\$ @kkrambo Yes, I did. The MCU is reading 1's in the last 32 bit's, I don't know why. \$\endgroup\$ – Abdelrahman Elshafiey Sep 4 '16 at 22:31
  • 2
    \$\begingroup\$ Why aren't you using explicitly sized types such as uint64_t? \$\endgroup\$ – Brian Drummond Sep 5 '16 at 6:16
  • \$\begingroup\$ Yeah, start by getting rid of int and long in favour for sane types from stdint.h. Then ensure that all integer literals have a large enough type. Then study implicit type promotion: Google "the usual arithmetic conversions". Then adopt a consistent indention and brace style. Then stop writing really obscure nonsense like for(int y=0; y<=iteration;)... y++.Somewhere along the way, the bug will likely disappear. As this code currently stands, it looks quite unsalvagable. \$\endgroup\$ – Lundin Sep 5 '16 at 6:58
  • 1
    \$\begingroup\$ an unsigned long int in your compiler seems to be only 32 bits. So when the code tries to reference a 33 bit, it is referencing trash. suggest using a parameter of type: uint64_t Then you could go to 64 for iteration. However, if you want to be able to use data of any length then use: char* number then index through the data 8 bits at a time. Note: this may result in the need of a third parameter, indicating how many characters (or bits) are in the data. \$\endgroup\$ – user3629249 Sep 6 '16 at 20:36
2
\$\begingroup\$

Just for reference, here's how I would code your function:

// number: data to be send serially
// iteration: number of bits to send
void DATA (unsigned long int number, int iteration)
{
  while (iteration--) {
    TIMER1->ICR |= (1<<0);       //Reset Timer1 flag
    while (!(TIMER1->RIS & 0x01)){} //wait until Timer1 times out
    GPIOC->DATA |= (1<<7);       //Toggle the clock at PC7 high

    if (number & 0x01)           //If bit is 1
      GPIOC->DATA |= (1<<6);     //Make C6 High 
    else                         //If bit is 0
      GPIOC->DATA &= ~(1<<6);    //Make C6 Low
    number >>= 1;

    TIMER1->ICR |= (1<<0);       //Reset Timer1 flag
    while (!(TIMER1->RIS & 0x01)){} //wait until Timer1 times out
    GPIOC->DATA &= ~(1<<7);      //Toggle the clock at PC7 low
  }
}

Note that all of your internal variables are completely unnecessary. I've also unrolled the loop to make the polarity of the clock edges explicit.


EDIT: In order to be able to send arbitrary amounts of data, a function that accepts an array of bytes would probably be more useful:

// p: pointer to data to be sent serially
// n: number of bytes to send
void data_str (unsigned char *p, int n)
{
  while (n--) {
    char c = *p++;
    int i;

    for (i=0; i<8; ++i) {
      TIMER1->ICR |= (1<<0);       //Reset Timer1 flag
      while (!(TIMER1->RIS & 0x01)){} //wait until Timer1 times out
      GPIOC->DATA |= (1<<7);       //Toggle the clock at PC7 high

      if (c & 0x01)                //If bit is 1
        GPIOC->DATA |= (1<<6);     //Make C6 High 
      else                         //If bit is 0
        GPIOC->DATA &= ~(1<<6);    //Make C6 Low
      c >>= 1;

      TIMER1->ICR |= (1<<0);       //Reset Timer1 flag
      while (!(TIMER1->RIS & 0x01)){} //wait until Timer1 times out
      GPIOC->DATA &= ~(1<<7);      //Toggle the clock at PC7 low
    }
  }
}

// Send 128 bits (16 bytes) of data.
data_str ("\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA\xAA", 16);

Now we need two temporary variables: c holds the data so that we don't actually modify the caller's array, and i is used as a counter for the inner loop. Note that the bytes are sent left-to-right, but each byte is sent LSB-first.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Yes, that is definitely better. \$\endgroup\$ – Abdelrahman Elshafiey Sep 5 '16 at 23:03
  • 1
    \$\begingroup\$ Sorry, I messed up the data phasing relative to the clock. See the edit above for a corrected version. \$\endgroup\$ – Dave Tweed Sep 5 '16 at 23:21
  • \$\begingroup\$ Is there a data type for 128 bits ? \$\endgroup\$ – Abdelrahman Elshafiey Sep 6 '16 at 3:55
  • 1
    \$\begingroup\$ Read your compiler documentation. Some support a long long type. However, a more general solution would be to rewrite the function to accept an array of byes of arbitrary length. \$\endgroup\$ – Dave Tweed Sep 6 '16 at 10:36
1
\$\begingroup\$

I believe the problem is in the line

X = number & (0x01 << y);

The problem is that the result of 0x01 << y is an integer because 0x01 fits in one. Change this to read

X = number & (0x01UL << y);

You should also change every long to an unsigned long to avoid problems with 64-bit values.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Ok, I will try that. \$\endgroup\$ – Abdelrahman Elshafiey Sep 4 '16 at 21:42
  • \$\begingroup\$ I am still getting the same problem. \$\endgroup\$ – Abdelrahman Elshafiey Sep 4 '16 at 21:46
  • \$\begingroup\$ You need to apply the same fix to the if(x==(0X1<<y)) line. Also, it isn't clear how this is working in the first place, becausey is counting iterations (half cycles of the clock), not bits. \$\endgroup\$ – Dave Tweed Sep 4 '16 at 22:16
  • \$\begingroup\$ The y is only incrementing every even cycle, which is the number of bits I need. The y is also shifting a '1' to the left every even cycle, to the next bit I need. Sorry if the code is not clear, I know it's not a readable one. When I changed 'if(x==(0X1<<y))' to 'if(x==(0x01UL<<y))' still not working. \$\endgroup\$ – Abdelrahman Elshafiey Sep 4 '16 at 22:41
  • \$\begingroup\$ @DaveTweed: you are right about changing the type of 0x01, it should be uint64_t. Thanks for helping \$\endgroup\$ – Abdelrahman Elshafiey Sep 5 '16 at 19:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.