From 3c6de37f387e1d310007d2969977e0cda09113ff Mon Sep 17 00:00:00 2001 From: ga Date: Sat, 27 Mar 2021 12:29:51 +0000 Subject: [PATCH] Make the 2560 PORTE/0 pin change interrupt visible as it mostly works. Add the missing pin change interrupts for 1280 and 1281. In avr_ioport.[ch], fix multiple causes (but not all) of mis-triggering of pin change interrupts including upstream issue #343: Pin change interrupts incorrectly fire when a timer compare event occurs. Other causes are initial setting of DDR and the AVR_IOPORT_OUTPUT bit. Move the mask and shift data for ATmega2560 into the ioport structure. Add a test for pin change interrupts. --- simavr/cores/sim_mega1280.c | 34 ++++++--- simavr/cores/sim_mega1281.c | 20 ++--- simavr/cores/sim_mega2560.h | 32 ++------ simavr/sim/avr_ioport.c | 50 ++++++++++--- simavr/sim/avr_ioport.h | 22 +++++- tests/atmega2560_pin_change.c | 115 +++++++++++++++++++++++++++++ tests/test_atmega2560_pin_change.c | 41 ++++++++++ 7 files changed, 256 insertions(+), 58 deletions(-) create mode 100644 tests/atmega2560_pin_change.c create mode 100644 tests/test_atmega2560_pin_change.c diff --git a/simavr/cores/sim_mega1280.c b/simavr/cores/sim_mega1280.c index 316bfd6..13685dc 100644 --- a/simavr/cores/sim_mega1280.c +++ b/simavr/cores/sim_mega1280.c @@ -85,23 +85,35 @@ const struct mcu_t { AVR_EXTINT_MEGA_DECLARE(7, 'E', PE7, B), }, AVR_IOPORT_DECLARE(a, 'A', A), - .portb = { - .name = 'B', .r_port = PORTB, .r_ddr = DDRB, .r_pin = PINB, + AVR_IOPORT_DECLARE_PC(b, 'B', B, 0), // PB0-7 have PCINT0-7 + AVR_IOPORT_DECLARE(c, 'C', C), + AVR_IOPORT_DECLARE(d, 'D', D), + .porte = { + .name = 'E', .r_port = PORTE, .r_ddr = DDRE, .r_pin = PINE, .pcint = { - .enable = AVR_IO_REGBIT(PCICR, PCIE0), - .raised = AVR_IO_REGBIT(PCIFR, PCIF0), - .vector = PCINT0_vect, + .enable = AVR_IO_REGBIT(PCICR, PCIE1), + .raised = AVR_IO_REGBIT(PCIFR, PCIF1), + .vector = PCINT1_vect, }, - .r_pcint = PCMSK0, + .r_pcint = PCMSK1, + .mask = 1, // PE0 has PCINT8 + .shift = 0 }, - AVR_IOPORT_DECLARE(c, 'C', C), - AVR_IOPORT_DECLARE(d, 'D', D), - AVR_IOPORT_DECLARE(e, 'E', E), AVR_IOPORT_DECLARE(f, 'F', F), AVR_IOPORT_DECLARE(g, 'G', G), AVR_IOPORT_DECLARE(h, 'H', H), - AVR_IOPORT_DECLARE(j, 'J', J), - AVR_IOPORT_DECLARE(k, 'K', K), + .portj = { + .name = 'J', .r_port = PORTJ, .r_ddr = DDRJ, .r_pin = PINJ, + .pcint = { + .enable = AVR_IO_REGBIT(PCICR, PCIE1), + .raised = AVR_IO_REGBIT(PCIFR, PCIF1), + .vector = PCINT1_vect, + }, + .r_pcint = PCMSK1, + .mask = 0b11111110, // PJ0-6 have PCINT9-15 + .shift = -1 + }, + AVR_IOPORT_DECLARE_PC(k, 'K', K, 2), // PK0-7 have PCINT16-23 AVR_IOPORT_DECLARE(l, 'L', L), AVR_UARTX_DECLARE(0, PRR0, PRUSART0), diff --git a/simavr/cores/sim_mega1281.c b/simavr/cores/sim_mega1281.c index 222743c..1d121e5 100644 --- a/simavr/cores/sim_mega1281.c +++ b/simavr/cores/sim_mega1281.c @@ -81,18 +81,20 @@ const struct mcu_t { AVR_EXTINT_MEGA_DECLARE(7, 'E', PE7, B), }, AVR_IOPORT_DECLARE(a, 'A', A), - .portb = { - .name = 'B', .r_port = PORTB, .r_ddr = DDRB, .r_pin = PINB, + AVR_IOPORT_DECLARE_PC(b, 'B', B, 0), // PB0-7 have PCINT0-7 + AVR_IOPORT_DECLARE(c, 'C', C), + AVR_IOPORT_DECLARE(d, 'D', D), + .porte = { + .name = 'E', .r_port = PORTE, .r_ddr = DDRE, .r_pin = PINE, .pcint = { - .enable = AVR_IO_REGBIT(PCICR, PCIE0), - .raised = AVR_IO_REGBIT(PCIFR, PCIF0), - .vector = PCINT0_vect, + .enable = AVR_IO_REGBIT(PCICR, PCIE1), + .raised = AVR_IO_REGBIT(PCIFR, PCIF1), + .vector = PCINT1_vect, }, - .r_pcint = PCMSK0, + .r_pcint = PCMSK1, + .mask = 1, // PE0 has PCINT8 + .shift = 0 }, - AVR_IOPORT_DECLARE(c, 'C', C), - AVR_IOPORT_DECLARE(d, 'D', D), - AVR_IOPORT_DECLARE(e, 'E', E), AVR_IOPORT_DECLARE(f, 'F', F), AVR_IOPORT_DECLARE(g, 'G', G), diff --git a/simavr/cores/sim_mega2560.h b/simavr/cores/sim_mega2560.h index 6f46993..e42ac49 100644 --- a/simavr/cores/sim_mega2560.h +++ b/simavr/cores/sim_mega2560.h @@ -87,32 +87,20 @@ const struct mcu_t { AVR_EXTINT_MEGA_DECLARE(7, 'E', PE7, B), }, AVR_IOPORT_DECLARE(a, 'A', A), - .portb = { - .name = 'B', .r_port = PORTB, .r_ddr = DDRB, .r_pin = PINB, - .pcint = { - .enable = AVR_IO_REGBIT(PCICR, PCIE0), - .raised = AVR_IO_REGBIT(PCIFR, PCIF0), - .vector = PCINT0_vect, - }, - .r_pcint = PCMSK0, // PB0-7 have PCINT0-7 - }, + AVR_IOPORT_DECLARE_PC(b, 'B', B, 0), // PB0-7 have PCINT0-7 AVR_IOPORT_DECLARE(c, 'C', C), AVR_IOPORT_DECLARE(d, 'D', D), -#ifdef SPLIT_INTERRUPTS .porte = { .name = 'E', .r_port = PORTE, .r_ddr = DDRE, .r_pin = PINE, .pcint = { .enable = AVR_IO_REGBIT(PCICR, PCIE1), .raised = AVR_IO_REGBIT(PCIFR, PCIF1), .vector = PCINT1_vect, - .mask = 1, // PE0 has PCINT8 - .shift = 0 }, .r_pcint = PCMSK1, + .mask = 1, // PE0 has PCINT8 + .shift = 0 }, -#else - AVR_IOPORT_DECLARE(e, 'E', E), // PE0 has PCINT8 -#endif AVR_IOPORT_DECLARE(f, 'F', F), AVR_IOPORT_DECLARE(g, 'G', G), AVR_IOPORT_DECLARE(h, 'H', H), @@ -122,20 +110,12 @@ const struct mcu_t { .enable = AVR_IO_REGBIT(PCICR, PCIE1), .raised = AVR_IO_REGBIT(PCIFR, PCIF1), .vector = PCINT1_vect, - .mask = 0b11111110, // PJ0-6 have PCINT9-15 - .shift = -1 }, .r_pcint = PCMSK1, + .mask = 0b11111110, // PJ0-6 have PCINT9-15 + .shift = -1 }, - .portk = { - .name = 'K', .r_port = PORTK, .r_ddr = DDRK, .r_pin = PINK, - .pcint = { - .enable = AVR_IO_REGBIT(PCICR, PCIE2), - .raised = AVR_IO_REGBIT(PCIFR, PCIF2), - .vector = PCINT2_vect, - }, - .r_pcint = PCMSK2, // PK0-7 have PCINT16-23 - }, + AVR_IOPORT_DECLARE_PC(k, 'K', K, 2), // PK0-7 have PCINT16-23 AVR_IOPORT_DECLARE(l, 'L', L), AVR_UARTX_DECLARE(0, PRR0, PRUSART0), diff --git a/simavr/sim/avr_ioport.c b/simavr/sim/avr_ioport.c index ab632f1..13dfea6 100644 --- a/simavr/sim/avr_ioport.c +++ b/simavr/sim/avr_ioport.c @@ -143,19 +143,47 @@ avr_ioport_irq_notify( int output = value & AVR_IOPORT_OUTPUT; value &= 0xff; uint8_t mask = 1 << irq->irq; - // set the real PIN bit. ddr doesn't matter here as it's masked when read. - avr->data[p->r_pin] &= ~mask; - if (value) - avr->data[p->r_pin] |= mask; + uint8_t ddr = avr->data[p->r_ddr]; - if (output) // if the IRQ was marked as Output, also do the IO write - avr_ioport_write(avr, p->r_port, (avr->data[p->r_port] & ~mask) | (value ? mask : 0), p); + if (output) { + if ((mask & ddr) == 0) + return; // TODO: stop further processing of IRQ. + + // If the IRQ was marked as Output, also do the IO write. + + avr_ioport_write(avr, + p->r_port, + (avr->data[p->r_port] & ~mask) | + (value ? mask : 0), + p); + } else { + // Set the real PIN bit. Ignore DDR as it's masked when read. + + avr->data[p->r_pin] &= ~mask; + if (value) + avr->data[p->r_pin] |= mask; + + /* BUG: If DDR bit is set here, there should be no + * interrupt. But a spurious IRQ call by the user + * is indestinguishable from an internal one + * caused by writing the output port register and + * that should cause an interrupt. Doh! + */ + } if (p->r_pcint) { + // Ignore lingering copy of AVR_IOPORT_OUTPUT, or + // differing non-zero values. + + if (!value == !(irq->value & 0xff)) + return; + // if the pcint bit is on, try to raise it + int raisedata = avr->data[p->r_pcint]; - uint8_t uiRegMask = p->pcint.mask; - int8_t iShift = p->pcint.shift; + uint8_t uiRegMask = p->mask; + int8_t iShift = p->shift; + if (uiRegMask) // If mask is 0, do nothing (backwards compat) raisedata &= uiRegMask; // Mask off @@ -282,9 +310,11 @@ void avr_ioport_init(avr_t * avr, avr_ioport_t * p) // allocate this module's IRQ avr_io_setirqs(&p->io, AVR_IOCTL_IOPORT_GETIRQ(p->name), IOPORT_IRQ_COUNT, NULL); - for (int i = 0; i < IOPORT_IRQ_REG_PIN; i++) + for (int i = 0; i < IOPORT_IRQ_REG_PIN; i++) { p->io.irq[i].flags |= IRQ_FLAG_FILTERED; - + if (i < IOPORT_IRQ_PIN_ALL) + p->io.irq[i].flags &= ~IRQ_FLAG_INIT; + } avr_register_io_write(avr, p->r_port, avr_ioport_write, p); avr_register_io_read(avr, p->r_pin, avr_ioport_read, p); avr_register_io_write(avr, p->r_pin, avr_ioport_pin_write, p); diff --git a/simavr/sim/avr_ioport.h b/simavr/sim/avr_ioport.h index 5734df8..024b695 100644 --- a/simavr/sim/avr_ioport.h +++ b/simavr/sim/avr_ioport.h @@ -104,9 +104,15 @@ typedef struct avr_ioport_t { avr_io_addr_t r_pin; avr_int_vector_t pcint; // PCINT vector - avr_io_addr_t r_pcint; // pcint 8 pins mask + avr_io_addr_t r_pcint; // pcint 8 pins mask - // this represent the default IRQ value when + // Mask and shift for PCINTs. This is needed for chips like the 2560 + // where PCINT do not align with IRQs. + + uint8_t mask; + int8_t shift; + + // This represent the default IRQ value when // the port is set as input. // If the mask is not set, no output value is sent // on the output IRQ. If the mask is set, the specified @@ -123,6 +129,18 @@ void avr_ioport_init(avr_t * avr, avr_ioport_t * port); .name = _cname, .r_port = PORT ## _uname, .r_ddr = DDR ## _uname, .r_pin = PIN ## _uname, \ } +#define AVR_IOPORT_DECLARE_PC(_lname, _cname, _uname, _pcnum) \ + .port ## _lname = { \ + .name = _cname, .r_port = PORT ## _uname, \ + .r_ddr = DDR ## _uname, .r_pin = PIN ## _uname, \ + .pcint = { \ + .enable = AVR_IO_REGBIT(PCICR, PCIE ## _pcnum), \ + .raised = AVR_IO_REGBIT(PCIFR, PCIF ## _pcnum), \ + .vector = PCINT ## _pcnum ## _vect, \ + }, \ + .r_pcint = PCMSK ## _pcnum, \ + } + #ifdef __cplusplus }; #endif diff --git a/tests/atmega2560_pin_change.c b/tests/atmega2560_pin_change.c new file mode 100644 index 0000000..a715aba --- /dev/null +++ b/tests/atmega2560_pin_change.c @@ -0,0 +1,115 @@ +/* + atmega2560_pin_change.c + + Test for pin_change interrupt simulation. + */ + +#include +#include +#include +#include + +#include "avr_mcu_section.h" + +/* + * This demonstrate how to use the avr_mcu_section.h file + * The macro adds a section to the ELF file with useful + * information for the simulator + */ +AVR_MCU(F_CPU, "atmega2560"); + +static int uart_putchar(char c, FILE *stream) +{ + if (c == '\n') + uart_putchar('\r', stream); + loop_until_bit_is_set(UCSR3A, UDRE3); + UDR3 = c; + return 0; +} + +static FILE mystdout = FDEV_SETUP_STREAM(uart_putchar, NULL, + _FDEV_SETUP_WRITE); + +volatile uint8_t interrupts; +volatile char buffer[32]; + +ISR(PCINT0_vect) +{ + buffer[interrupts++] = '0'; +} + +ISR(PCINT1_vect) +{ + buffer[interrupts++] = '1'; +} + +ISR(PCINT2_vect) +{ + buffer[interrupts++] = '2'; +} + +int main() +{ + stdout = &mystdout; + PCICR = (1 <value ^ 1) | flag); +} + +int main(int argc, char **argv) { + static const char *expected = " 0 0 1021 102210\r\n"; + avr_t *avr; + + tests_init(argc, argv); + avr = tests_init_avr("atmega2560_pin_change.axf"); + + twiddle_irq = avr_io_getirq(avr, + AVR_IOCTL_IOPORT_GETIRQ('B'), + IOPORT_IRQ_PIN4); + avr_irq_register_notify(avr_io_getirq(avr, + AVR_IOCTL_IOPORT_GETIRQ('A'), + IOPORT_IRQ_REG_PORT), + reg_write, avr); + + tests_assert_uart_receive_avr(avr, 10000000, expected, '3'); + tests_success(); + return 0; +} -- 2.39.5