Reading from a pulled-up port after writing to it fails

[gergoerdi]

I have the following code, reduced as far as I could:

#include <avr/io.h>
#include <avr/cpufunc.h>
#include <stdbool.h>

#define nanowait() _NOP()

int main ()
{
    /* Input trigger: PD0 */
    DDRD |= _BV(DDD0);
    PORTD |= _BV(PD0);

    /* Input trigger: PC0 */
    DDRC |= _BV(DDC0);
    PORTC |= _BV(PC0);

    /* Set up input pin PC5 with pull-up resistor */
    DDRC &= ~_BV(DDC5);
    PORTC |= _BV(PC5);

    /* Debug output: PB0 */
    DDRB |= _BV(DDB0);
    PORTB &= ~_BV(PB0);

    for (;;)
    {
        /* Read input */
        uint8_t x = 0;

        PORTD &= ~_BV(PD0);
        PORTC &= ~_BV(PC0);
        nanowait();
        x = PINC;
        PORTD |= _BV(PD0);
        PORTC |= _BV(PC0);

        x = (~x & _BV(PC5)) ? 1 : 0;

        /* Write out result to debug port */
        if (x)
            PORTB |= _BV(PB0);
        else
            PORTB &= ~(_BV(PB0));
    }
}

What it tries to do, is to pulse low PD0 and PC0 before reading in some input on PC5. Originally, this came up in the context of a 4x4 keypad where there are four row selectors and four column readers, but the problem can be shown even on this smaller example.

In SimAVR, I set PC5 in response to PD0 going low:

#include <stdlib.h>
#include <stdbool.h>
#include <stdio.h>

#include "sim_avr.h"
#include "avr_ioport.h"
#include "sim_elf.h"

void debug_cb(struct avr_irq_t* irq, uint32_t value, void* closure)
{
    printf("PIN%s%d = %d\n", (char*)closure, irq->irq, value);
}

int count = 0;

void selector_cb(struct avr_irq_t* irq, uint32_t value, void* closure)
{
    avr_irq_t* output = (avr_irq_t*)(closure);

    if (value) return;

    if (++count == 7)
    {
        avr_raise_irq(output, false);
        count = 0;
    } else {
        avr_raise_irq(output, true);
    }
}

int main(int argc, char *argv[])
{
    elf_firmware_t f;
    elf_read_firmware("image.elf", &f);
    f.frequency = 16e6;

    const char *mmcu = "atmega328p";
    avr_t * avr = avr_make_mcu_by_name(mmcu);
    if (!avr)
        return 1;
    avr_init(avr);
    avr_load_firmware(avr, &f);

    const char* keypadName = "keypad";
    avr_irq_t *keypad = avr_alloc_irq(&(avr->irq_pool), 0, 1, &keypadName);

    /* Input selector PD0 */
    avr_irq_register_notify(
        avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('D'), 0),
        selector_cb, keypad);

    /* Input signal PC5 */
    avr_connect_irq(keypad, avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('C'), 5));

    /* Debug output PB0 */
    avr_irq_register_notify(
        avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('B'), 0),
        debug_cb, "B");

    for (;;)
    {
        avr_run(avr);
    }
}

If I run this, I see that the debug output on PB0 never goes high.

However, if I comment out the line PORTC &= ~_BV(PC0); i.e. I don't write to PORTC before reading PINC, then it works as expected and I see PB0 changing between 0 and 1.

[gergoerdi]

I've just noticed that instead of commenting out the write to PORTC, it is enough if I swap the two writes to PORTD and PORTC, i.e. if I write to PORTC first, then PORTD, and then read PINC, then I get the expected result from PINC.

[gergoerdi]

Furthermore, just leaving in another PORTD write, i.e. having

    PORTD &= ~_BV(PD0);
    PORTC &= ~_BV(PC0);
    PORTD &= ~_BV(PD0);
    nanowait();
    x = PINC;

doesn't fix the issue; the PORTD write that triggers changing PC5 really has to happen, it seems, after PORTC is written to.

[gergoerdi]

I have a maybe even simpler way of demonstrating this bug. Take the following AVR program:

#include <avr/io.h>
#include <avr/cpufunc.h>

#define nanowait() _NOP()

#define TRIGGER_PORT C
#define TRIGGER_PIN 0
#define SENSE_PORT C
#define SENSE_PIN 1

#define C(x, y) x ## y

#define DDR(P) C(DDR, P)
#define PORT(P) C(PORT, P)
#define PIN(P) C(PIN, P)

#define DDRT DDR(TRIGGER_PORT)
#define PORTT PORT(TRIGGER_PORT)
#define DDRS DDR(SENSE_PORT)
#define PORTS PORT(SENSE_PORT)
#define PINS PIN(SENSE_PORT)

int main ()
{
    DDRT |= _BV(TRIGGER_PIN);

    DDRS &= ~_BV(SENSE_PIN);
    PORTS = _BV(SENSE_PIN);

    DDRB |= _BV(DDB5);
    PORTB &= ~_BV(PB5);

    for (;;)
    {
        PORTT |= _BV(TRIGGER_PIN);
        nanowait();
        uint8_t x = PINS;
        PORTT &= ~_BV(TRIGGER_PIN);

        if (!(x & _BV(SENSE_PIN)))
            PORTB |= _BV(PB5);
    }
}

and the following SimAVR program:

#include <stdio.h>

#include "sim_avr.h"
#include "avr_ioport.h"
#include "sim_elf.h"

void debug_cb(struct avr_irq_t* irq, uint32_t value, void* closure)
{
    printf("%s%d = %d\n", (char*)closure, irq->irq, value);
}

int count = 0;

void trigger_cb(struct avr_irq_t* irq, uint32_t value, void* closure)
{
    avr_irq_t* output = (avr_irq_t*)(closure);

    if (!value) return;

    if (count == 7)
    {
        avr_raise_irq(output, 0);
    } else {
        printf("Reading %d\n", count);
        avr_raise_irq(output, 1);
        ++count;
    }
}

int main(int argc, char *argv[])
{
    elf_firmware_t f;
    elf_read_firmware("image.elf", &f);
    f.frequency = 16e6;

    const char *mmcu = "atmega328p";
    avr_t * avr = avr_make_mcu_by_name(mmcu);
    if (!avr)
        return 1;
    avr_init(avr);
    avr_load_firmware(avr, &f);

    avr_irq_t *signal = avr_alloc_irq(&(avr->irq_pool), 0, 1, 0);
    avr_raise_irq(signal, 1);

    avr_irq_register_notify(
        avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('B'), 0),
        trigger_cb, signal);
    avr_irq_register_notify(
        avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('C'), 0),
        trigger_cb, signal);
    avr_irq_register_notify(
        avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('D'), 0),
        trigger_cb, signal);

    avr_connect_irq(signal, avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('B'), 1));
    avr_connect_irq(signal, avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('C'), 1));
    avr_connect_irq(signal, avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('D'), 1));

    avr_irq_register_notify(
        avr_io_getirq(avr, AVR_IOCTL_IOPORT_GETIRQ('B'), 5),
        debug_cb, "PORTB");

    for (;;)
    {
        avr_run(avr);
    }
}

Then, we can play around with changing TRIGGER_PORT and SENSE_PORT. In all the configurations I've tried, SimAVR works as expected iff TRIGGER_PORT != SENSE_PORT.

[gergoerdi]

I have also just now discovered that the bug only manifest itself if the "sense" port has its pull-up resistor enabled. By commenting out the line

PORTS = _BV(SENSE_PIN);

I am seeing correct behaviour from SimAVR.

[hovercraft-github]

@gergoerdi : Yes, I also stumbled upon it some time ago. This is actually not a bug, but rather a misconception in how an internal pullup simulation implemented currently (see avr_ioport_update_irqs() in avr_ioport.c). Any pin configured as pulled-up input and connected to any signal source is affected.
In essence, the simulator should be able to sense the output impedance of connected signal source (internal or external), and re-enforce the pull-up only if it would win. The fix for internal source (i.e. PORTXn circuit) may be trivial, but general solution requires introduction of special API, with which user can specify the impedance of the signal source explicitly, at least in two levels: LOW and HIGH.
To @buserror : Michel, please give a hint, what is the preferred way to implement it?

[buserror]

Well, my /prefered/ way was for a 'peripheral' to enforce it's own pullup/down to be fair. The pullup simulation was introduced to handle the case where there the pin isn't attached to anything, so it gets a default state. I knew it wasn't perfect.
The alternative might be complicated, but it might also be done with a 'logic' IRQ 'peripheral' that takes multiple input and generate a single output. that peripheral output would always be hooked to the PIN.

But, I've been thinking we might need to expand the IRQ a bit to handle 'floating' state, when you can raise_irq(... 'X'). I realized I needed that for the VCD anyway...

[hovercraft-github]

@buserror : I vote for your idea of extending IRQ with 'floating' state and can try to sketch it somehow, if you would outline it more deeply.

[buserror]

Hmm github munched my previous comment it seems?!?! FTH

Anyway, @hovercraft-github I don't have a way to contact you, but I've added you as contributor. Please never push to master, we can continue doing pull requests and I will continue to merge to master. As you've seen, I've started to do it for my own features as well.
However, you can edit/review everything else, have fun ;-)

[buserror]

I'll retype what I had typed before tho. Github did munch my message!:

Idea is to add an AVR_IRG_FLAG_FLOAT...
Add an accesor like avr_irq_raise_float(avr, value, float true/false) and use that flag in the main beef of avr_irq_raise to propagate it down the chain.

The receiving handler will be responsible to do a avr_irq_getflags(irq) & ... to know if it's floating or not...

Thats should be compatible with every other piece of code...?

[hovercraft-github]

There is already an unnamed array with flags in sim_irq.h:
enum { IRQ_FLAG_NOT = (1 << 0), //!< change polarity of the IRQ IRQ_FLAG_FILTERED = (1 << 1), //!< do not "notify" if "value" is the same as previous raise IRQ_FLAG_ALLOC = (1 << 2), //!< this irq structure was malloced via avr_alloc_irq IRQ_FLAG_INIT = (1 << 3), //!< this irq hasn't been used yet };
I will just add IRQ_FLAG_FLOAT there, Ok?

[hovercraft-github]

Yes, I believe this shouldn't break any existing code.

[buserror]

Unless you have started this, i'm going to do it @hovercraft-github -- I need it for my VCD files, and it's a good test.

[hovercraft-github]

Ok, I have not made significant progress yet

[buserror]

I've pushed what I had in mind for the avr_irq, and the support for it in the VCD bits. Still need to creep up in the ioports module.

[hovercraft-github]

@buserror Michel, please help me understand the idea behind this. I see there is a new procedure avr_raise_irq_float now. To handle pull-ups with it I think we should add a set of new IRQs into ioport for every pin, something like [IOPORT_IRQ_PIN0_SRC_IMP] = ">srcimp0" .. ">srcimp7", giving the application a way to specify an output impedance of connected circuit. Is it correct?

[buserror]

Do you mean you want to change the pullups states 'externally'? I think it's already handled by the ioport, since you can specify the pullups from the firmware -- perhaps it has an ioctl already? Otherwise we could do with an IRQ to set the pullup values, but it'd need to be get/set 8 bits so it can be 'chained'...

otherwise we could also set an IRQ state that gets set when an IRQ is marked as floating? not sure I like that that much, as it becomes a bit too 'generic' and makes tracking down code more complicated, that's why I prefer to keep the pullup values into the module itself...

If you explain your use case we can come up with something.

[hovercraft-github]

The use case is simple and very common: an application connects an output of some ("external" relative to the simavr) component to the MCU pin, which is configured as internally pulled-up input by the firmware.
For example this can be a bus driver with three states. Whenever the bus driver output goes to the Z (high impedance) state, simavr should set the pin HIGH, i.e. call

avr_raise_irq(p->io.irq + i, 1)

in the avr_ioport_update_irqs. The same when there is nothing connected to the pin at all (the default). Otherwise, when the bus driver output goes to the active (low output impedance) state, simavr should NOT call avr_raise_irq(p->io.irq + i, 1) in the avr_ioport_update_irqs in response to any DDR or PORT writes or other events.
To achieve this behaviour, an application can raise some specific IRQ (using avr_raise_irq_float(irq, value, true)) passing a numeric argument, specifying the output impedance of the connected signal source.
Simavr then can compare this value with internal pull-up resistance (defined in the datasheet) to decide
whether it should take this internal (or whatever) pull-up resistor into account.
I have a poof-of-concept implementation of this in my branch https://github.com/hovercraft-github/simavr/tree/used_with_simutron_cumm - at least it works.

[hovercraft-github]

In principle it would be enough to have just two signal source impedance values: low_source_impedance=0 and high_source_impedance=-1(meaning infinity)

[hovercraft-github]

@buserror
Currently, simavr calls avr_raise_irq(p->io.irq + i, 1) in response to every avr_raise_irq(p->io.irq + i, whatever) from application, when pin is configured as internally pulled-up input.
This is incorrect. Hence this issue #204 (see my two posts above).
Michel?

[hovercraft-github]

To make things clear, the following simple arduino sketch will not work in the current version of simavr:

void setup()
{
	pinMode(0, INPUT_PULLUP);
	pinMode(1, INPUT_PULLUP);
	pinMode(7, OUTPUT);
	pinMode(13, OUTPUT);
}

void loop()
{
	if (digitalRead(0)) {
		digitalWrite(13, HIGH);   // set first LED on
	} else {
		digitalWrite(13, LOW);    // set first LED off
	}
	if (digitalRead(1)) {
		digitalWrite(7, HIGH);   // set second LED on
	}  else {
		digitalWrite(7, LOW);    // set second LED off
	}
}

If we connect buttons to the input pins, the firmware will not sense its press and LEDs will always remain lit.

In PR #246 I created board_keypress example and atmega88_pullups_test which demonstrates this behaviour.
My vision: an application (the host, not AVR firmware!) should have a means to notify the simulator about the fact that some source is connected to the input pin and controls its state. Otherwise the problem has no solution in principle. Also this is a common practice for any simulator and/or HDL.

@buserror Michel, please give your opinion about this and my posts above.

[buserror]

Have you tried AVR_MCU_EXTERNAL_PORT_PULL macros?

simavr/simavr/sim/avr/avr_mcu_section.h

Line 196 in 5715c08

#define AVR_MCU_EXTERNAL_PORT_PULL(_port, _mask, _val) \

It's supposed to handle that sort of case really.

[hovercraft-github]

As far as I can understand, the AVR_MCU_EXTERNAL_PORT_PULL macro controls the state of external pull-ups only. This is not the same feature as an internal pull-up, so seems it's not applicable in our case. When we are connecting an external voltage source to the input pin, our objective in this situation is to avoid call like avr_raise_irq(p->io.irq + i, ...) inside the avr_ioport_update_irqs_pin procedure (avr_ioport.c line 61 for internall pull-ups).
Looking at the source avr_ioport.c, line 58 and below:

	else if (p->external.pull_mask & (1 << i))
		avr_raise_irq(p->io.irq + i, (p->external.pull_value >> i) & 1);
	else if ((avr->data[p->r_port] >> i) & 1)
		avr_raise_irq(p->io.irq + i, 1);

, I would say we can't achieve that even we somehow abuse p->external.