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!

[jamesrwaugh]

Just spent the last few hours debugging this same issue. Using poll-based TWI handling, like so. As stated above, TW_STATUS is not set before TWINT is cleared. Enabling debugging logging, it is clear the status is updated only later.

My option was to maintain a fork for now, since I am not using interrupt-driven TWI and this is the quickest option forward, as the others by @taylorconor seem to be large refactors.

But according to most of the (newer?) AVR datasheets I can find, it appears the correct behavior would be to set it immediately. I am fairly new to AVRs in general, but it still seems like even if interrupt-driven, the status would be updated before TWINT cleared, given that:

• The Datasheets of Atmegas, and an "older" avr such as ATtiny48 have the same text in the OP and show while (!(TWCR0 & (1<<TWINT))); as a code sample, implying an immediate set is correct, and currently would not work in simavr;
• I am not seeing any other mention of a slower set of TW_STATUS if interrupt-driven logic is used.

I am wondering if a counter-example exists that suggest the reason for a delayed set? However, if there is one, it seems incorrect based on the AVR list simavr supports and is not correct for the vast majority of chips.

  /* Wait for TWINT flag to set */
  while (!(TWCR & (1 << TWINT)))
    ;
  if (TW_STATUS != TW_MT_DATA_ACK) {    // Oh no, TW_STATUS is not updated yet
    return TW_STATUS;
  }