Grouping AVR pins for multi-bit output

[Lomax]

I'm curious to know if there's any neat way to group pins on an 8-bit AVR so that I can assign them a multi-bit value without much messing around. Of course they're already grouped as PORTs, but I would like to have a macro that refers only to two of them, so I can do stuff like

Code: [Select]

MYPINS = 2;


With it resulting in MYPINS[0] being low and MYPINS[1] high. I've spent some time searching and have drawn a blank, so guess the answer is "no", but thought I'd ask...

[TerminalJack505]

If you're using C++ then that would be something you could do by using a class to represent the port and pins and overloading the assignment operator.

[Lomax]

Yeah, I could see how one might do this in any OO language I've used, but this is for C. I've got four 2-bit values that are updated and output on AVR pins and since the other pins on the ports are used for other things, including inputs, I have only been able to come up with a rather clumsy way of setting them, e.g.:

Code: [Select]

PORTC = data[1]<<3 | BTN0 | BTN1;
data[1] in this case contains the 2-bit value I want to see on pins C3 & C4. The common way of setting pins is to OR the current port value with the pins you want to set, and AND it with the pins you want to unset, but you can't do that with a 2-bit value, so I have to set the port from scratch every time. I don't like it. BTN0 and BTN1 set pull-up on two input pins. I would like to leave the port otherwise untouched and only set those two pins to match data[1]. Maybe something like this could work?

Code: [Select]

PORTC |= data[1]<<3 & 0b00011000;
If so I could define a constant for the bitmask and use it instead:

Code: [Select]

static uint8_t const OUT1 = 0b00011000;
...
PORTC |= data[1]<<3 & OUT1;

But that won't work either, because OR. My head hurts

[cv007]

No overthinking-
https://godbolt.org/z/4z1exvq5x

You just have to make sure the instructions emitted are only sbi/cbi. If you start using loops and other things other than this simple inline code then you can end up with the compiler resorting to pointer use which then loses atomicity in changing port bits.

[Lomax]

No overthinking

I never miss a chance. 

I was just thinking that my question really boils down to "how can I elegantly set a pin to a stored bit without knowing if that bit is 1 or 0?" If I understand your answer correctly you can't - you have to check what the value is then AND/OR depending on what it is. If so I will stop searching for a better answer and learn to live with the clumsy.

[PDP-1]

Does this do what you want it to do? It should only modify bits where the mask is set to 1.

Code: [Select]

void setPins(uint8_t newVal)
{
        uint8_t mask = 0x03;        // 1 = change the bit, 0 = don't change it
        newVal &= mask;              //  make sure we don't touch anything else
newVal |= ~0x03;                      
PORTC = (PORTC & newVal) | newVal;
}

edit: I tried the website cv007 linked to, this code won't be atomic apparently. That site is a nifty tool, hadn't seen it before!

[Lomax]

No overthinking-
https://godbolt.org/z/4z1exvq5x

You just have to make sure the instructions emitted are only sbi/cbi. If you start using loops and other things other than this simple inline code then you can end up with the compiler resorting to pointer use which then loses atomicity in changing port bits.

Code: [Select]

void setLevel(uint8_t channel, uint8_t level) {
registers[channel] = level;
switch(channel) {
case 0:
OCR1A = registers[4 + level];
if(registers[0] & 1) PORTC |= 1; else PORTC &= ~(1);
if(registers[0] & 2) PORTC |= 1<<1; else PORTC &= ~(1<<1);
break;
case 1:
OCR1B = registers[8 + level];
if(registers[1] & 1) PORTC |= 1<<3; else PORTC &= ~(1<<3);
if(registers[1] & 2) PORTC |= 1<<4; else PORTC &= ~(1<<4);
break;
case 2:
OCR2A = registers[12 + level];
if(registers[2] & 1) PORTD |= 1<<6; else PORTD &= ~(1<<6);
if(registers[2] & 2) PORTD |= 1<<7; else PORTD &= ~(1<<7);
break;
case 3:
OCR2B = registers[16 + level];
if(registers[3] & 1) PORTB |= 1<<4; else PORTB &= ~(1<<4);
if(registers[3] & 2) PORTB |= 1<<5; else PORTB &= ~(1<<5);
break;
}
}

This works, but by god is it messy.

[Lomax]

Does this do what you want it to do? It should only modify bits where the mask is set to 1.

Code: [Select]

void setPins(uint8_t newVal)
{
        uint8_t mask = 0x03;        // 1 = change the bit, 0 = don't change it
        newVal &= mask;              //  make sure we don't touch anything else
newVal |= ~0x03;                      
PORTC = (PORTC & newVal) | newVal;
}


The mask would have to be passed in as well, but maybe.

[Sacodepatatas]

I'm curious to know if there's any neat way to group pins on an 8-bit AVR so that I can assign them a multi-bit value without much messing around. Of course they're already grouped as PORTs, but I would like to have a macro that refers only to two of them, so I can do stuff like

Code: [Select]

MYPINS = 2;


With it resulting in MYPINS[0] being low and MYPINS[1] high. I've spent some time searching and have drawn a blank, so guess the answer is "no", but thought I'd ask...

Something similar to this might work if you don't mind losing atomicity:

Code: [Select]


struct MyPort {
          unsigned int MYPINS:2;
          unsigned int OTHERS:6;
          } *MyPortC=(struct MyPort *)&PORTC;

MyPortC->MYPINS=2;



[Lomax]

Something similar to this might work if you don't mind losing atomicity:

Thanks! I'm not 100% sure but I think this might lead to problems since I have an ISR that also writes to one of the ports. In fact, saying this, I realise that my implementation of @cv007's solution also has this weakness. And in fact, saying that, I now wonder if things like PORTC |= XXX are atomic in themselves! C is hard when you're coming from languages like Python and Ruby 

[Ian.M]

Microchip PICs have similar issues when you need to change groups of pins on a port simultaneously, but cant simply take over the whole port, and cant tolerate a glitch to an all set or all cleared state during the pin group update.  An effective solution is XOR masking, i.e. build a mask of pins to be flipped to get from the old value to the new one, then flip them all in one instruction (provided the architecture supports single instruction read-modify-write operations on ports).  Fortunately most embedded C compilers optimise well enough that this can be done in C rather than having to resort to assembly.

See http://www.microchip.com/forums/FindPost/715375

[cv007]

I now wonder if things like PORTC |= XXX are atomic in themselves!

It is not, but the compiler will optimize, for the 'original' avr, to a sbi/cbi instruction which is atomic for setting/clearing bits (in the address range they can handle). That is why you have to somewhat keep an eye on port bit manipulation as you can easily lose atomicity if you introduce something that compiler cannot know at compile time. Since the instruction is hard coded with the address and bit information, it requires both a known address and a known single bit (position).

For newer avr, and any other mcu with set/clr/tgl registers, you can do pin manipulation atomically with runtime values because the atomicity is from the use of these specific registers, not from the use of a special instruction (so can also do multiple pins at once)-
s[1].port->OUTSET = 1<<(s[1].pin); //atomic setting of port.pin (no reading takes place)
and it matters not what the compiler optimizer does- there is always a single store instruction (st or sts) to the OUTSET register.

Code: [Select]

An effective solution is XOR maskingIts probably ok up until your bits of interest are not consecutive. If the bits to test need manipulation other than a simple shift to get them in the correct position, or the pins are a mix of different ports, then it can start to look worse depending on the instruction set.

Simply testing bits and setting/clearing individual pins doesn't look all that great at first, but usually ends up being the easiest to write, and pins are easily changed later without any restrictions and no need to figure out how to manipulate values to match a new pin position.

[Ian.M]

The bits being manipulated don't need to be consecutive.
The result of the macro expansion is the C statement:

Code: [Select]

reg^=(reg^x)&mask(simplified by removing extraneous parentheses)
where reg is the register being manipulated (i.e. PORTB), x are the correctly aligned bits to be inserted in the register, and mask is a '1's mask of which bits to insert.
Determining which bits to flip occurs in parallel, using the architecture's native bitwise logic operations, so its computational cost is constant no matter the pattern of the mask, provided all parameters fit within the native word size.  The variable cost is within the mungling function that must be used to prepare x by shuffling its bits to match the port pin allocations, and on most architectures for anything more than two bits, its strongly preferable to allocate consecutive port bits in order so a simple shift can be used to position the bits in x correctly.

However multi-port operations are more problematic and architecture dependent.  All port registers of the same type need to be at consecutive addresses with no address alignment constraints that prevent mapping them to a suitably sized C integer variable.  For many MCUs, this alone is a show-stopper.

Then, you must resign yourself to the operation being non-atomic on any architecture with a word size smaller than the variable mapping the ports, and possible even on architectures with a larger word size, due to alignment issues. 

[Siwastaja]

I'm curious to know if there's any neat way to group pins

There is, it's called a function. In C++ you would be able to do weird and confusing stuff like overloading the = operator.

Function has many advantages: the name documents what it's about, parameters have obvious types that are compile-time type checked, function implementation can change while interface keeps the same, it is possible to add parameter sanity checks, and so on.

[magic]

1. disable interrupts if there is any chance that ISRs may manipulate PORTC at the same time.
2. read the value of PORTC
3. mask out the bits you want to disable
4. OR the bits you want to enable
5. write back to PORTC register - all pins change at the same time, no glitches
6. enable interrupts, if applicable
7. write a function which does all of the above so you can simply call it when necessary

[cv007]

The bits being manipulated don't need to be consecutive.

No, but moving the value bits into the matching pin position is not that pleasant, and the code also becomes uglier at least in this case. Something like an stm32 without a pin toggle register means a different macro is then needed to setup the bssr register to do this job (to get to a set+reset register value).

https://godbolt.org/z/joMxaxnhx


The simple bit test, pin set/clr combo will work the same in all cases. Just need an atomic way to set/clr an individual pin, which any mcu will have. I'm not sure if there are many instances, which do not already use a whole port (by the user, or as a peripheral), which need a set of pins set atomically in relation to each other. Getting a group of pins set atomically individually is fine in most cases, even though will not be atomically set as a group.

[Lomax]

1. disable interrupts if there is any chance that ISRs may manipulate PORTC at the same time.
...
6. enable interrupts, if applicable

Now there's a good idea, is that as simple as doing cli(); at the top of the function and sei(); at the end?

2. read the value of PORTC
3. mask out the bits you want to disable
4. OR the bits you want to enable
5. write back to PORTC register - all pins change at the same time, no glitches

That's how I'm now doing it, courtesy of @cv007.

7. write a function which does all of the above so you can simply call it when necessary

There is, it's called a function.

It's already a separate function which does only one thing (sets the channel levels). I was hoping I could abstract the 2-bit value pins into runtime values and pass them to another smaller function that just sets those pins on a port. That would be more elegant IMO. But I now understand that using runtime values this way is not recommended, because using runtime values in port assignments is not atomic. Though I guess if I enable/disable interrupts around the call I could still get away with it?

Microchip PICs have similar issues when you need to change groups of pins on a port simultaneously, but cant simply take over the whole port, and cant tolerate a glitch to an all set or all cleared state during the pin group update.  An effective solution is XOR masking, i.e. build a mask of pins to be flipped to get from the old value to the new one, then flip them all in one instruction (provided the architecture supports single instruction read-modify-write operations on ports).  Fortunately most embedded C compilers optimise well enough that this can be done in C rather than having to resort to assembly.

See http://www.microchip.com/forums/FindPost/715375

Interesting. I might try to implement this solution.

[Lomax]

moving the value bits into the matching pin position is not that pleasant, and the code also becomes uglier at least in this case

The wiring too becomes ugly, so my four 2-bit outputs are all in pairs of adjacent pins.

Getting a group of pins set atomically individually is fine in most cases, even though will not be atomically set as a group.

I've been trying to wrap my head around what would happen if the port assignment is interrupted with a new value in between setting the two bits. My guess is it won't matter.

[TerminalJack505]

Now there's a good idea, is that as simple as doing cli(); at the top of the function and sei(); at the end?

Since you are using C, check your library for an implementation of the ATOMIC_BLOCK macro.  Most AVR compilers will have one.  This is the cleanest way to do that.

[Siwastaja]

But I now understand that using runtime values this way is not recommended, because using runtime values in port assignments is not atomic.

Atomicity has nothing to do with the configuration being run-time or fixed. If by atomicity you mean immunity against interrupts, then you just disable interrupts during the modification of state. If you mean that you expect the outputs to change state on the same clock cycle, then you need to have the pins on the same PORT register, no way around it; still being run-time or fixed doesn't matter.

Remember that in C, the accesses to PORT/PIN registers happen as many times they are referred to in your code, because they are qualified volatile. Whenever you need some bitshifting, combining stuff etc., it's a good idea to use a temporary variable:

Yes:

Code: [Select]

uint8_t tmp = DDRC;
tmp <<= 1;
tmp |= 0b10111001;
if(something)
   tmp &= 0b1;
DDRC = tmp;

no:

Code: [Select]

DDRC <<= 1;
DDRC |= 0b10111001;
if(something)
   DDRC &= 0b1;


And, similarly, if your goal is to make two PORT writes as close as possible, with no interrupt intervening, then:

Code: [Select]

uint8_t tmp_a = ...;
uint8_t tmp_b = ...;

cli();
PORTA = tmp_a;
PORTB = tmp_b;
sei();


Still not on the same clock cycle, but pretty close, and should be predictable delay. It's usually a good idea to make this interrupt-disabled critical section as short as possible.

[berke]

How about using a structure with bitfields?  https://godbolt.org/z/f8Pf54E4E

[Lomax]

Atomicity has nothing to do with the configuration being run-time or fixed. If by atomicity you mean immunity against interrupts, then you just disable interrupts during the modification of state. If you mean that you expect the outputs to change state on the same clock cycle, then you need to have the pins on the same PORT register, no way around it; still being run-time or fixed doesn't matter.

Remember that in C, the accesses to PORT/PIN registers happen as many times they are referred to in your code, because they are qualified volatile. Whenever you need some bitshifting, combining stuff etc., it's a good idea to use a temporary variable

Thank you, that's a good tip - and a method I would instinctively use in other languages. Not sure why I haven't thought of doing so in C. This could also provide a route to further abstracting the port value assignments into a generic function, which would be neat.

How about using a structure with bitfields?  https://godbolt.org/z/f8Pf54E4E

Perhaps. The problem is I don't understand it!

Since you are using C, check your library for an implementation of the ATOMIC_BLOCK macro.  Most AVR compilers will have one. This is the cleanest way to do that.

Thank you. Here's what I've got now:

Code: [Select]

void setLevel(uint8_t channel, uint8_t level) {
//cli(); // disable interrupts
registers[channel] = level;
uint8_t tmpB = PORTB;
uint8_t tmpC = PORTC;
uint8_t tmpD = PORTD;
switch(channel) {
case 0:
OCR1A = registers[4 + level];
if(registers[0] & 1) tmpC |= 1; else tmpC &= ~(1);
if(registers[0] & 2) tmpC |= 1<<1; else tmpC &= ~(1<<1);
PORTC = tmpC;
break;
case 1:
OCR1B = registers[8 + level];
if(registers[1] & 1) tmpC |= 1<<3; else tmpC &= ~(1<<3);
if(registers[1] & 2) tmpC |= 1<<4; else tmpC &= ~(1<<4);
PORTC = tmpC;
break;
case 2:
OCR2A = registers[12 + level];
if(registers[2] & 1) tmpD |= 1<<6; else tmpD &= ~(1<<6);
if(registers[2] & 2) tmpD |= 1<<7; else tmpD &= ~(1<<7);
ATOMIC_BLOCK(ATOMIC_FORCEON) {
// avoid ISR conflicts w. serial pins on PORTD
PORTD = tmpD;
}
break;
case 3:
OCR2B = registers[16 + level];
if(registers[3] & 1) tmpB |= 1<<4; else tmpB &= ~(1<<4);
if(registers[3] & 2) tmpB |= 1<<5; else tmpB &= ~(1<<5);
PORTB = tmpB;
break;
}
//sei(); // re-enable interrupts
}
Not particularly elegant, in fact it looks downright ugly to me, but it does work and does not interfere with the serial communication ISR on PORTD.

[Siwastaja]

Not that ugly. One thing more to do is to not assign all of those tmp variables every time, because they are not used in every case statement; move the reads to where they are needed, inside the cases. Remember, compiler cannot optimize out the reads because the PORT registers are volatile, so this code would result in readout of PORTB, PORTC, PORTD, even if compiler then just ignores the value.

[Lomax]

It works! 

[cv007]

Thank you. Here's what I've got now:

Except as shown (with cli commented out) you now are not atomically setting your pins of interest. You have to protect the read and write 'together'.

Take the route for case 0-

Code: [Select]

uint8_t tmpC = PORTC;
switch(channel) {
case 0:
OCR1A = registers[4 + level];
if(registers[0] & 1) tmpC |= 1; else tmpC &= ~(1);
if(registers[0] & 2) tmpC |= 1<<1; else tmpC &= ~(1<<1);
PORTC = tmpC;
You read PORTC, an interrupt takes place after the read/before the write, and in that interrupt you are blinking an led on portc-
tmpC = PORTC; //led is pin 7, currently off so bit7 is 0
//interrupt fires- led turns on in isr, so PORTC bit7 is now 1
//return from interrupt
tmpC gets bits0/1 set as needed
PORTC = tmpC; //your original read of bit7 was 0, and are now writing that bit as 0
you just turned off the led, which was not your job to do

Your original code looks no worse (probably better), and since you are getting the instructions sbi/cbi you do not have to worry about interrupt protection at all.

For this avr, you either get instructions emitted to get atomic bit writes, or you interrupt protect the whole read/modify/write sequence. To get these instructions, the requirements are a known address in the range these instructions can handle, and a single known bit position-
*(address_range_for_SBI) |= 1|2|4|8|16|32|64|128  //set bit-> sbi 0xaddress, 0xbitposition
*(address_range_for_CBI) &= ~ 1|2|4|8|16|32|64|128  //clear bit-> cbi 0xaddress, 0xbitposition
the compiler will then emit the sbi/cbi instruction (even though the appearance is read/modify/write).

If you fall outside these requirements, then the compile will end up with read/modify/write and will need interrupt protection surrounding this sequence. If you look at arduino source code for the original avr, they interrupt protect all writes to ddr/port as they use port/pin values that are derived at runtime. They also have single functions you use for these writes, so they all end up in the same function with the required interrupt protection applied. If you want to get it right without regard to what the compiler is emitting for instructions, then you would either surround all pin manipulation with interrupt protection, or would create callable functions to do this work so you have a single place to take care of the interrupt protection (same as arduino).

Any other 'modern' mcu with set/clr/[tgl] registers has the ability to set/clr/[tgl] specific bits in a register, and the compiler has no involvement whether you get this atomicity (as long as you use an assignment, =), since it does not depend on the instruction used.
    PORTC.OUTSET = 1<<3; //avr0/1, this is atomically setting pin PC3, no interrupt protection needed regardless of what instruction is used (sts/st)
The big advantage of this is you can now have runtime values for the address and bits, and still get atomicity since it derives from the register used, not the instructions produced-
    mypins[2].port->OUTSET = 1<<(mypins[2].pin);


None of this is specific to ports/pins, and requires thought in about every other area when dealing with registers/variables/etc that are potentially shared. Its also easy to get wrong. The stm32 for example has set/reset gpio registers, but it also has all the other gpio registers shared among pins. If you start changing any of these registers in code at more than one interrupt level, then access to these registers will require interrupt protection. Not that common to be changing pin configurations at runtime, but by the time you do it you will have forgotten the potential problem of doing so, and if not initially treated as a potential future problem only the low probabilities will save you from a corrupt register.

edit-
For these avr's, 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-
https://godbolt.org/z/rjasvG1K5

for the 'modern' mcu such as avr0/, you can come up with any code you want and as long as you use assignment on these set/clr registers, there will be no need for interrupt protection-
https://godbolt.org/z/zs6ce74ce
(nothing optimal about this code, and emitted code is not a pleasant sight, but it will work and you will get your pins set without any need to deal with interrupt protection)

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