8000 CBI/SBI affect entire register, in contradiction with documented 'true Read-Write-Modify' functionality · Issue #342 · buserror/simavr · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
djfd opened this issue Apr 21, 2019 · 2 comments
Open

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

djfd opened this issue Apr 21, 2019 · 2 comments

Comments

@djfd
Copy link
djfd commented Apr 21, 2019 •
edited
Loading

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
@piopawlu
Copy link

This could potentially solve without introducing any external variable hacks: piopawlu@32112e8

@smalcom
Copy link
smalcom commented Aug 21, 2020

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

@smalcom smalcom mentioned this issue Aug 21, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@djfd @piopawlu @smalcom
0