MYPINS = 2;
PORTC = data[1]<<3 | BTN0 | BTN1;
PORTC |= data[1]<<3 & 0b00011000;
static uint8_t const OUT1 = 0b00011000;
...
PORTC |= data[1]<<3 & OUT1;
No overthinking
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;
}
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.
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;
}
}
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;
}
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 likeCode: [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...
struct MyPort {
unsigned int MYPINS:2;
unsigned int OTHERS:6;
} *MyPortC=(struct MyPort *)&PORTC;
MyPortC->MYPINS=2;
Something similar to this might work if you don't mind losing atomicity:
I now wonder if things like PORTC |= XXX are atomic in themselves!
An effective solution is XOR masking
Its 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. reg^=(reg^x)&mask
(simplified by removing extraneous parentheses)I'm curious to know if there's any neat way to group pins
The bits being manipulated don't need to be consecutive.
1. disable interrupts if there is any chance that ISRs may manipulate PORTC at the same time.
...
6. enable interrupts, if applicable
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
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.
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
moving the value bits into the matching pin position is not that pleasant, and the code also becomes uglier at least in this case
Getting a group of pins set atomically individually is fine in most cases, even though will not be atomically set as a group.
Now there's a good idea, is that as simple as doing cli(); at the top of the function and sei(); at the end?
But I now understand that using runtime values this way is not recommended, because using runtime values in port assignments is not atomic.
uint8_t tmp = DDRC;
tmp <<= 1;
tmp |= 0b10111001;
if(something)
tmp &= 0b1;
DDRC = tmp;
DDRC <<= 1;
DDRC |= 0b10111001;
if(something)
DDRC &= 0b1;
uint8_t tmp_a = ...;
uint8_t tmp_b = ...;
cli();
PORTA = tmp_a;
PORTB = tmp_b;
sei();
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
How about using a structure with bitfields? https://godbolt.org/z/f8Pf54E4E
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.
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.
Thank you. Here's what I've got now:
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;
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);
}
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);
}
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
main: int x = PORTC;
ISR: int y = PORTC;
ISR: PORTC = y | PIN1;
main: PORTC = x | PIN2; // at this point PIN1 gets turned off
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);
}
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
in r25,0x8
in r18,0x8
eor r24,r25
andi r24,lo8(3)
eor r24,r18
out 0x8,r24
ret
#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. #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. which exploits the ability of the port's PINx register to toggle any pins you write a '1' bit to
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:
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();
}
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);
}
}
You need to tell us what your circuitry does
now, in this example,