TWI: TWCR and TWSR inconsistencies

[taylorconor]

HI!

I'm doing a read transaction using poll-based acking (as opposed to TWIE) on an atmega32u4. The MCU's datasheet (section 20.5.5) states:

When an event requiring the attention of the application occurs on the TWI bus, the TWI Interrupt Flag (TWINT) is asserted. In the next clock cycle, the TWI Status Register (TWSR) is updated with a status code identifying the event.

Code that uses the megax TWI modules in this way inspect the TWSR immediately after TWINT is cleared, e.g.:

TWCR = (1 << TWINT) | (1 << TWEN);
while (!(TWCR & (1 << TWINT))) {}
if (TWSR != TW_MR_SLA_ACK) {
  // fail the transaction.
}

However when running within simavr, TWINT is immediately asserted, without the corresponding TWSR flags. The TWSR flags are updated after a number of clock cycles, so this can be mitigated with busylooping within the AVR code. This is obviously not a workable solution, and is inconsistent with the description of the TWSR from the MCU's datasheet.

This is possibly related to #137. Is this a simavr issue, or have I misunderstood the simulated TWI integration?

[taylorconor]

I'm currently working around this issue using the following mitigation:

diff --git a/simavr/sim/avr_twi.c b/simavr/sim/avr_twi.c
index 521c03c..649b635 100644
--- a/simavr/sim/avr_twi.c
+++ b/simavr/sim/avr_twi.c
@@ -319,7 +319,7 @@ avr_twi_write(

                        if (p->peer_addr & 1) { // read ?
                                p->state |= TWI_COND_READ;      // always allow read to start with
-                               _avr_twi_delay_state(p, 9,
+                               _avr_twi_delay_state(p, 0,
                                                p->state & TWI_COND_ACK ?
                                                                TWI_MRX_ADR_ACK : TWI_MRX_ADR_NACK);
                        } else {
@@ -328,7 +328,7 @@ avr_twi_write(
                                                        p->state & TWI_COND_ACK ?
                                                                        TWI_MTX_ADR_ACK : TWI_MTX_ADR_NACK);
                                }else{
-                                       _avr_twi_delay_state(p, 9,
+                                       _avr_twi_delay_state(p, 0,
                                                        p->state & TWI_COND_ACK ?

This forces TWSR to be accurate as soon as TWINT is cleared (set to 1) in TWCR, so the non-interrupt-based TWI implementation in my original comment above works as expected.

What is the purpose of the 9 TWI cycle delay for these updates? Are there integrations that depend on this?

[buserror]

I'm not sure why is this, but I don't think I would have made a convoluted timer like this without a good reason; possibly the older AVRs have that issue and I used an old, "common" AVR as a reference?

[taylorconor]

Thanks for taking a look.

Is it possible that simavr's twi module is only designed to work with firmware that uses an interrupt-driven twi integration? It seems incorrect that TWINT is cleared before updating TWSR, but this would make sense if the simulator assumes that the firmware won't react to this until TWI_IRQ_STATUS is raised. The 9-cycle delay would in that case make sense as a simulated delay for receipt of the full 9-bit address packet on the TWI bus.

In any case, do you have a suggestion for how I should proceed with resolving this? I have some options assuming the above assumption is correct:

1. Rework the avr_twi module to satisfy interrupt-driven and poll-driven twi integrations. There's a risk of backwards incompatibility here or breaking assumptions made by other users of simavr. Is there a way of doing this safely?
2. Introduce some new IRQs in simavr for poll-driven twi, to isolate the interrupt-driven code from potential breaking changes. This pollutes the IRQ space though.
3. Maintain my own fork. I'd really rather not have to do this for what is currently a 2-byte diff.

I'd appreciate any opinions!