buserror
CBI/SBI affect entire register, in contradiction with documented 'true Read-Write-Modify' functionality
Hi,
the situation. A port has some pins assigned as outputs, and some as inputs. Inputs have their associated pin-change interrupts enabled. So when setting just a single output pin with
CBI PORTA, PIN_OUT
we expect to (e.g. see at-tiny-24 reference guide 12.1 IO Ports Overview, p.49):
All Atmel® AVR® ports have true read-modify-write functionality when used as general digital I/O ports. This means that the SBI and CBI instructions can be used to change direction of one port pin without unintentionally changing the direction of any other pin. The same applies when changing drive value (if configured as output) or enabling/disabling of pull-up resistors (if configured as input).
But actually, current implementation is going to update all IRQs for all port pins (even if we are changing the single one only) causing pin change interrupts occurrence for every input pin.
There is backtrace:
-
#0 avr_ioport_irq_notify (irq=0x55555555ae20, value=1, param=0x555555561400)at sim/avr_ioport.c:158 -
#1 0x00007ffff7ebb272 in avr_raise_irq_float (irq=0x55555555ae20, value=1, floating=0)at sim/sim_irq.c:214 -
#2 0x00007ffff7ebb303 in avr_raise_irq (irq=0x55555555ae20, value=1)at sim/sim_irq.c:232 -
#3 0x00007ffff7ecfc1e in avr_ioport_update_irqs (p=0x555555561400)at sim/avr_ioport.c:61 -
#4 0x00007ffff7ecfe36 in avr_ioport_write (avr=0x55555555cfa0, addr=59, v=195 '\303', param=0x555555561400)at sim/avr_ioport.c:89 -
#5 0x00007ffff7ebd020 in _avr_set_r (avr=0x55555555cfa0, r=59, v=195 '\303')at sim/sim_core.c:196 -
#6 0x00007ffff7ebd273 in _avr_set_ram (avr=0x55555555cfa0, addr=59, v=195 '\303')at sim/sim_core.c:247 -
#7 0x00007ffff7ec0a6c in avr_run_one (avr=0x55555555cfa0)at sim/sim_core.c:1266
Particularly, check the frames #7, #3. We are handling the only pin (frame 7), but iterating through all register bits (frame 3) causing the interrupts for every pin configured as input.
Their vars:
(gdb) fr 7
#7 0x00007ffff7ec0a6c in avr_run_one (avr=0x55555555cfa0) at sim/sim_core.c:1266
(gdb) p/x mask
$10 = 0x20
ie. we are handling bit 5, but
(gdb) fr 3
#3 0x00007ffff7ecfc1e in avr_ioport_update_irqs (p=0x555555561400) at sim/avr_ioport.c:61
(gdb) p/x i
$7 = 0x6
triggering pin change interrupt for pin6
Thanks
UPD As a workaround, until proper solution is found, I use this patch which did the trick for my usecase
--- a/simavr/simavr/sim/avr_ioport.c 2019-04-22 10:10:33.167726622 +1000
+++ b/simavr/simavr/sim/avr_ioport.c 2019-04-22 10:12:50.819532128 +1000
@@ -42,6 +42,8 @@
return v;
}
+extern int bit_mask;
+
static void
avr_ioport_update_irqs(
avr_ioport_t * p)
@@ -53,6 +55,10 @@
// otherwise, if the PORT pin was 1 to indicate an
// internal pullup, set that.
for (int i = 0; i < 8; i++) {
+ if (!(bit_mask & (1<<i)))
+ {
+ continue;
+ }
if (ddr & (1 << i))
avr_raise_irq(p->io.irq + i, (avr->data[p->r_port] >> i) & 1);
else if (p->external.pull_mask & (1 << i))
@@ -70,6 +76,7 @@
if (avr->io[port_io].irq) {
avr_raise_irq(avr->io[port_io].irq + AVR_IOMEM_IRQ_ALL, avr->data[p->r_port]);
for (int i = 0; i < 8; i++)
+ if (bit_mask & (1<<i))
avr_raise_irq(avr->io[port_io].irq + i, (avr->data[p->r_port] >> i) & 1);
}
}
--- a/simavr/simavr/sim/sim_core.c 2019-04-22 10:06:30.784471229 +1000
+++ b/simavr/simavr/sim/sim_core.c 2019-04-22 10:08:34.909485872 +1000
@@ -32,6 +32,9 @@
// SREG bit names
const char * _sreg_bit_name = "cznvshti";
+/* bit mask used during SBI/CBI handling to restrict interrupts to this pin only */
+int bit_mask = 0xff;
+
/*
* Handle "touching" registers, marking them changed.
* This is used only for debugging purposes to be able to
@@ -1263,7 +1266,9 @@
get_io5_b3mask(opcode);
uint8_t res = _avr_get_ram(avr, io) & ~mask;
STATE("cbi %s[%04x], 0x%02x = %02x\n", avr_regname(io), avr->data[io], mask, res);
+ bit_mask = mask;
_avr_set_ram(avr, io, res);
+ bit_mask = 0xff;
cycle++;
} break;
case 0x9900: { // SBIC -- Skip if Bit in I/O Register is Cleared -- 1001 1001 AAAA Abbb
@@ -1282,7 +1287,9 @@
get_io5_b3mask(opcode);
uint8_t res = _avr_get_ram(avr, io) | mask;
STATE("sbi %s[%04x], 0x%02x = %02x\n", avr_regname(io), avr->data[io], mask, res);
+ bit_mask = mask;
_avr_set_ram(avr, io, res);
+ bit_mask = 0xff;
cycle++;
} break;
case 0x9b00: { // SBIS -- Skip if Bit in I/O Register is Set -- 1001 1011 AAAA Abbb
This could potentially solve without introducing any external variable hacks: https://github.com/piopawlu/simavr/commit/32112e8f86e1df551d0e3968fbc116f9bda0268d
I've got that trouble today. I decided this using ioctl
avr_ioport.h
#define AVR_IOCTL_IOPORT_GET_EXTERNAL(_name) AVR_IOCTL_DEF('i','o','e',(_name))
avr_ioport.c
/*
* Get the default IRQ values when pin is set as input
*/
if (ctl == AVR_IOCTL_IOPORT_GET_EXTERNAL(p->name)) {
avr_ioport_external_t * m = (avr_ioport_external_t*)io_param;
m->mask = p->external.pull_mask;
m->value = p->external.pull_value;
res = 0;
}
My code with buttons connected to GND:
static void ButtonSet_SetStimul(const uint8_t pButton, const bool pPressed)
{
avr_ioport_external_t ext;
avr_ioctl(MCU, AVR_IOCTL_IOPORT_GET_EXTERNAL(ButtonSet.Port[pButton].Name), &ext);
ext.mask |= (1 << ButtonSet.Port[pButton].Pin);
if(pPressed)
ext.value &= ~(1 << ButtonSet.Port[pButton].Pin);
else
ext.value |= (1 << ButtonSet.Port[pButton].Pin);
avr_ioctl(MCU, AVR_IOCTL_IOPORT_SET_EXTERNAL(ButtonSet.Port[pButton].Name), &ext);
}
static void ButtonSet_Press(const uint8_t pButton)
{
ButtonSet_SetStimul(pButton, true);
avr_raise_irq(&ButtonSet.IRQ[pButton], 0);
}
static void ButtonSet_Release(const uint8_t pButton)
{
ButtonSet_SetStimul(pButton, false);
avr_raise_irq(&ButtonSet.IRQ[pButton], 1);
}
