You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mynewt.apache.org by Christopher Collins <ch...@runtime.io> on 2018/08/30 01:59:42 UTC
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
Re: I2C retries
Posted by Christopher Collins <ch...@runtime.io>.
On Thu, Aug 30, 2018 at 07:47:34PM +0300, marko kiiskila wrote:
> > On Aug 30, 2018, at 7:21 PM, Christopher Collins <ch...@runtime.io> 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
Posted by marko kiiskila <ma...@runtime.io>.
> On Aug 30, 2018, at 7:21 PM, Christopher Collins <ch...@runtime.io> 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 <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.
>
> 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
Re: I2C retries
Posted by Vipul Rahane <vi...@runtime.io>.
Sorry, just read Will’s comment.
Did not want to repeat it.
- Vipul
> On Aug 30, 2018, at 11:35 AM, Vipul Rahane <vi...@runtime.io> 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_XXXX 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 <wi...@runtime.io> 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 <ch...@runtime.io> 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 <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.
>>>
>>> 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
Posted by Christopher Collins <ch...@runtime.io>.
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_XXXX 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
Posted by Vipul Rahane <vi...@runtime.io>.
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_XXXX 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 <wi...@runtime.io> 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 <ch...@runtime.io> 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 <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.
>>
>> 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
Posted by Christopher Collins <ch...@runtime.io>.
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
Posted by will sanfilippo <wi...@runtime.io>.
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 <ch...@runtime.io> 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 <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.
>
> 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
Posted by Christopher Collins <ch...@runtime.io>.
On Wed, Aug 29, 2018 at 10:06:37PM -0500, Greg Stein wrote:
> 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.
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
Posted by Greg Stein <gs...@gmail.com>.
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
Re: I2C retries
Posted by Christopher Collins <ch...@runtime.io>.
Thanks for clarifying, Will.
FYI- I have created a second PR which implements retries at the driver
level rather than the HAL. For comparison, the two PRs are:
HAL: https://github.com/apache/mynewt-core/pull/1373
Driver: https://github.com/apache/mynewt-core/pull/1375
I prefer the "driver" version because the required MCU changes are
minimal.
Chris
On Fri, Aug 31, 2018 at 10:21:33AM -0700, will sanfilippo wrote:
> I do not think a driver requires an os device personally. You could write a driver that does not have os dev functionality in it. I do agree that it seems most people implement drivers with os dev functionality but I do not think it is a requirement.
>
>
> > On Aug 31, 2018, at 9:51 AM, Christopher Collins <ch...@runtime.io> wrote:
> >
> > I do think that approach makes more sense than implementing retries in
> > the HAL. By the way, I did submit a PR for performing I2C retries in
> > the HAL:
> > https://github.com/apache/mynewt-core/pull/1373#issue-212250961
> >
> > I am happy to redo this at a higher layer, though. If nothing else, it
> > gives us two implementations to compare.
> >
> > I am a bit confused by the term "driver," though. I could be wrong, but
> > my impression is that a Mynewt driver necessarily involves the use of
> > `os_dev` objects. That is not how I would envision such an I2C layer to
> > look. Specifically, I am thinking of a thin library that offers two
> > functions (names simplified):
> >
> > int i2c_read(uint8_t i2c_num, struct hal_i2c_master_data *pdata,
> > uint32_t timeout, uint8_t last_op, int retries);
> >
> > int i2c_write(uint8_t i2c_num, struct hal_i2c_master_data *pdata,
> > uint32_t timeout, uint8_t last_op, int retries);
> >
> > These functions would just repeatedly call the HAL I2C functions until
> > they succeed, or until all retries are exhausted. Is anything else
> > needed?
> >
> > Chris
> >
> > On Fri, Aug 31, 2018 at 05:58:39PM +0200, Sterling Hughes wrote:
> >> I agree with this as well. For awhile now, I think there has been a
> >> need for an I2C driver layer, which also can manage bus arbitration.
> >> Retries is another good one to add to that.
> >>
> >> Sterling
> >>
> >> On 31 Aug 2018, at 17:32, Andrzej Kaczmarek wrote:
> >>
> >>> Hi all,
> >>>
> >>> Seems like the majority already voted to go for retries in HAL, but
> >>> I'll add my opinion anyway: I do not think HAL is proper place to
> >>> include such logic.
> >>> I think HAL should only provide interfaces to hide differences between
> >>> underlying MCU hardware while read/write retry logic is something
> >>> driver should do (or any caller in general) since it can decide when
> >>> and if it makes sense to retry. This is what common error codes (so
> >>> #1) would allow to do. This does not however mean we can't have common
> >>> retry code. What we likely need is some bus-level driver/layer that
> >>> drivers will use instead of HAL directly so it can take care of
> >>> handling multiple devices on the same bus seamlessly (in general) and
> >>> also it would be the place where we can put extra common logic if
> >>> needed (like retries).
> >>>
> >>> Best,
> >>> Andrzej
> >>>
> >>>
> >>>
> >>> On Thu, Aug 30, 2018 at 3:59 AM 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.
> >>>> 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
>
Re: I2C retries
Posted by will sanfilippo <wi...@runtime.io>.
I do not think a driver requires an os device personally. You could write a driver that does not have os dev functionality in it. I do agree that it seems most people implement drivers with os dev functionality but I do not think it is a requirement.
> On Aug 31, 2018, at 9:51 AM, Christopher Collins <ch...@runtime.io> wrote:
>
> I do think that approach makes more sense than implementing retries in
> the HAL. By the way, I did submit a PR for performing I2C retries in
> the HAL:
> https://github.com/apache/mynewt-core/pull/1373#issue-212250961
>
> I am happy to redo this at a higher layer, though. If nothing else, it
> gives us two implementations to compare.
>
> I am a bit confused by the term "driver," though. I could be wrong, but
> my impression is that a Mynewt driver necessarily involves the use of
> `os_dev` objects. That is not how I would envision such an I2C layer to
> look. Specifically, I am thinking of a thin library that offers two
> functions (names simplified):
>
> int i2c_read(uint8_t i2c_num, struct hal_i2c_master_data *pdata,
> uint32_t timeout, uint8_t last_op, int retries);
>
> int i2c_write(uint8_t i2c_num, struct hal_i2c_master_data *pdata,
> uint32_t timeout, uint8_t last_op, int retries);
>
> These functions would just repeatedly call the HAL I2C functions until
> they succeed, or until all retries are exhausted. Is anything else
> needed?
>
> Chris
>
> On Fri, Aug 31, 2018 at 05:58:39PM +0200, Sterling Hughes wrote:
>> I agree with this as well. For awhile now, I think there has been a
>> need for an I2C driver layer, which also can manage bus arbitration.
>> Retries is another good one to add to that.
>>
>> Sterling
>>
>> On 31 Aug 2018, at 17:32, Andrzej Kaczmarek wrote:
>>
>>> Hi all,
>>>
>>> Seems like the majority already voted to go for retries in HAL, but
>>> I'll add my opinion anyway: I do not think HAL is proper place to
>>> include such logic.
>>> I think HAL should only provide interfaces to hide differences between
>>> underlying MCU hardware while read/write retry logic is something
>>> driver should do (or any caller in general) since it can decide when
>>> and if it makes sense to retry. This is what common error codes (so
>>> #1) would allow to do. This does not however mean we can't have common
>>> retry code. What we likely need is some bus-level driver/layer that
>>> drivers will use instead of HAL directly so it can take care of
>>> handling multiple devices on the same bus seamlessly (in general) and
>>> also it would be the place where we can put extra common logic if
>>> needed (like retries).
>>>
>>> Best,
>>> Andrzej
>>>
>>>
>>>
>>> On Thu, Aug 30, 2018 at 3:59 AM 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.
>>>> 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
Re: I2C retries
Posted by Christopher Collins <ch...@runtime.io>.
I do think that approach makes more sense than implementing retries in
the HAL. By the way, I did submit a PR for performing I2C retries in
the HAL:
https://github.com/apache/mynewt-core/pull/1373#issue-212250961
I am happy to redo this at a higher layer, though. If nothing else, it
gives us two implementations to compare.
I am a bit confused by the term "driver," though. I could be wrong, but
my impression is that a Mynewt driver necessarily involves the use of
`os_dev` objects. That is not how I would envision such an I2C layer to
look. Specifically, I am thinking of a thin library that offers two
functions (names simplified):
int i2c_read(uint8_t i2c_num, struct hal_i2c_master_data *pdata,
uint32_t timeout, uint8_t last_op, int retries);
int i2c_write(uint8_t i2c_num, struct hal_i2c_master_data *pdata,
uint32_t timeout, uint8_t last_op, int retries);
These functions would just repeatedly call the HAL I2C functions until
they succeed, or until all retries are exhausted. Is anything else
needed?
Chris
On Fri, Aug 31, 2018 at 05:58:39PM +0200, Sterling Hughes wrote:
> I agree with this as well. For awhile now, I think there has been a
> need for an I2C driver layer, which also can manage bus arbitration.
> Retries is another good one to add to that.
>
> Sterling
>
> On 31 Aug 2018, at 17:32, Andrzej Kaczmarek wrote:
>
> > Hi all,
> >
> > Seems like the majority already voted to go for retries in HAL, but
> > I'll add my opinion anyway: I do not think HAL is proper place to
> > include such logic.
> > I think HAL should only provide interfaces to hide differences between
> > underlying MCU hardware while read/write retry logic is something
> > driver should do (or any caller in general) since it can decide when
> > and if it makes sense to retry. This is what common error codes (so
> > #1) would allow to do. This does not however mean we can't have common
> > retry code. What we likely need is some bus-level driver/layer that
> > drivers will use instead of HAL directly so it can take care of
> > handling multiple devices on the same bus seamlessly (in general) and
> > also it would be the place where we can put extra common logic if
> > needed (like retries).
> >
> > Best,
> > Andrzej
> >
> >
> >
> > On Thu, Aug 30, 2018 at 3:59 AM 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.
> >> 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
Re: I2C retries
Posted by Sterling Hughes <st...@gmail.com>.
I agree with this as well. For awhile now, I think there has been a
need for an I2C driver layer, which also can manage bus arbitration.
Retries is another good one to add to that.
Sterling
On 31 Aug 2018, at 17:32, Andrzej Kaczmarek wrote:
> Hi all,
>
> Seems like the majority already voted to go for retries in HAL, but
> I'll add my opinion anyway: I do not think HAL is proper place to
> include such logic.
> I think HAL should only provide interfaces to hide differences between
> underlying MCU hardware while read/write retry logic is something
> driver should do (or any caller in general) since it can decide when
> and if it makes sense to retry. This is what common error codes (so
> #1) would allow to do. This does not however mean we can't have common
> retry code. What we likely need is some bus-level driver/layer that
> drivers will use instead of HAL directly so it can take care of
> handling multiple devices on the same bus seamlessly (in general) and
> also it would be the place where we can put extra common logic if
> needed (like retries).
>
> Best,
> Andrzej
>
>
>
> On Thu, Aug 30, 2018 at 3:59 AM 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.
>> 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
Re: I2C retries
Posted by Andrzej Kaczmarek <an...@codecoup.pl>.
Hi all,
Seems like the majority already voted to go for retries in HAL, but
I'll add my opinion anyway: I do not think HAL is proper place to
include such logic.
I think HAL should only provide interfaces to hide differences between
underlying MCU hardware while read/write retry logic is something
driver should do (or any caller in general) since it can decide when
and if it makes sense to retry. This is what common error codes (so
#1) would allow to do. This does not however mean we can't have common
retry code. What we likely need is some bus-level driver/layer that
drivers will use instead of HAL directly so it can take care of
handling multiple devices on the same bus seamlessly (in general) and
also it would be the place where we can put extra common logic if
needed (like retries).
Best,
Andrzej
On Thu, Aug 30, 2018 at 3:59 AM 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.
> 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
Re: I2C retries
Posted by Jacob Rosenthal <ja...@gmail.com>.
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 <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.
> 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
>