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 >
