 # 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 ={ 0, 1, 2, 3, 6, 4, 5,11,15,14,13,10, 9, 7,12,08};
//examples:  Q0 goes to PINC . . . Q11 goes to PINC . . . Q8 connects to PIND

//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.

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 . . . Q11 goes to PINC . . . Q8 connects to PIND

//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

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 ={ 0, 1, 2, 3, 6, 4, 5,11,15,14,13,10, 9, 7,12,08};
//examples:  Q0 goes to PINC . . . Q11 goes to PINC . . . Q8 connects to PIND

//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={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 )
{
}
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.