Switch Bits, but does not switch them back?

Question

I was messing around with a bit of code and decided to attempt to switch some bits around in files, and then switch them back to get the original file. Somehow, it changes the bits, but will not change them back.

Here is what I have:

The magic happens by me opening a file and fread into rByte[0] and rByte[1].

unsigned char rByte[] = {0, 0};

int isBitSet(unsigned char byte, int bytePosition){

    unsigned char mask[] = {128, 64, 32, 16, 8, 4, 2, 1};
    return ( (byte & mask[bytePosition]) != 0 );
}

unsigned char setBit(unsigned char byte, int pos) {
    byte |= (0x01 << pos);
    return byte;
}

unsigned char clearBit(unsigned char byte, int pos){
    byte &= ~(0x01 << pos);
    return byte;
}

/* DO NOT TOUCH */

void switchBits (unsigned char byte1, unsigned char byte2, int x, int y) {

    int reg1 = 0;
    int reg2 = 0;

    reg1 = isBitSet(byte1, x);
    reg2 = isBitSet(byte2, y);

    if ( (reg1 == reg2) ) {

    }

    if ( (reg1 == 0) && (reg2 != 0) ){
        rByte[0] = setBit(byte1, x);
        rByte[1] = clearBit(byte2, y);
    }

    if( (reg2 == 0) && (reg1 != 0) ){
        rByte[0] = clearBit(byte1, x);
        rByte[1] = setBit(byte2, y);
    }

}

Now, I assume that if the same switchBits() will be applied, the way I perceive it, the program should go back to regular. I made a music file make weird noises, but did not go back to normal.

Answer 1

Assuming that rByte is globally defined (which is not nice).

You have the position of your bits reversed in isBitSet() compared to setBit() and clearBit().

Example assuming pos = 0

isBitSet checks the MSB (most significant bit) as mask[0] == 128 but in setBit and clearBit functions you shift 0x01 << 0 meaning it corresponds to a 1.

edit: What I suggest is that you change

unsigned char mask[] = {128, 64, 32, 16, 8, 4, 2, 1};

to

unsigned char mask[] = {1, 2, 4, 8, 16, 32, 64, 128};

edit end

You should overthink your design of the function, just chaining ifs seems quite strange and the logic you want to implement could be improved. Here an example with simplified logic, you might do it another way to be more specific on the logic, so just see this as a suggestion.

if ( (reg1 == reg2) ) {

}
else{
  if ( (reg1 == 0)){
    rByte[0] = setBit(byte1, x);
    rByte[1] = clearBit(byte2, y);
  }

  else{
    rByte[0] = clearBit(byte1, x);
    rByte[1] = setBit(byte2, y);
  }
}

Another thing which would simplify the program again would be a switch function that just makes a 0 out of a 1 and vice versa. This is easily done by the XORoperator ^. Meaning if both are different you just have to switch the state of both bits so it would reduce your function to this.

if ( (reg1 != reg2) ) {
  rByte[0] = switchBit(byte1,x);
  rByte[1] = switchBit(byte2,y);
}