Re: I2C retries
On Wed, Aug 29, 2018 at 8:59 PM Christopher Collins 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
Re: I2C retries
Just a note most of the sensors and drivers have clumsy repeat attempts and timeouts in their init usually on whoami query that could be cleaned up with something like this On Wed, Aug 29, 2018, 6:59 PM Christopher Collins 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. > This means an unresponsive peripheral is indistinguishable from one that > is actively nacking the master's writes. 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. > > 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. > > 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.) > > 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. > > I am leaning towards (1). Thoughts? > > Thanks, > Chris >
I2C retries
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. This means an unresponsive peripheral is indistinguishable from one that is actively nacking the master's writes. 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. 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. 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.) 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. I am leaning towards (1). Thoughts? Thanks, Chris
