Is there a way to make my ones complement code far more efficient and to allow for multiple outputs at a time?

Question

I am using dip switches as inputs and LEDs as outputs to resemble the ones complement of any given input. I am programming an Arduino Uno to attempt to do that. I am also not very experienced with bitwise efficiency; is there a way to greatly reduce lines in my code?

What I currently have is a bunch of if-statements.

#include <avr/io.h>//library used to access the pin addresses

int main () {
        DDRB |= 0b00001111;
        DDRD &= ~(0b11110000);
        while (1) {
        if (PIND & 0b00010000) {
            PORTB |= 0b00001110;
            PORTB &= ~(0b00000001);
        }
        else if (PIND & 0b00100000) {
            PORTB |= 0b00001101;
            PORTB &= ~(0b00000010);
        }
        else if (PIND & 0b00110000) {
            PORTB |= 0b00001100;
            PORTB &= ~(0b00000011);
        }
        else if (PIND & 0b01000000) {
            PORTB |= 0b00001011;
            PORTB &= ~(0b00000100);
        }
        else if (PIND & 0b01010000) {
            PORTB |= 0b00001010;
            PORTB &= ~(0b00000101);
        }
        else if (PIND & 0b01100000) {
            PORTB |= 0b00001001;
            PORTB &= ~(0b00000110);
        }
        else if (PIND & 0b01110000) {
            PORTB |= 0b00001000;
            PORTB &= ~(00000111);
        }
        else if (PIND & 0b10000000) {
            PORTB |= 0b00000111;
            PORTB &= ~(0b00001000);
        }
        else if (PIND & 0b10010000) {
            PORTB |= 0b00000110;
            PORTB &= ~(0b00001001);
        }
        else if (PIND & 0b10100000) {
            PORTB |= 0b00000101;
            PORTB &= ~(0b00001010);
        }
        else if (PIND & 0b10110000) {
            PORTB |= 0b00000100;
            PORTB &= ~(0b00001011);
        }
        else if (PIND & 0b11000000) {
            PORTB |= 0b00000011;
            PORTB &= ~(0b00001100);
        }
        else if (PIND & 0b11010000) {
            PORTB |= 0b00000010;
            PORTB &= ~(0b00001101);
        }
        else if (PIND & 11100000) {
            PORTB |= 0b00000001;
            PORTB &= ~(0b00001110);
        }
        else if (PIND & 11110000) {
            PORTB |= 0b00000000;
            PORTB &= ~(0b00001111);
        }
    }
    return 0;
}

Also, another problem I'm having is that only one LED turns off at a time; if I flip one switch and then the other, the LED for the first switch I flipped turns back on as soon as I flip the other one.

These tests look wrong. Rather than `else if (PIND & 0b10110000) {`, I'd expect `else if ((PIND & 0b11110000) == 0x0b10110000) {`. — chux - Reinstate Monica, Jan 06 '19 at 09:06
If you use this approach (which you should not), You should read PIND _once_ into a variable before tf-else if-else chain and test that rather then repeatedly re-reading PIND which may change at any time. — Clifford, Jan 06 '19 at 12:25

Some programmer dude · Accepted Answer · 2019-01-06T09:24:56.047

0

First of all I recommend that you simply clear PORTB first. Then you can easily set just the bits you need to be set.

As for the big if...else if chain, you could shift down the value of PIND four bits, and complement it.

Something like:

// Clear all bits (and turn off all LEDs)
PORTB = 0;

// Move the four high bits of PIND to the four lowest,
// gets the bitwise complement of that, and set those bits
// (Turning on some LEDs)
PORTB |= ~(PIND >> 4);

Of course, you you can still use your current way to turn on/off the LEDs (but still without the long if ... else if chain):

PORTB |= ~(PIND >> 4);
PORTB &= ~(PIND >> 4);

edited Jan 06 '19 at 09:24

answered Jan 06 '19 at 09:03

Some programmer dude

400,186
35
402
621

1

This approach has a weakness in that it makes for non-continuousness of having a given LED enabled. Although not very visually discernible, makes for "noisy" LEDs – chux - Reinstate Monica Jan 06 '19 at 09:22
@chux Turning LEDs on and off is a common technique for controlling LED brightness. And if you look at many LED signs, you will see that they have a limited set of rows on at any one time because they have to select the row to update. I don't think turning the LED off for a few CPU cycles is going to cause much of a problem. – janm Jan 06 '19 at 14:53
As @Clifford mentioned in another answer, for the "current way" case, you should read `PIND` once into a local variable and then use that. – janm Jan 06 '19 at 14:55
PORTB is obviously a GPIO register so no, you should not clear it first. That will cause needless flicker that might not be noted by the human eye, still bad programming and might cause needless current consumption if the LEDs are active low. You should use a temp variable in RAM and store the data there, set it to zero first, then when everything is done, write the new value to the port. – Lundin Jan 07 '19 at 10:39
@Lundin I agree you can get the CPU to do it with PWM but it does also get done in software with GPIO. I do agree that it is better practice to read `PORTB`, update the desired bits and then write the new value. – janm Jan 07 '19 at 11:26
@janm GPIO in microcontroller systems tends to refer to general-purpose I/O pins that are plain digital in/out. PWM will have a dedicated timer hardware peripheral separate from GPIO. You _can_ bit-bang PWM through GPIO but it is painful and pointless, since every MCU has some manner of PWM hardware. At any rate, PWM is not what we see in this answer, but just some confused PC programming solution... – Lundin Jan 07 '19 at 11:33

score 0 · Answer 2 · edited Jun 20 '20 at 09:12

This is not correct code and it could be done in easier and more readable ways.

You should get rid of the binary notation as it is hard to read and not actually C standard.
You should only read hardware registers once and write to them once. Accessing the same register in a long if else if chain as done here is a big no-no. The state of the port might change between reads.
When dealing with any form of switches, be it physical buttons, dip-switches, relays etc, you must absolutely implement some manner of signal de-bouncing. A button, when pressed, has an electromechanical bounce, which manifests itself as a spike on the line until it goes stable. You can view this with an oscilloscope by connecting one side of a button to supply through a resistor, and the other side to ground.

Failing to de-bounce such a signal is a very common embedded beginner mistake. It is most often done in software, by reading the button several times before making a decision. Alternatively through a RC filter in hardware, but that costs extra components.

You should have code like this:

#define LED0 (1u << 0) // corresponding to PORTx:0
#define LED1 (1u << 1) // corresponding to PORTx:1
...

Then you can declare an output matrix such as this:

const uint8_t LED_OUTPUT [16] = 
{
  [0] = LED1 | LED2 | LED3,
  [1] = LED0 | LED2 | LED3,
  ...
};

Input is read from PIND once, debounced through a periodic read, possibly with a simple digital filter (like median of 3 reads), then shifted in place (right shift 4 steps) so it corresponds to a number 0 to 15:

uint8_t index = get_buttons(); // function that returns the debounced valued read from PIND
PORTB = LED_OUTPUT[index];

And that's it.

Is there a way to make my ones complement code far more efficient and to allow for multiple outputs at a time?

2 Answers2