Adding USI two wire mode start & stop detection

[AlvyneZ]

For USI two wire (TWI/I2C) mode:

Added the USI Start Detect functionality (SDA falling edge when SCL is high, and SDA pin in input mode) - Raises the Start interrupt vector.

Added the USI Stop Detect functionality (SDA rising edge when SCL is high, and SDA pin in input mode) - Sets the Stop interrupt flag.

[gatk555]

Looks promising, but should it not also do:

2. In addition, the start detector will hold the SCL line low after the master has forced a negative edge on this line (B). This allows the slave to wake up from sleep or complete other tasks before setting up the USI Data Register to receive the address. This is done by clearing the start condition flag and resetting the counter

There is some slightly messy formatting: "int up, down" is indented with spaces and "git diff" shows some rogue white space.

Is there any chance that your application code could become a test case or example, perhaps using VCD input to simulate the master?

Thanks, G.

[AlvyneZ]

I had initially not added the clock stretching feature since I couldn't see a clean way of implementing it. Just realized that the external pin pulling feature of the ioctl module can be used for this. However, it needs to allow for editing of a specific pin's external pull, without editing the pull of other pins in the module.
That is what I have added in the 2f27703 commit.

[AlvyneZ]

For the clock stretching (commit 6967541), I have implemented it as an external pull down on the SCL pin. During simulation, the master would need to keep checking the state of the MCU's pin after trying to set it high.

I am not sure if the USI peripheral also does clock stretching for the overflow interrupt. I do not have any ATtiny chips with me at the moment so I cannot test this. The documentation says "can" instead of the "will" used in the description of the start detector's clock stretching:

5. When the slave is addressed, it holds the SDA line low during the acknowledgment cycle before holding the SCL line low again (i.e., the USI Counter Register must be set to 14 before releasing SCL at (D)). Depending on the R/W bit the master or slave enables its output. If the bit is set, a master read operation is in progress (i.e., the slave drives the SDA line) The slave can hold the SCL line low after the acknowledge (E).

With this commit, I hope I've addressed your concerns @gatk555. All but the examples/test request. I would need to clean up my code first for that :)

Edit: Just realized why the datasheet says "can".. The WM bits can configure the "Hold" when set to 0b11. I will get to this later, my brain is clearly exhausted.

[gatk555]

Ingenious use of the existing ioport features, but I see multiple problems. I do not want to write an essay, so I mention just one: there is no release when USISIF is cleared. But I think you are right that the clock stretching is not easy to implement: the USI peripheral really needs to control the output drivers of the clock pin (override DDR) and the current ioport code has no support for that.

I think transfers can work without clock stretching if the firmware responds promptly. (I seem to recall reading that there are devices that do not tolerate it!) So I can merge the original commit if it includes the white-space changes and a TODO comment to say stretching is still missing. If you want to do that, please force-push it as a single commit.

Thanks, G.

[AlvyneZ]

Please don't hold back on any criticism, I welcome it completely.

For the releasing of the SCL line after USISIF is cleared, I intentionally left it out. I saw it being problematic should the master not have released the line yet. The preferred approach in my opinion is to simply have the slave prevent the master from raising the SCL line during stretching, using the ioport's external pull down.

So the implementation on the simavr board-side master would have to use a write-read loop when trying to raise the line. Only proceeding with the rest of the transmission timings when the line is successfully raised. For a single-threaded implementation:

/**
 * @brief: Clock stretching aware raising of the SCL line after the SCL low period is over
 * @param ma_scl The irq to be toggled by the master, it is connected to irq_scl
 */
void raise_scl_w_stretch(avr_t *avr, struct avr_irq_t *ma_scl) {
    struct avr_irq_t *irq_scl = avr_io_getirq(avr, AVR_IOCTL_USI_GETIRQ(), USI_IRQ_USCK);
    do {
        avr_raise_irq(ma_scl, 1);
        avr_run(avr);
    } while(irq_scl->value == 0);
}

[AlvyneZ]

Just found out from some troubleshooting that the ioport's external pull feature only affects pin changes coming from the simulated firmware and not from other externally connected IRQs.

But I believe the irq flags can be used instead for this purpose. The IRQ_FLAG_USER can be set when the clock is being pulled low. Then the master must always check this flag before trying to pull the clock high:

/**
 * @brief: Clock stretching aware raising of the SCL line after the SCL low period is over
 * @param ma_scl The irq to be toggled by the master, it is connected to the io module irq for the scl pin
 */
void raise_scl_w_stretch(avr_t *avr, avr_irq_t *ma_scl) {
    avr_irq_t *irq_scl = avr_io_getirq(avr, AVR_IOCTL_USI_GETIRQ(), USI_IRQ_USCK);
    do {
        avr_run(avr);
    } while(irq_scl->flags & IRQ_FLAG_USER);
    avr_raise_irq(ma_scl, 1);
}

I will squash the final changes with the first commit 471aa7c, leaving out 2f27703.

[gatk555]

I like this approach, but still see some problems.

You can not use the USER flag here: it's for users and this is core stuff, so this needs to be a new flag. I suggest FORCE or STRONG. The flag could have some real meaning: if it is set and an attempt is made to change the value, avr_raise_irq_float() returns without doing anything. That looks a better way to handle pins with pull-ups than the present method. (The "essay" above would have been mostly why the current pull-up/external stuff can never really work and is a source of bugs.).

The indentation of the new comments in _avr_usi_usck_changed() looks off to me.

[AlvyneZ]

Alright, I will add the IRQ_FLAG_STRONG. As well as the bypass in avr_raise_irq_float().

In the current implementation, the IRQ_FLAG is applied to the irqs of the USI module (which is chained as a listener to the IO module's irqs). I will move it to the irqs of the IO module, since these are the ones that will be chained as listener to the master's irqs.
I will also leave in the flag changes on the irqs of the USI module, as a fallback, in the event the flag is edited by another section of the code in the future. This implementation allows for both:

/**
 * @brief: Clock stretching aware raising of the SCL line after the SCL low period is over
 * @param ma_scl The irq to be toggled by the master, it is connected to irq_scl
 */
void raise_scl_w_stretch_0(avr_t *avr, struct avr_irq_t *ma_scl)
{
    struct avr_irq_t *irq_scl = avr_io_getirq(avr, AVR_IOCTL_USI_GETIRQ(), USI_IRQ_USCK);
    do {
        avr_raise_irq(ma_scl, 1);
        avr_run(avr);
    } while(irq_scl->value == 0);
}
void raise_scl_w_stretch_1(avr_t *avr, avr_irq_t *ma_scl)
{
    avr_irq_t *irq_scl = avr_io_getirq(avr, AVR_IOCTL_USI_GETIRQ(), USI_IRQ_USCK);
    while (irq_scl->flags & IRQ_FLAG_STRONG) {
        avr_run(avr);
    }
    avr_raise_irq(ma_scl, 1);
}