Configuring AVR USI_IRQ_DO for two wire mode data output onto SDA line

[AlvyneZ]

AVR USI in two wire mode - pushing data onto SDA line:
Adding IRQ hooking and unhooking for USI_IRQ_DO and USI_IRQ_DI on DDR changes, in order to flip between shifting in and shifting out data.

A cleaner solution may be needed for the DDR io_write registering section:

avr_register_io_write(avr, (p->pin_di.reg - 1), avr_usi_write_ddr, p);

I am also working on an example board for the examples section. I will add it in a following commit in this pull request.

[gatk555]

Thanks for the example code. I have some comments, there may be more.

72dea82:
My guess is that this can be simplified. The DI IRQ need not be disconnected as the datasheet shows nothing that does that. That does mean firmware can give itself a start interrupt by writing to the output port. It might even be useful (and could be tested on hardware).

Can the DO IRQ also be left connected? It will still call the IOPORT IRQ when reading, but that should be ignored if the pin is set to input, because AVR_IOPORT_OUTPUT is set. When the IRQ comes back to USI, the flag can be used to ignore the call.

Without those, there is not much left, but the change to _avr_usi_di_changed() still looks correct to me. Again, there is nothing in the datasheet to suggest that the USI hardware monitors the DDR. I should have picked that up before.

There is a call to avr_connect_irq() in _avr_usi_disconnect_irqs(), that looks wrong.

Why add "FALLTHROUGH"? I never saw that used unless some code comes first.

51aec0d:
Not yet reviewed, but I noticed that console output from firmware is now green, like logged UART output. That may surprise people, but seems an improvement. But I think this very visible change should not be buried in a commit that adds an example. Can it be in a separate commit?

[AlvyneZ]

I was not aware that the firmware can loop a start condition back to itself. I currently don't have the physical hardware with me so I cannot test the actual behavior. I will update the implementation to remove the connecting and disconnecting of the DI_IRQ & DO_IRQ.

For the point on USI hardware not monitoring DDR, I misinterpreted the section of the datasheet under WM=10: The SCL line is held low when a start detector detects a start condition and the output is enabled. Clearing the Start Condition Flag (USISIF) releases the line. The SDA and SCL pin inputs is not affected by enabling this mode. Pull-ups on the SDA and SCL port pin are disabled in Two-wire mode.

I've gotten rid of the stray FALLTHROUGH's and the avr_connect_irq() in _avr_usi_disconnect_irqs().

Because of the start condition feedback, the 50 to 300 ns delay of the SDA line at the input of the start condition detector is necessary. I have implemented it as a timer delayed by a single cycle.

For the console output, I have changed it to blue to ensure a clear difference from the UART output. I hope that is OK.

[gatk555]

Please could you explain why the new delay functions are needed, perhaps in a comment?

There is some rogue white space that shows up red in "git show", in those two functions, the Makefile and i2ctest_usislave.c.

I thought the green was fine, as UART and "console" are both displaying firmware output. But no problem with blue.

I am interested that you use your own formatting in firmware. I guess that comes out a lot smaller than printf().

Thanks, G.

[AlvyneZ]

The delays are needed primarily because of the Start condition feedback. Because of the sequence of hooks executions, without a delay, if the SDA line is to be flipped from high to low at the falling edge of the SCL line, it would trigger a Start condition. What would happen is:

• Master causes a falling edge on SCL.
• Falling edge propagates through hooks till USI_IRQ_USCK, and _avr_usi_usck_changed() executes.
• Since the edge is falling, _avr_usi_push_high_bit() executes, raising USI_IRQ_DO to 0x100 if USIDR=0.
• The raising propagates through hooks till USI_IRQ_DI, and at this point the IRQ_USCK value is still 1 since its callbacks haven't all completed. Note: In avr_ioport.c, there is a TODO on line 184: // TODO: stop further processing of IRQ., should this be implemented, this final propagating would not happen and the delay would be unnecessary. I however saw that since hardware implements it, there'd be no harm in adding it.

I have gotten rid of the rogue white spaces. I have force pushed the fix.

The issue with printf was its speed especially initially before the clock stretching was implemented. I needed a quicker execution so thats the v_puts and v_putchar.

[gatk555]

Thanks for the explanation. I guessed it was something like that, but did not consider it acting on a clock edge. I think your experience with both PRs shows that the IOPORT code really needs better support for pin take-overs. I have made a slow start on supporting the newer AVRs and the IOPORT for that keeps state for seized pins. It is still a skeleton implementation though.

There is still some "red" white space in avr_usi.c, but I merged anyway: I think I have asked for enough changes already. But if you plan to do more here, please remove it next time.

Thanks, G.

Edit: White space is gone, as there were some build failures, that I fixed. I though it had already built, but apparently not. I also killed a build that I think you started as it was stuck in set-up. I was surprised it let me do that.

[AlvyneZ]

For the TODO in the avr_ioport.c file, I think it can be solved with an IRQ_FLAG. The notify function can set the flag if it wants to stop further processing of the irq. Maybe define another flag IRQ_FLAG_NTF_XHOOKS and define it to the same value as IRQ_FLAG_INIT (at that point it has already been cleared and serves no more use). The overlapping would be so as to avoid using all the bits of the 8-bit flag.

Edit: I've tried to implement this in PR #578