Pull request for fix of SDA glitch

[bobnewgard]

This pull request is associated with issue #4.

The changes in this pull request have been verified only in the testbench at https://codeberg.org/bobn/tb_i2c_herveille_oc. No hardware validation has been done.

The important change is to the ST_SL_WR state of the byte controller c_state FSM. Instead of resetting everything upon slave_reset, the change transitions the FSM to ST_IDLE when the FSM is in ST_SL_WR and slave_reset is high.

I think that the addition of default clauses in FSM case statements is important in the presence of a single event upset (SEU). Of course, SEU mitigation only applies when targeting RAM-based FPGAs.

The FSM without a default clause would go into an indeterminate state, perhaps detectably, but perhaps not. With the default clause, the FSM halts, which is always detectable.

All whitespace changes are immaterial.

[bobnewgard]

I think that the original intent of slave_reset in the if clause for synchronous reset was to provide the byte controller FSM a way to recover from target writes.

In target reads, the final NACK on the bus indicates that it is time to recover, and the FSM uses it to return to ST_IDLE.

In target writes, the FSM ends up in state ST_SL_WR, staying there in case another byte is written on the bus. Currently, return to ST_IDLE is done by resetting the FSM, resulting in the SDA glitch. I'm sure there's a good reason to do it this way, perhaps because the return to ST_IDLE is also required in states ST_SL_WAIT, ST_SL_PRELOAD, ST_SL_ACK.

If that's the case, the tb_i2c_herveille_oc testbench should to be extended to simulate unexpected STA/STO conditions.

Anyway, the fix presented in this pull request gets the FSM out of state ST_SL_WR and back to ST_IDLE upon either STA or STO without an SDA glitch.

[gavin5342]

Thank you, @bobnewgard. I just spent an interesting hour concluding almost the same fix. I left the case statement as is and qualified slave_reset with slave_act - I figure if the FSM is any slave state (e.g. ST_SL_RD) it should go back to idle if START or STOP are detected. I haven't added these sort of error conditions to by tb, yet.

[bobnewgard]

@gavin5342,

I like your fix, since it seems to follow the spirit of the original code better than mine. My fix is limited to conditions uncovered by randomized transactions in the testbench so I cannot be sure that every reasonable case was covered.

If it's possible to push your version to github, please do so. I'd like to see your fix in context.

[bobnewgard]

@gavin5342,

Thanks for the patch (gavin5342/i2c@44856f5). I will validate it in my testbench. If everything looks good, I will update this pull request.