Grouping AVR pins for multi-bit output

[Lomax]

Thanks, that's a lot of interesting information - some of which I think I understand The only pins that are changed by an interrupt are on PORTD (for serial communication); the other ports are only modified by my setLevel() function. Button inputs are read in the main loop and trigger setLevel().

[Lomax]

Direct assignment:

Code: [Select]

void setLevel(uint8_t level) {
    if(level & 1) PORTC |= 1<<3; else PORTC &= ~(1<<3);
    if(level & 2) PORTC |= 1; else PORTC &= ~(1);
}
int main(void) {
    setLevel(3);
}

Result:

setLevel:
.L__stack_usage = 0
        sbrs r24,0
        rjmp .L2
        sbi 0x8,3
        rjmp .L3
.L2:
        cbi 0x8,3
.L3:
        sbrs r24,1
        rjmp .L4
        sbi 0x8,0
        ret
.L4:
        cbi 0x8,0
        ret

Using temporary variable:

Code: [Select]

void setLevel(uint8_t level) {
    uint8_t tmpC = PORTC;
    if(level & 1) tmpC |= 1<<3; else tmpC &= ~(1<<3);
    if(level & 2) tmpC |= 1; else tmpC &= ~(1);
    PORTC = tmpC;
}
int main(void) {
    setLevel(3);
}

Result:


setLevel:
.L__stack_usage = 0
        in r25,0x8
        sbrs r24,0
        rjmp .L2
        ori r25,lo8(8)
        rjmp .L3
.L2:
        andi r25,lo8(-9)
.L3:
        sbrs r24,1
        rjmp .L4
        ori r25,lo8(1)
        rjmp .L5
.L4:
        andi r25,lo8(-2)
.L5:
        out 0x8,r25
        ret


Conclusion: if two "cbi"/"sbi" instructions are preferred over a single "out" then I should not be using a temporary variable.

[Siwastaja]

Note the codes are functionally different: one changes all bits in the output register at once, only subject to tiny picosecond scale timing error between bits. With the one doing multiple sbi/cbi, there is delay in microsecond range between different bits, and an intermediate "glitch" state which is neither the initial nor the final state. Whether this matters depends on what the thing actually does with the signals.

AVR is quite special for having two special instructions which can set or clear any single bit, but only on some of the IO ports, not all on larger devices! Many other microcontrollers have more capable set/clear registers which can set/clear more than just one bit at once, even combine sets and resets in same atomic operation, and they work for all ports - e.g. see BSRR on STM32.

[Lomax]

Now I am confused again. I thought cbi/sbi were preferred over "out" because they are "atomic", while "out" needs eight ticks to update the pins. Or in other words, changing a single pin with cbi/sbi would not need any interrupt protection, whereas doing so with "out" would, since it sets all the pins on a port over multiple steps. Though in my case I would still need to protect against interrupts, since I'm modifying two pins. From your description it sounds like "out" is to be preferred? @cv007 appears to suggest the opposite:

one of the reasons macros are used when dealing with pins is to make sure you get the sbi/cbi instructions produced (meeting compiler requirements to produce the wanted sbi/cbi), which eliminates the need to interrupt protect

[Lomax]

I think I'm going to have to accept that this discussion goes somewhat over my head, and that I will not be able to grasp the details of avr-gcc optimisations (or lack thereof), or the specifics of different AVR assembly instructions - at least before I have more experience. I'm happy that I have been able to produce code that works reliably, and a completed circuit that achieves what I set out to do, even if that means the code is ugly to read (to my eyes at least) and I have to take crude steps to protect it from ISR interference. In other languages where I have more experience I always strive for elegance and readability (code as documentation), and reducing repetition, but I understand that in the microcontroller world it's more important to optimise for compiler output, and I think this is the main thing I have learned from this discussion. Thanks to everyone who's contributed!

[magic]

SBI/CLI modifies one bit with one instruction which cannot be interrupted.
OUT (or STS) sets all bits of one port with one instruction. This is the only way to change multiple pins at the same time. It cannot be interrupted either, but...

Disabling interrupts is necessary in read-modify-write scenarios, to prevent situations like:

Code: [Select]

main: int x = PORTC;
ISR:  int y = PORTC;
ISR:  PORTC = y | PIN1;
main: PORTC = x | PIN2; // at this point PIN1 gets turned off

[Lomax]

Ok, one more suck of the sav

Code: [Select]

static uint8_t const LED0 = 0b00000011; //PORTC

#define OUT(port, led, level) (port ^= (port ^ level) & (led))

void setLevel(uint8_t level) {
    OUT(PORTC, LED0, level);
}

int main(void) {
    setLevel(3);
}


setLevel:
.L__stack_usage = 0
        in r25,0x8
        in r18,0x8
        eor r24,r25
        andi r24,lo8(3)
        eor r24,r18
        out 0x8,r24
        ret


 

Edit: With thanks to @Ian.M who suggested using XOR.

[Ian.M]

Yes, that's not nice as the AVR architecture cant do logic operations (other than single bit test/set/clear) direct to port registers, so is inherently incapable of a single instruction atomic update.   If the port is also modified by an ISR, you'd need to wrap the macro invocation with disable and enable interrupt instructions to prevent disaster, at which point you might as well simply clear the affected bits with an inverted AND mask and OR in the new  data.

[Lomax]

I thought the conclusion was that there is no safe way to output a 2-bit value on a port that contains pins which are under the control of an ISR without preventing interrupts from happening during the port write(s)? In what way is

Code: [Select]

        in r25,0x8
        sbrs r24,0
        rjmp .L2
        ori r25,lo8(8)
        rjmp .L3
        andi r25,lo8(-9)
        sbrs r24,1
        rjmp .L4
        ori r25,lo8(1)
        rjmp .L5
        andi r25,lo8(-2)
        out 0x8,r25
        ret

better than

Code: [Select]

        in r25,0x8
        in r18,0x8
        eor r24,r25
        andi r24,lo8(3)
        eor r24,r18
        out 0x8,r24
        ret

 

[Ian.M]

That's what you get if you do the bit-shuffling within the conditionals that set or clear the bits individually.    That's a poor approach if there is any possibility the bits in question can be contiguous, or can be pre-calculated, preferably at compile time, or less desirably at runtime, outside the loop.

If you can *guarantee* level does not contain '1' bits outside of the led mask, try:

Code: [Select]

#define OUT(port, led, level) (port = (port & ~led) | level))which should save one instruction if led is a constant expression evaluable at compile time, but if you also need to mask level or led is variable, one might as well stick with the XOR version.

[Lomax]

I want to output a 2-bit value on two pins, physically adjacent and consecutive on the PORT. This is all set in stone, and there is neither need nor capability to change any of it post flashing; the pins will always be the same, the value will always be 2-bits long.

[Ian.M]

Try:

Code: [Select]

#define OUTC(ledmask, levels) (PINC=(PORTC ^ (levels)) & (ledmask))which exploits the ability of the port's PINx register to toggle any pins you write a '1' bit to, (which is equivalent to XORing PORTx with the same data), which should give you a single instruction atomic update, that doesn't require you to mess with interrupt enables, unless the pins in question (specified by ledmask) are also modified in an ISR. 

It gets more complex if you don't want to hard-code the port, and don't want to give both the PORTx and PINx registers as parameters, as either the macro must do token pasting to get the port and pin register names from a single port letter, or one has to do some *nasty* pointer creation, offsetting and dereferencing stuff to get one from the other and *hope* the compiler realises the whole pointer offset mess can be precalculated at compile time.

[cv007]

which exploits the ability of the port's PINx register to toggle any pins you write a '1' bit to

One of the problems using this xor/toggle method on this avr is you will have to come up with a different method when it comes time to deal with the DDR register, and as already mentioned it gets cumbersome as soon as you have different ports involved and/or the test bits require some work to get into their respective bit positions.

Another thing you (Lomax) seem to hint at, is affecting the state of peripheral controlled pins (assuming the in/out instructions used in a rd/mod/wr sequence)- I'm not sure there is any mcu in which a peripheral has 'taken over' a pin for its own use where you can then 'override' its output on your own. In other words, if another pin on the same port is doing hardware pwm, your rd/mod/wr sequence will not affect it even though that pwm pin changed inside that sequence. You are preserving that pins 'non-peripheral' output state in any case (for example if you specifically wanted/set the pwm pin high when the pwm peripheral is not enabled, that state still gets preserved).

I treat all pins individually. If they belong to some group, I still treat them as individual pins. If I have a char lcd using 4 data pins, it will mean I can use any pins (due to availability, or for easy board routing) and easily change later as needed without any code changes except to specify which pin is wanted. It makes no difference they are not all set in the same clock cycle, and will be plenty fast for about anything that does not use a full port (at which point you will switch to dealing with a full port, or have a hardware peripheral that does the job).

For your avr, where the port/pin is known at compile time, the simple thing to do is treat these pins individually to get the sbi/cbi instructions produced. Done. I knew that when I posted the original example, but also knew this thread would go through various phases before most likely getting back to the original solution (pin manipulation is a popular subject, as no mcu user is excluded from its use). Sometimes you just end up back where you started when seeking a 'better' solution.

Here is a C++ way to deal with pins on your avr (in a limited way, just as an example)-
https://godbolt.org/z/Th8eqe7h7
and that type of thing is mostly good as you contain/hide the details where they belong (not in your app code), plus have a single interface to your port peripheral that can be used in any project. BUT, when you have 8 lines of code that already does what you want, in a language you are already familiar with, in an app that does not see a lot of pin setup/manipulation, then those 8 lines of code are not so bad after all.

This avr is not that pleasant to work with compared with newer mcu's. The pin thing is one of them, dealing with constants in flash is another. Move up to an avr0/1 (or any 32bit mcu) and you get much nicer use of pins due to the set/clr/tgl registers, and the flash is mapped to data space so const use is as easy as it gets (const's get stored in flash and the storage/usage of them requires no extra effort on your part). A fractional baud rate generator is also nice in these newer avr's, along with an rtc, and... so on. Move up when you can.

[Siwastaja]

Now I am confused again. I thought cbi/sbi were preferred over "out" because they are "atomic", while "out" needs eight ticks to update the pins. Or in other words, changing a single pin with cbi/sbi would not need any interrupt protection, whereas doing so with "out" would, since it sets all the pins on a port over multiple steps. Though in my case I would still need to protect against interrupts, since I'm modifying two pins. From your description it sounds like "out" is to be preferred? @cv007 appears to suggest the opposite:

The problems is you are not understanding your own application. No one can tell you if the "right way" to update your outputs is in multiple cbi/sbi steps or a single OUT. You need to tell us what your circuitry does if the bits update in two steps, with intermediate state which was something you did not ask for. For example, if there is another trigger signal during which the bits are sampled by the rest of the circuitry, then the glitch would not matter and smaller code size with two sbi/cbi could be preferred. If the signal is e.g. audio, bandwidth of which cannot be filtered to reject the glitch, then you would want the bits to change simultaneously, even if that results in larger code.

There is no such a thing as "interrupt protection". You need to understand what your code does. For example, if your interrupt asynchronously modifies some control variable which you use to control the two IO pins, then all you need to do is to disable interrupts just for making a local copy of that control variable:

Code: [Select]

void ISR()
{
    io_control++;
}

void loop()
{
    cbi();
    uint8_t tmp_io_control = io_control;
    sbi();
    if(tmp_io_control == 123)
       do_thing_1();
    if(tmp_io_control == 124)
       do_thing_2();
}

now, in this example, decisions to do thing_1 and thing_2 are based on the variable sampled at one point in time, even if ISR changes the value between the two comparisons.

[Ian.M]

Remember, our O.P. is driving LEDs.  Unless they are doing anything exotic with optical data links or high speed cameras or the like, I bet it can tolerate milliseconds of skew between outputting the individual bits and no human will ever notice without hooking up a scope!

[Siwastaja]

Yeah, if it's for indication for human eye, then obviously the solution yielding smallest code size is a good idea. Also if the led state is updated often, even a glitchy state might not be a problem. For example:

Let ISR set state=0 or state=1;

Let LEDs indicate the states as: state 0 {0,1}, state 1{1, 0}

Let the while(1) loop in main() update the LEDs using separate sbi/cbi:

Code: [Select]

volatile int state;
void ISR()
{
   state = 0;
   // or
   state = 1;
}

void main()
{
   while(1)
   {
      set_led_a(state);
      set_led_b(!state);
      delay_ms(100);
   }
}

Now, if state is modified between set_led_a, set_led_b, LEDs would be either {0,0} or {1,1}, either is an invalid indication, for 100 milliseconds. Make that 1 milliseconds instead, and human eye wouldn't notice. Or, you could do a local copy of state and use that for indication to remove the glitch. If you do this, and if state is an atomic type - uint8_t would be, int would not - then there is no need to disable interrupts. If you used int, which is not atomic on AVR, then you would need to disable interrupts during the copy to local variable.

[Lomax]

You need to tell us what your circuitry does

I have built a 4-channel PWM generator for controlling fans, though it would be equally suitable for dimming lights etc. Each channel can be controlled by means of a push button, and each button is accompanied by four LEDs which show that channel's current level (0-3). Pushing a button simply increments that channel's level (on release), wrapping at 3 - though I am toying with using long-press and double-click for additional functions. The AVR also provides a Modbus interface for controlling the channels, setting the 16 individual levels (0-255), configuring the PWM generation (phase and scaler), and configuring the RTU (slave ID, comm paramaters). The Modbus registers are held in EEPROM, and they are saved by setting the last register to 1 (to reduce EEPROM writes). In order to simplify the cabling and reduce pin usage each channel's level indication is output as a 2-bit value. The control panels use a 74HC138 3to8 decoder to drive the four LEDs from these 2-bit values. This leaves just a single GPIO unused (D4). I ran into an issue with the data direction pin on PORTD (D2) being reset by my setLevel() function, which broke Modbus responses for those function codes that trigger setLevel() - this has long since been fixed and my application now works perfectly. I am pleased with it.

[cv007]

now, in this example,

You are showing cbi/sbi instead of cli/sei, which makes it look like the sbi/cbi instructions which were being talked about are somehow related/involved. Also, that variable needs no interrupt protection anyway, as a read of a uint8_t on an avr (and any other mcu) is already atomic/cannot be interrupted (in this case, just the copying of the var to a local var is doing the job of getting a single unchanging value to work with in the following code). If the var was larger than a uint8_t, then on the avr one would need to interrupt protect the read as it now requires two or more instructions to do so.

Bottom line- there are various ways to deal with pins on an mcu. Since pins are typically plenty, and have many unrelated uses, and pins typically share registers as a group (port), they are prime candidates to get corrupted when manipulating in more than one 'run level' (via interrupts). So, you take care of the potential problem preemptively whether its currently an actual problem or not (can easily change as code development progresses). Most of the battle is knowing something like this even exists, and once aware you will start to see other areas that have the same problem.