8000 Unable to reproduce tests/atmega88_example.axf results · Issue #486 · buserror/simavr · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Unable to reproduce tests/atmega88_example.axf results #486

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
jimustafa opened this issue Apr 14, 2022 · 6 comments
8000
Open

Unable to reproduce tests/atmega88_example.axf results #486

jimustafa opened this issue Apr 14, 2022 · 6 comments

Comments

@jimustafa
Copy link

Running

./simavr/run_avr tests/atmega88_example.axf

yields

Loaded 1722 .text at address 0x0
Loaded 114 .data
Loaded 4 .eeprom
Read from eeprom 0xdeadbeef -- should be 0xdeadbeef..
Read from eeprom 0xcafef00d -- should be 0xcafef00d..

which is inconsistent with the expected output documented in the README.

All the tests are passing...

I am on CentOS 8 and using avr-gcc installed through arduino-cli.
@gatk555
Copy link
Collaborator
gatk555 commented Apr 20, 2022

The README does not say that the program you build will produce that output, just "A program running with these instructions ... will display". Evidently an older version of run_avr with a higher logging level was used. Indeed, you can get closer by adding several "-v" options to the command.

So there seems to be no real issue here.

@gatk555
Copy link
Collaborator
gatk555 commented Apr 20, 2022

It has been logged before: #168 and #445. The main part of #168 (empty VCD file) is fixed, probably by #440.

@jimustafa
Copy link
Author

Thanks for the references; yes, this issue (if even an issue) seems to be a duplicate.

So, this is just a case of the output shown in the README being out of date with the state of the main branch.

@gatk555 gatk555 closed this as completed Nov 1, 2024
@atluft
Copy link
atluft commented Apr 25, 2025 •
edited
Loading

Can this issue be reopened?

I reproduced this issue and was able to get the desired / matching output by modifying the function _avr_set_r to invoke the function avr_raise_irq.

Image

Here is the changed code:

diff --git a/simavr/sim/sim_core.c b/simavr/sim/sim_core.c
index bf7a819..7884fa5 100644
--- a/simavr/sim/sim_core.c
+++ b/simavr/sim/sim_core.c
@@ -284,6 +284,13 @@ static inline void _avr_set_r(avr_t * avr, uint16_t r, uint8_t v)
                avr_io_addr_t io = AVR_DATA_TO_IO(r);
                if (avr->io[io].w.c) {
                        avr->io[io].w.c(avr, r, v, avr->io[io].w.param);
+                       if (avr->io[io].irq) {
+                               avr_raise_irq(avr->io[io].irq + AVR_IOMEM_IRQ_ALL, v);
+                               for (int i = 0; i < 8; i++)
+                               {
+                                       avr_raise_irq(avr->io[io].irq + i, (v >> i) & 1);
+                               }
+                       }
                } else {
                        avr->data[r] = v;
                        if (avr->io[io].irq) {

TLDR

Enable CONFIG_SIMAVR_TRACE and collect a trace.

CFLAGS	+= -Werror -DCONFIG_SIMAVR_TRACE
make
./simavr/run_avr --mcu atmega88 --trace tests/atmega88_example.axf &> output

output

Trace output shows that address UDR0 (0x00c6) is getting modified using the sts instruction.

007c: uart_putchar              sbrs r24[20], 0x20	; Will branch
0080: uart_putchar              sts 0x00c6, YL[52]		io:c6

That specific output comes from these lines of code in function avr_run_one which invokes the avr_set_ram function which then invokes the _avr_set_r function.

avr_run_one invokes avr_set_ram which invokes _avr_set_r

  case 0x9200: {	// STS -- Store Direct to Data Space, 32 bits -- 1001 0010 0000 0000
    get_vd5(opcode);
    uint16_t x = _avr_flash_read16le(avr, new_pc);
    new_pc += 2;
    STATE("sts 0x%04x, %s[%02x]\t\t%s\n",
      x, AVR_REGNAME(d), vd, DAS(x));
    cycle++;
    _avr_set_ram(avr, x, vd);

_avr_set_r has a path to change the value without calling avr_raise_irq.

/*
 * Set a register (r < 256)
 * if it's an IO register (> 31) also (try to) call any callback that was
 * registered to track changes to that register.
 */
static inline void _avr_set_r(avr_t * avr, uint16_t r, uint8_t v)
{
	REG_TOUCH(avr, r);

	if (r == R_SREG) {
		avr->data[R_SREG] = v;
		// unsplit the SREG
		SET_SREG_FROM(avr, v);
		SREG();
	}
	if (r > 31) {
		avr_io_addr_t io = AVR_DATA_TO_IO(r);
		if (avr->io[io].w.c) {
			// ^^^^^^^ THIS PATH WONT RAISE AN IRQ ^^^^^^
			avr->io[io].w.c(avr, r, v, avr->io[io].w.param);
		} else {
			avr->data[r] = v;
			if (avr->io[io].irq) {
				avr_raise_irq(avr->io[io].irq + AVR_IOMEM_IRQ_ALL, v);
				for (int i = 0; i < 8; i++)
				{
					avr_raise_irq(avr->io[io].irq + i, (v >> i) & 1);
				}
			}
		}
        // _call_register_irqs(avr, r); // else section above could then be removed ?
		_call_sram_irqs(avr, r); // Only for io region
	} else
		avr->data[r] = v;
}

@jamesrwaugh
Copy link
jamesrwaugh commented May 3, 2025 •
edited
Loading

Good find @atluft

Running into this myself, yeah, it appears the VCD change dumps do not work for any IO registers with write handlers.

  1. simavr always attaches the VCD change to the IO mem IRQ
  2. In @atluft 's analysis, any IO register with a side effect, aka write handler, skips the IO mem IRQ, thus no change is reported

If the change is made, the question is: Now we would be calling 2 IRQs, the IO one, and the peripheral specific one. There is a peripheral one in this case: the UART TX IRQ. Now, IRQs would be potentially duplicated.

@gatk555 talked about something similar in #540 (comment)

In the latest commits, there is a _call_register_irqs, but there is a bug: The value must be passed instead of reading the register in-function, because in the write case, it is not yet written. Changes for that:

static inline void _call_register_irqs(avr_t * avr, uint16_t addr, uint8_t v)
{
	if (addr > 31 && addr < 31 + MAX_IOs) {
		avr_io_addr_t io = AVR_DATA_TO_IO(addr);

		if (avr->io[io].irq) {
			avr_raise_irq(avr->io[io].irq + AVR_IOMEM_IRQ_ALL, v);
			for (int i = 0; i < 8; i++)
				avr_raise_irq(avr->io[io].irq + i, (v >> i) & 1);
		}
	}
}

_avr_set_r could be then simplified to this;

static inline void _avr_set_r(avr_t * avr, uint16_t r, uint8_t v)
{
	REG_TOUCH(avr, r);

	if (r == R_SREG) {
		avr->data[R_SREG] = v;
		// unsplit the SREG
		SET_SREG_FROM(avr, v);
		SREG();
	}
	if (r > 31) {
		avr_io_addr_t io = AVR_DATA_TO_IO(r);
		if (avr->io[io].w.c) {
			avr->io[io].w.c(avr, r, v, avr->io[io].w.param);
		} else {
			avr->data[r] = v;
		}
		_call_register_irqs(avr, r, v);
		_call_sram_irqs(avr, r); // Only for io region
	} else
		avr->data[r] = v;
}

@atluft
Copy link
atluft commented May 3, 2025

@jamesrwaugh , that approach is much better!
I can confirm changing _call_register_irqs as described by @jamesrwaugh produces VCD output that matches the readme.

Thank you for sharing and explaining the issue of duplicated IRQs. I see that now and prefer the solution you have presented.

@gatk555 gatk555 reopened this May 4, 2025
atluft added a commit to atluft/simavr that referenced this issue May 5, 2025
@atluft
fix: support VCD change dumps for IO registers with write handlers (b…
b008d4b 
…userror#486)
@atluft atluft mentioned this issue May 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
3EDA
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jamesrwaugh @atluft @jimustafa @gatk555
0