Can these two lines of code be simplified?

[OITGUY]

PORTB ^= ~( 1 << pos );
PORTB = ~PORTB;

Just looking for to learn if there are any tricks to making this type of thing more concise. Thank you.

[ninja9578]

Well, comments are always nice :P

PORTB ^= ~(1 << pos);  //PORTB XOR  the 2's compliment of 2 ^ pos
PORTB = ~PORTB;  //2's compliment of PORTB

If you want something more readable

#define TIMES_TWO_TO_THE_POWER_OF\
   <<
#define BINARY_NEGATION(x)\
   ~(x)
#define SELF_XOR(var, x)\
   var ~= x

SELF_XOR(PORTB, BINARY_NEGATION(1 TIMES_TWO_TO_THE_POWER_OF pos);
PORTB = BINARY_NEGATION(PORTB);

I personally find the latter harder to understand, but I've been using the binary operators for quiet some time

[nuzzle]

PORTB ^= ~( 1 << pos );
PORTB = ~PORTB;

Just looking for to learn if there are any tricks to making this type of thing more concise. Thank you.

What about a one-liner,

PORTB = ~(PORTB ^ ~( 1 << pos ));

For the sake of easier manipulation, you have this equivalent expression

A = ~(A ^ ~B);

which you can manipulate into

A = ~((A & B) | (~A & ~B));
A = ~~(A ^ B);
A = (A ^ B);

So the expression can be reduced to

PORTB = PORTB ^ (1 << pos);

or

PORTB ^= (1 << pos);

Once you've come up with a Boolean (or bitwise Boolean) expression you can always manipulate it to a minimal form using the rules of Boolean algebra.

[ninja9578]

Ohhh, you meant a boolean algebra simplification. My mistake, I thought you meant you were confused by the notation, the word concise threw me off.

[OITGUY]

Thanks Nuzzle for the obvious one-liner that illuded me & the boolean simplification. I have learned to comment my code next time as this is my first thread- thanks ninja9578.

This code is used for clearing or setting an output for Atmel AVR microcontrollers if anyone is curious.

The function on a tutorial reads (our one-liner replaces the entire if-else statement):


int set_PORTB_bit(int position, int value)
{
// Sets or clears the bit in position 'position'
// either high or low (1 or 0) to match 'value'.
// Leaves all other bits in PORTB unchanged.

if (value == 0)
{
PORTB &= ~(1 << position); // Set bit position low
}
else
{
PORTB |= (1 << position); // Set high, leave others alone
}

return 1;
}

[OITGUY]

Here is the same code as last reply except formatted (newbie here):

int set_PORTB_bit(int position, int value)
{
  // Sets or clears the bit in position 'position' 
  // either high or low (1 or 0) to match 'value'.
  // Leaves all other bits in PORTB unchanged.
                
  if (value == 0)
  {
    PORTB &= ~(1 << position);      // Set bit position low
  }
  else
  {
    PORTB |= (1 << position);       // Set high, leave others alone
  }

  return 1;
}

equal to:

int set_PORTB_bit(int position, int value)
{
   PORTB ^= (1 << pos);             // Set binary digit at index of 'position' with 'value', leave others unaltered
}

[VladimirF]

int set_PORTB_bit(int position, int value)
{
   PORTB ^= (1 << pos);             // Set binary digit at index of 'position' with 'value', leave others unaltered
}

I don’t think you should end a thread with multiple typos
In your final statement the name “pos” is undefined and the parameter “value” is not used.

[ninja9578]

Right, that code is not equal to the original.

[OITGUY]

CORRECTED CODE:

int set_PORTB_bit(int position, int value)
{
   PORTB ^= (value << position);             // Set binary digit at index of 'position' with 'value', leave others unaltered

   return 1;
}

[treuss]

It's still not equivalent. For value 0 it won't do anything (and not clear the bit). As value is an int, it can also have values different from 0 or 1 and while the original function interprets any value other than 0 as "set the bit", your function will modify bits other than the one at position.

[OITGUY]

Thank you treuss.

Original code seeking to simplify:

int set_PORTB_bit(int position, int value)
{
  // Sets or clears the bit in position 'position' 
  // either high or low (1 or 0) to match 'value'.
  // Leaves all other bits in PORTB unchanged.
                
  if (value == 0)
  {
    PORTB &= ~(1 << position);      // Set bit position low
  }
  else
  {
    PORTB |= (1 << position);       // Set high, leave others alone
  }

  return 1;
}

Backtracking and starting over I see very little that can be done to make this code more concise, the following is slightly revised:

int set_PORTB_bit(int position, int value)
{
  // Sets or clears the bit in position 'position' 
  // either high or low (1 or 0) to match 'value'.
  // Leaves all other bits in PORTB unchanged.
                
  if (value)
    PORTB |= (1 << position);       // Set high, leave others alone
  else
    PORTB &= ~(1 << position);      // Set bit position low

  return 1;
}

The challenge I suppose is eliminating the if-else statement. There may be clever equivalent code alternatives but not sure.

[GeoRanger]

Based on your last post, are you not so much concerned with a concise executable but rather with concise source code?

If so, is this closer to what you are after?

int set_PORTB_bit(int position, int value)
{
  // Sets or clears the bit in position 'position' 
  // either high or low (1 or 0) to match 'value'.
  // Leaves all other bits in PORTB unchanged.

  PORTB = value ? (PORTB | (1 << position)) : (PORTB & ~(1 << position));
                
  return 1;
}

You may be interested in the related topic "Kolmogorov complexity". See <http://en.wikipedia.org/wiki/Kolmogorov_complexity>.

[OITGUY]

Thanks GeoRanger, the conditional operator is a good way to compress it to a one-liner.

As for the optimization, the generated .hex files are the same using Windows AVR Studio 4 with "-Os" optimization, and also using the free GCC compiler. Seeing as how this is for implementing in a microcontroller, further optimization is definitely a good thing. I believe programmable flash memory sizes start at 1 kB for TinyAVR.

[OITGUY]

Also to clarify I am interested in both condensing source code and optimization.