A better way to do my bit reordering?

I have a 4040 binary counter (I use it as a prescaler) which is attached to PORT C and D on a Atmega 1284.
Q1 through Q12 are connected in such a way that there isn’t a 1 to 1 relationship to the Ports.
As a result I wrote this code section to place the bits in proper order.

The code woks fine, however I don’t like the using the “if” statement and feel like things could be done better.

Considering speed of execution, would you recommend a more concise way for doing the reordering of the bits?

Thanks

// function to read the Pre-Scaler
void PreScaler ()
{
  //Outputs from the Pre-Scaler (Q0 to Q15) go to Ports C and D. They are wired as shown in the array actualBits[]
  //                  |-------PORTC--------|  |-------PORTD--------|
  //                  0, 1, 2, 3, 4, 5, 6, 7  0, 1, 2, 3, 4, 5, 6, 7 
  //actualBits[16] ={ 0, 1, 2, 3, 6, 4, 5,11,15,14,13,10, 9, 7,12,08}; 
  //examples:  Q0 goes to PINC[0] . . . Q11 goes to PINC[7] . . . Q8 connects to PIND[7]

  //read the current value coming from the Pre-Scaler
  unsigned int y = (PIND << 8) | PINC;  
  //y now contains the bit values, according to the way things are wired.
  //Bit position in y                   0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F
  //Value of the Pre-Scaler output pin 00,01,02,03,06,04,05,11,15,14,13,10,09,07,12,08

  unsigned long actual = 0;  //this will be the corrected value when all bits are place in their proper position

  //now let's place all the bits in their proper positions
  for (int i = 0; i < 16; i++)
  {
    byte bitValue = bitRead(y , i); //get the current value for this bit
    byte x = actualBits[i];         //get the actual position for this bit

    if (bitValue == 1)
    {
      actual = actual | 1 << x;     //put the bit in the proper location
    }
  }

  //now that we have all the bits in the proper order we remove bits 12 thru 15 as they don't exist
  //bit position      FEDCBA9876543210 
  actual = actual & 0b0000111111111111;
  botPScounter =actual;
  return;
}

//                       End of PreScaler

Can't the counter outputs be wired to the port inputs in a more straightforward way? Sure would save a lot of trouble!

I would just do 16 straight masks and 16 straight assignments with shifts if needed:

actual = 0;
actual = actual | ( (0b0000000000000001 & remapped) << # needed ); // assign bit 0
actual = actual | ( (0b0000000000000010 & remapped) << or >> # needed); // assign bit 1
etc.
actual = actual | ( (0b1000000000000000 & remapped) << or >> # needed); // assign  bit 15

16 operations, done. No if loop needed, so it’s fast too. Don’t care if the mask results in 0 of 1, just OR the result 1 or 0 into position.

PCB is made already. @Crossroads Neat. I'll give it a try and compare it for speed.

PCB is made already.

Then shoot the designer (It wasn't you was it :)), that is unforgivable unless there is a good reason that's not obvious.

I tried to think of a clever way to do this but that mapping is so bad I don't think there is a good way short of a 65k lookup table.

You could reduce the loop by 4 iterations as the lower 4 bits are correct.


Rob

Yes it was me. BANG!
The reason is I only use single sided boards.
With a SS PCB there were no jumpers needed when this configuration is laid down.
If I could do double sided PCBs with plated through holes I would have done it the direct way.

Anyway, I came to this compromise, works great. (an offshoot from Crossroads idea)

// function to read the Pre-Scaler
unsigned int PreScaler ()
{
  //Outputs from the Pre-Scaler (Q0 to Q15) go to Ports C and D. They are wired as shown in the array actualBits[]
  //                         |-------PORTC--------|  |-------PORTD--------|
  //                         0, 1, 2, 3, 4, 5, 6, 7  0, 1, 2, 3, 4, 5, 6, 7 
  //actual prescaler bits =  0, 1, 2, 3, 6, 4, 5,11,15,14,13,10, 9, 7,12,08 
  //examples:  Q0 goes to PINC[0] . . . Q11 goes to PINC[7] . . . Q8 connects to PIND[7]

  //read the current value coming from the Pre-Scaler
  unsigned int y = (PIND << 8) | PINC;  
  //y now contains the bit values, according to the way things are wired.
  //Bit position in y                   0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F
  //Value of the Pre-Scaler output pin 00,01,02,03,06,04,05,11,15,14,13,10,09,07,12,08
  //in reality, bits 12, 13, 14 and 15 do not exist.

  unsigned int actual = 0;  //this will be the corrected value when all bits are place in their proper position

  //now let's place all the bits in their proper positions
  bitWrite(actual, 0, bitRead(y,0));
  bitWrite(actual, 1, bitRead(y,1));
  bitWrite(actual, 2, bitRead(y,2));
  bitWrite(actual, 3, bitRead(y,3));
  bitWrite(actual, 4, bitRead(y,5));
  bitWrite(actual, 5, bitRead(y,6));
  bitWrite(actual, 6, bitRead(y,4));
  bitWrite(actual, 7, bitRead(y,13));
  bitWrite(actual, 8, bitRead(y,15));
  bitWrite(actual, 9, bitRead(y,12));
  bitWrite(actual, 10, bitRead(y,11));
  bitWrite(actual, 11, bitRead(y,7));

  return actual;
}

//               End of PreScaler()

Check out this recent conversation on AVR Freaks: http://www.avrfreaks.net/index.php?name=PNphpBB2&file=viewtopic&t=145208&start=0 (And in particular "__builtin_avr_insert_bits()")

I sort-of like your current code; it's very general and obvious. Does it actually need to be faster? You can probably speed it up by making the actualBits[] array be 16bits wide and contain a mask instead of a bit number, which turns the inside of your loop into:

if (bitValue == 1)
    {
      actual = actual |  x;     //Or in the actual bit value.
    }

(multibit shifts by a variable amount being particularly slow on an AVR... Another speedup might be to shift y in the loop instead of using bitread. That would ensure shifting by a constant 1 position.)

it's very general and obvious.

That's what I thought too.

__builtin_avr_insert_bits()

Way out of my league :astonished:

Prescaler bit 11 also goes to D2 (int0) where it is used to manage the overflows.

Doesn't need to be faster as the fastest signal I will be looking at will be 60Mhz. 60 MHZ / 4096 ~ 15 Khz well within the capability of the 16Mhz 1284Weeny.

The reason is I only use single sided boards.

OK, you're forgiven :)


Rob

another trick is to prevent shifts as much as possible to speed up performance

  • fetch the bit from the variable y one at a time by shifting them from y
  • have a pre calculated array of masks with the right location

code is not tested but you should get the idea

// function to read the Pre-Scaler
void PreScaler ()
{
  //Outputs from the Pre-Scaler (Q0 to Q15) go to Ports C and D. They are wired as shown in the array actualBits[]
  //                  |-------PORTC--------|  |-------PORTD--------|
  //                  0, 1, 2, 3, 4, 5, 6, 7  0, 1, 2, 3, 4, 5, 6, 7 
  //actualBits[16] ={ 0, 1, 2, 3, 6, 4, 5,11,15,14,13,10, 9, 7,12,08}; 
  //examples:  Q0 goes to PINC[0] . . . Q11 goes to PINC[7] . . . Q8 connects to PIND[7]

  //read the current value coming from the Pre-Scaler
  unsigned int y = (PIND << 8) | PINC;  
  //y now contains the bit values, according to the way things are wired.
  //Bit position in y                   0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F
  //Value of the Pre-Scaler output pin 00,01,02,03,06,04,05,11,15,14,13,10,09,07,12,08

  unsigned long actual = 0;  //this will be the corrected value when all bits are place in their proper position

  //now let's place all the bits in their proper positions
  uint16_t masks[16]={1, 2, 4, 8, 64, 16, 32, 2048, 32768, 16384, 8192, 1024, 512, 128, 4096, 256};
  
  for (byte i = 0; i < 16; i++)
  {
    if ( y & 1 )
    {
      actual |=  masks[i]; 
    }
    y >>= 1;
  }

  //now that we have all the bits in the proper order we remove bits 12 thru 15 as they don't exist
  //bit position      FEDCBA9876543210 
  actual = actual & 0b0000111111111111;
  botPScounter = actual;
  return;
}

In the original just use the bit to prime the shift, so

    if (bitValue == 1)
    {
      actual = actual | 1 << x;     //put the bit in the proper location
    }

becomes:

    actual |= bitValue << x;     //put the bit in the proper location

or even:

    bitWrite (actual, x, bitValue);     //put the bit in the proper location

Then the whole loop can become the much more concise:

  for (int i = 0; i < 16; i++)
  {
    bitWrite (actual, actualBits[i], bitRead (y, i)) ;    //put the bit in the proper location
  }

Thas all for sharing your skill sets.