update simavr

[orukusaki]

Updates simavr to the latest (personally I wanted buserror/simavr#550 available)
Redid the twi patch - not totally sure how necessary this is now?

[Patryk27]

Thanks!

While the TWI patch seems to be unnecessary nowadays, there actually seems to be a new bug either in simavr or avr-tester - if you run avr-tester's testsuite (so basically git clone ..., adjust Cargo.toml to point at new simavr and run just test), it will fail on the SPI test:

thread 'examples::spi::test' panicked at avr-tester/tests/examples/spi.rs:18:5:
assertion `left == right` failed
  left: "Ready!"
 right: "Ready!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

git bisect points towards buserror/simavr@604cc13 (2023, good times!) and I need a bit more time to understand what's going on (it's possible that avr-tester has been doing something mildly illegal before that just happened to surface over this commit).

Of course, if you'd like to take a look and debug, feel free as well 😄

[orukusaki]

Wow, super weird bug.
I can't see what avr-simulator might be doing wrong there.
I've tried running the simavr binary against that fixture firmware (with added print statements) and can see the same behaviour happening .

Loaded 392 bytes of Flash data at 0
Loaded 6 bytes of Flash data at 0x188
avr_spi_write = 52
avr_spi_write = 65
avr_spi_write = 61
avr_spi_write = 64
avr_spi_write = 79
avr_spi_write = 21
avr_spi_read = 21
avr_spi_write = 21
avr_spi_read = 21
avr_spi_write = 21
avr_spi_read = 21
avr_spi_write = 21
avr_spi_read = 21
avr_spi_write = 21
etc..

So it looks more like a simavr bug.

What I don't understand though, is why the simavr examples that use spi seem to run ok.

[orukusaki]

Ah I think I get it, reading after writing is always returning the last byte sent, it's your rot13 that's sending those extra !s. still think the bug is in simavr though

[Patryk27]

Ah, I think the test might be wrong after all! -- that test simulates an SPI-master, so actively waiting for incoming data:

https://github.com/Patryk27/avr-tester/blob/be5003e6e9eb7e1aff2764618485dbe5abccef55/avr-tester-fixtures/src/spi.rs#L35

... is wrong, because that's something an SPI-slave would do.

It just happened to work previously, because simavr had a separate buffer for sent and incoming data, which seems not to be how the hardware actually works (i.e. that test wouldn't work on an actual AVR).

I'll adjust the test.

Edit: or maaaybe it's because the irq handler within avr-tester doesn't call avr_regbit_clear(avr, p->spi.raised); 🤔

[orukusaki]

Reading as a master is definitely a valid use case. I've got an old project that uses full duplex SPI - shifting data in and out at the same time. The process is:

1. write to SPDR,
2. wait for SPIF to clear,
3. read from SPDR.

The result will be the byte shifted in while the byte you wrote was shifted out.

Hmm if everything was working the same as the hardware, I think I would expect the first byte read by the firmware in that test to be 0, because the harness only sets up the send buffer after "Ready!" has been received. Because there was nothing to be read into the avr before then, SPDR would still be 0.

Perhaps switching to slave mode would make it a more realistic scenario - since it's job is to respond to bytes sent to it, slave mode would probably be more appropriate.

I think there's a timing issue somewhere too, it seems simavr clears SPIF too early after SPDR is written to, and perhaps avt-tester doesn't leave enough time before writing the next byte - adjusting the increment in pub Spi::tick() seems to have an impact, but I'm not sure what would be correct - maybe you'd have to take the SPI clock bits into account.

I think the process in simavr is a bit wrong, and wouldn't support duplex SPI at all. It seems like in master mode it should be raising an irq when a byte is sent, but also another to collect the next byte shifted in, or maybe that can be done in one?

[Patryk27]

It seems like in master mode it should be raising an irq when a byte is sent, but also another to collect the next byte shifted in, or maybe that can be done in one?

Yes, this seems about right - instead of using avr.spi0().read() and avr.spi0().write(), slave-SPI should be exposed in a similar way to TWI, via a callback that forces you to provide some response:

https://github.com/Patryk27/avr-tester/blob/be5003e6e9eb7e1aff2764618485dbe5abccef55/avr-tester/tests/examples/twi.rs#L16

This way instead of keeping this hack:

https://github.com/Patryk27/avr-tester/blob/be5003e6e9eb7e1aff2764618485dbe5abccef55/avr-simulator/src/spi.rs#L76

... we'd raise SPI_IRQ_OUTPUT during SPI_IRQ_INPUT:

https://github.com/Patryk27/avr-tester/blob/be5003e6e9eb7e1aff2764618485dbe5abccef55/avr-simulator/src/spi.rs#L84

... similarly to TWI:

https://github.com/Patryk27/avr-tester/blob/be5003e6e9eb7e1aff2764618485dbe5abccef55/avr-simulator/src/twi.rs#L51

[Patryk27]

On a second thought, it might be actually easier and better to adjust the SPIF behavior - like you said, it seems to be cleared too early, without simavr waiting for the response to actually appear (which is to say that it probably expects the SPI interrupt to be synchronous and raise the input interrupt at the same time it sends the output interrupt).

[orukusaki]

Oh yeah I think you're on to something, the way you have TWI working looks like the way It'd need to be handled. I wonder if it would just work without any modification to simavr?

[orukusaki]

maybe instead of needing a specific slave device, you could just send from the tx buffer whenever there was something in it - possibly defaulting to 0 is there isn't.

[orukusaki]

Patryk27/avr-tester#8 It works! It probably needs some refactoring though to make it clearer what's going on - and I'm not sure how this would impact when the AVR is in slave mode

[Patryk27]

Nice! Looks like a good way forward - though, like you mention, probably doesn't compose well in AVR in slave mode as the on_output callback doesn't get called then (I think?).

Maybe a good idea would be to have avr.spi0().set_mode(Master | Slave);, which would allow to adjust this behavior? (just spitballing)

[orukusaki]

I've added support for spi slave mode, with a test... it doesn't work 😭

The irqs I'm getting back from simavr are not what I'd expect:
https://github.com/buserror/simavr/blob/master/simavr/sim/avr_spi.c#L103

Here simavr raises an output irq whenever a byte is received in slave mode, but it sends the new byte value, when I would
expect it to send the old one. if I change that line to return the old value, then the test passes.

I'd love if you could have a look and let me know what you think. We were wrong about it being simavr's fault previously, so I'm hesitant, but can't at the moment see any other way it could work.

Also, avr_hal doesn't support slave mode - it always runs in master mode. That can be got around with direct register access, but it makes me wonder how necessary it is to support it in avr-tester, since maybe not a lot of people use it.

[Patryk27]

I see, thanks! I'll take a look on Friday 🙂

[Patryk27]

but it sends the new byte value, when I would expect it to send the old one

Yeah, it seems simavr doesn't provide any way to get the modified SPDR out of it - this:

https://github.com/buserror/simavr/blob/e2ce7f1cbe3ccb77484903d502465a9be65700fd/simavr/sim/avr_spi.c#L103

... doesn't work as expected, because this call:

https://github.com/buserror/simavr/blob/e2ce7f1cbe3ccb77484903d502465a9be65700fd/simavr/sim/avr_spi.c#L98

... is sort of asynchronous - it doesn't execute the interrupt handler right there, it just registers that the interrupt needs to be run some time later in the future (most likely on the next CPU tick), so then:

https://github.com/buserror/simavr/blob/e2ce7f1cbe3ccb77484903d502465a9be65700fd/simavr/sim/avr_spi.c#L103

... raises interrupt with the same value that was just written into the register.

The response-value is available later, over:

https://github.com/buserror/simavr/blob/e2ce7f1cbe3ccb77484903d502465a9be65700fd/simavr/sim/avr_spi.c#L27

... but since simavr doesn't raise the interrupt when AVR is a slave:

https://github.com/buserror/simavr/blob/e2ce7f1cbe3ccb77484903d502465a9be65700fd/simavr/sim/avr_spi.c#L36

... the write to SPDR from within the interrupt handler is effectively discarded.

Considering this, and the fact that avr-hal doesn't allow you to use AVR as slave anyway, I think it's safe not to implement it and stick to just the AVR-as-master scenario.

[orukusaki]

Great I'm glad we reached an agreed way forward. I've redone the necessary changes as Patryk27/avr-tester#9 and updated this PR removing the TWI patch. I'll leave version bumping to you if that's ok.

Thanks for all your input on this, and thanks for creating this great project, it's been really useful for me :)

[Patryk27]

Great, I'm glad you've found the project useful as well 🙂
I'll try to take a look at the changes tomorrow.