search

buserror / simavr

simavr is a lean, mean and hackable AVR simulator for linux & OSX
GNU General Public License v3.0
1.56k stars 365 forks source link

CBI/SBI affect entire register, in contradiction with documented 'true Read-Write-Modify' functionality #342

Open djfd opened 5 years ago

djfd commented 5 years ago

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:

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
piopawlu commented 4 years ago

This could potentially solve without introducing any external variable hacks: https://github.com/piopawlu/simavr/commit/32112e8f86e1df551d0e3968fbc116f9bda0268d

smalcom commented 4 years ago

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);
}