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
