On Wed, Aug 29, 2018 at 8:59 PM Christopher Collins <ch...@runtime.io> wrote:
> Hello all, > > I noticed the HAL master I2C API does not include any retry logic. As > you probably know, in I2C, the default state of the SCL line is NACK. > Euh... ACK/NACK is on SDA while the master cycles SCL. > This means an unresponsive peripheral is indistinguishable from one that > is actively nacking the master's writes. Yeup! > If the master's writes are > being NACKed because the slave is simply not ready to receive data, then > retrying seems like the appropriate solution. > Retry is also good if a bus conflict is detected (on a multi-master bus). > I can think of two ways to add I2C retries. Each of them requires > changes: > > (1) Delegate the retry logic to the code calling into the HAL (e.g., > drivers). > (2) Implement retry logic directly in the HAL. > > The problem with (1) is that HAL implementations currently return > MCU-specific error codes. A generic driver cannot determine if a retry > is in order just from inspecting the error code. If there were a common > set of error codes that all HAL I2C implementations returned, drivers > would have the information they need. Actually, even ignoring the > subject of retries, I think common error codes here makes sense. > Agreed, on common codes. I would suggest a common retry system. The retry stuff can get pretty ugly/wrong, so a common hunk o' code is best (IMO). > Re: (2) - I was thinking this could be implemented by adding two new > members to the `hal_i2c_master_data` struct that gets passed to every > master I2C function: > > /** > * Total number of times to attempt the I2C operation. Certain > * failures get retried: > * o NACK received after sending address. > * o (if configured) NACK received after sending data byte. > */ > uint8_t tries; > > /** Whether to retry when a data byte gets NACKed. */ > uint8_t retry_on_dnack:1; > > (I hate to complicate things with the `retry_on_dnack` member, but some > peripherals seem to become unresponsive in the middle of a transaction.) > Or that something monkeyed the bus, so the peripheral decided to stop receiving/processing. ... Your basic point holds: a peripheral might just go away for any number of reasons. I wouldn't even bother with the on_dnack flag. Seems better to consider retry at the whole-packet level. It all gets sent, or it did not and should be retried. Since these additions are members of a struct, rather than new function > parameters, this change would be mostly backwards-compatible. There is > still a problem here, though: code that doesn't zero-out the > `hal_i2c_master_data` struct will likely get retries when it isn't > expecting them, which could conceivably be a problem. > Yeah, or code compiled against old-headers being linked/run against a new-header mynewt (which then looks past the end of the passed-in-struct). I'm not familiar enough with the mynewt API paradigms to make any suggestion. I just know I2C well after writing MCU code for both master and slave, from scratch (PIC16F688 has no built-in I2C). Cheers, -g
