Re: I2C retries
On Thu, Aug 30, 2018 at 11:35:44AM -0700, Vipul Rahane wrote: > Hey, > > I think that’s a really great idea. One thing I would add is that we > definitely should honor timeouts in any of the retries. Also, a way to > disable the retries should be a good idea. Probably making it a syscfg > which when set to 0 does not do any retries. Yes, the `timo` parameter should apply to each retry. That said, I think a timeout should only occur if there is something wrong with the I2C bus (e.g., the problem we've seen with the nRF52: https://github.com/apache/mynewt-core/blob/1702cdeed8d8f718ed75f40961b9b2f37bae2ff3/hw/mcu/nordic/nrf52xxx/src/hal_i2c.c#L314-L325). > I am assuming the errors codes will be HAL_I2C_ERR_ error codes, > so all the mcus would have to return the same error codes on errors. Yes, sounds good. > Personally, I like calling them retries than tries since if they are > set to 0 the operation still happens once, but that’s just me. `retries` it is, then :). Chris
Re: I2C retries
Sorry, just read Will’s comment. Did not want to repeat it. - Vipul > On Aug 30, 2018, at 11:35 AM, Vipul Rahane wrote: > > Hey, > > I think that’s a really great idea. One thing I would add is that we > definitely should honor timeouts in any of the retries. Also, a way to > disable the retries should be a good idea. Probably making it a syscfg which > when set to 0 does not do any retries. > > I am assuming the errors codes will be HAL_I2C_ERR_ error codes, so all > the mcus would have to return the same error codes on errors. > > Personally, I like calling them retries than tries since if they are set to 0 > the operation still happens once, but that’s just me. > > Regards, > Vipul Rahane > >> On Aug 30, 2018, at 9:48 AM, will sanfilippo wrote: >> >> I think my only comment is tries vs retries. You always want to make at >> least one try so you would have to set that to 1 every time right? I like >> retries personally but that is just me. Mainly because you would never allow >> zero for tries. >> >> >>> On Aug 30, 2018, at 9:21 AM, Christopher Collins wrote: >>> >>> On Wed, Aug 29, 2018 at 10:06:37PM -0500, Greg Stein wrote: 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. >>> >>> Yes, you are right. Thanks for the correction. >>> >>> [...] >>> 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. >>> >>> Thank you for the insight. I am starting to think you are right - >>> retries should be done by the low-level HAL implementation. So, my >>> current thinking is that we should make the following changes: >>> >>> 1. Define a common set of error code for the I2C HAL [1]. >>> 2. Add a single member to the `hal_i2c_master_data` struct: `tries` [2] >>> 3. Modify the HAL I2C implementations so that they retry up to (tries-1) >>> times when an unexpected NACK is recieved. >>> >>> [1] We should do the same for the other HAL APIs (i.e., non-I2C), but >>> that can come later. >>> >>> [2] I prefer specifying number of tries rather than number of retries. >>> I think it helps avoid some ambiguity. >>> >>> All comments welcome. >>> >>> Thanks, >>> Chris >> >
Re: I2C retries
Hey, I think that’s a really great idea. One thing I would add is that we definitely should honor timeouts in any of the retries. Also, a way to disable the retries should be a good idea. Probably making it a syscfg which when set to 0 does not do any retries. I am assuming the errors codes will be HAL_I2C_ERR_ error codes, so all the mcus would have to return the same error codes on errors. Personally, I like calling them retries than tries since if they are set to 0 the operation still happens once, but that’s just me. Regards, Vipul Rahane > On Aug 30, 2018, at 9:48 AM, will sanfilippo wrote: > > I think my only comment is tries vs retries. You always want to make at least > one try so you would have to set that to 1 every time right? I like retries > personally but that is just me. Mainly because you would never allow zero for > tries. > > >> On Aug 30, 2018, at 9:21 AM, Christopher Collins wrote: >> >> On Wed, Aug 29, 2018 at 10:06:37PM -0500, Greg Stein wrote: >>> 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. >> >> Yes, you are right. Thanks for the correction. >> >> [...] >> >>> 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. >> >> Thank you for the insight. I am starting to think you are right - >> retries should be done by the low-level HAL implementation. So, my >> current thinking is that we should make the following changes: >> >> 1. Define a common set of error code for the I2C HAL [1]. >> 2. Add a single member to the `hal_i2c_master_data` struct: `tries` [2] >> 3. Modify the HAL I2C implementations so that they retry up to (tries-1) >> times when an unexpected NACK is recieved. >> >> [1] We should do the same for the other HAL APIs (i.e., non-I2C), but >> that can come later. >> >> [2] I prefer specifying number of tries rather than number of retries. >> I think it helps avoid some ambiguity. >> >> All comments welcome. >> >> Thanks, >> Chris >
Re: I2C retries
On Thu, Aug 30, 2018 at 09:48:32AM -0700, will sanfilippo wrote: > I think my only comment is tries vs retries. You always want to make > at least one try so you would have to set that to 1 every time right? > I like retries personally but that is just me. Mainly because you > would never allow zero for tries. That's a good point. I don't feel too strongly about this. I think my mind just prefers to see a `3` in the code when something is attempted up to three times. If any other HAL APIs specify tries / retries, we should stay consistent with them. Otherwise, I am fine with either. Chris
Re: I2C retries
On Thu, Aug 30, 2018 at 07:47:34PM +0300, marko kiiskila wrote: > > On Aug 30, 2018, at 7:21 PM, Christopher Collins wrote: > > [1] We should do the same for the other HAL APIs (i.e., non-I2C), but > > that can come later. > > Not sure this makes sense for other ones, as i2c is the only one with > ACK. Perhaps we don't need dedicated status code for the other APIs, but I think we want to make sure the implementations don't return MCU-specific errors. Chris
Re: I2C retries
> On Aug 30, 2018, at 7:21 PM, Christopher Collins wrote: > > On Wed, Aug 29, 2018 at 10:06:37PM -0500, Greg Stein wrote: >> 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. > > Yes, you are right. Thanks for the correction. > > [...] > >> 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. > > Thank you for the insight. I am starting to think you are right - > retries should be done by the low-level HAL implementation. So, my > current thinking is that we should make the following changes: > > 1. Define a common set of error code for the I2C HAL [1]. > 2. Add a single member to the `hal_i2c_master_data` struct: `tries` [2] > 3. Modify the HAL I2C implementations so that they retry up to (tries-1) > times when an unexpected NACK is recieved. Sounds good. Extending the struct seems like the natural place to do it. I don’t think we should worry about binary compatibility. > [1] We should do the same for the other HAL APIs (i.e., non-I2C), but > that can come later. Not sure this makes sense for other ones, as i2c is the only one with ACK. > > [2] I prefer specifying number of tries rather than number of retries. > I think it helps avoid some ambiguity. Seems fine to me. > > All comments welcome. > > Thanks, > Chris
