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
