Re: incubator-mynewt-core git commit: I2C interface supporting blocking read and write

[Greg Stein <gs...@gmail.com>]

Heya Paul,

I do a *ton* of I2C. I've written PIC code for both master and slave
operation, actually. This stuff has been loaded into my cortex for the past
couple years. I've got some feedback below ...

On Tue, Apr 19, 2016 at 10:49 PM, <pa...@apache.org> wrote:
>...

> +++ b/hw/hal/include/hal/hal_i2c.h
>
>...

> +#ifndef HAL_I2C_H
> +#define HAL_I2C_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <inttypes.h>
> +#include <bsp/bsp_sysid.h>
>

#include should not occur within the __cplusplus guard. This might
(incorrectly) apply the extern "C" inappropriately to those headers.


> +
> +/* This is the API for an i2c bus.  I tried to make this a simple API */
> +
> +struct hal_i2c;
> +
> +/* when sending a packet, use this structure to pass the arguments */
> +struct hal_i2c_master_data {
> +    uint8_t  address;   /* destination address */
> +            /* NOTE: Write addresses are even and read addresses are odd
> +             * but in this API, you must enter a single address that is
> the
> +             * write address divide by 2.  For example, for address
> +             * 80, you would enter a 40 */
>

More precisely: the address is a 7-bit integer. The I2C layer will
correctly insert the read/write flag, so it should not be placed within the
"address".


> +    uint8_t *buffer;    /* buffer space to hold the transmit or receive */
> +    uint16_t len;       /* length of buffer to transmit or receive */
>

uint16 seems awfully large. Are there I2C devices that regularly move more
than 255 bytes?


> +};
> +
> +/* Initialize a new i2c device with the given system id.
> + * Returns negative on error, 0 on success. */
> +struct hal_i2c*
> +hal_i2c_init(enum system_device_id sysid);
>

The comment is incorrect, as this function returns a pointer.


> +
> +/* Sends <len> bytes of data on the i2c.  This API assumes that you
> + * issued a successful start condition.  It will fail if you
> + * have not. This API does NOT issue a stop condition.  You must stop the
> + * bus after successful or unsuccessful write attempts.  Returns 0 on
> + * success, negative on failure */
> +int
> +hal_i2c_master_write(struct hal_i2c*, struct hal_i2c_master_data *pdata);
>

I'd like to see some kind of detail on the possible error return values.
Specifically to resolve whether bus arbitration was lost, or whether the
address was never ACK'd.

Further, the device might not allow all bytes to be written to it. It might
NAK after the first byte, where the app intended a 10-byte write.

Using the _data structures seems to complicate this API. Couldn't you just
include 3 parameters instead of 1 struct?

Can I also assume this call will perform clock-stretching on the bus until
I decide to issue a repeated-start or a stop condition?

For that matter, what happens if the slave performs clock stretching before
issuing a ACK/NAK ? ... does this API block? Can I set a timeout?

+
> +/* Reads <len> bytes of data on the i2c.  This API assumes that you
> + * issued a successful start condition.  It will fail if you
> + * have not. This API does NOT issue a stop condition.  You must stop the
> + * bus after successful or unsuccessful write attempts.  Returns 0 on
>

"read attempts"


> + * success, negative on failure */
> +int
> +hal_i2c_master_read(struct hal_i2c*, struct hal_i2c_master_data *pdata);
>

This needs to somehow return how many bytes were actually read. And on that
matter, how long should/will the master wait between its ACK to the slave,
and *not* seeing the slave transmit the next byte? ie. the slave is
refusing to transfer more bytes. Is that wait time configurable?

Same argument feedback: how about 3 args rather than 1 struct arg?

Same request for detail on the error condition to resolve what happened.

Does this API allow the slave to block the master via per-bit clock
stretching? Or via stretching between delivered bytes? Thus, does this API
block? Can I set a timeout?

+
> +/* issues a start condition and address on the SPI bus. Returns 0
> + * on success, negative on error.
> + */
> +int
> +hal_i2c_master_start(struct hal_i2c*);
>

The comment is wrong, as no address is provided here.

Do I use this API to issue a repeated start? For example:

start()
write()
start()
read()
stop()

Second question: does start() block until the bus is acquired? ie. what
happens if I2C bus is busy when I make this call? Do I get an error code
saying "busy", or does it block?

This API must leave the bus "held" by keeping SCL low. What mechanism(s)
are available to release the bus in case of an app failure? I2C busses
sometimes get locked up because some device is holding SCL low, when it
shouldn't.

+
> +/* issues a stop condition on the bus.  You must issue a stop condition
> for
> + * every successful start condition on the bus */
> +int
> +hal_i2c_master_stop(struct hal_i2c*);
>

This comment would imply my above repeated start pattern is inappropriate.
So if that is true, then how do I do a repeated start? (eg. the MPR121
requires repeated starts to read registers, so I'm gonna push for some way
to do them :-) )


> +
> +
> +/* Probes the i2c bus for a device with this address.  THIS API
> + * issues a start condition, probes the address and issues a stop
> + * condition.
> + */
> +int
> +hal_i2c_master_probe(struct hal_i2c*, uint8_t address);
>

Woah. What exactly is a "probe"?? Are you simply looking for a device to
ACK the specified 7-bit address?

I'm not sure this will always work, because you don't know what to use for
the R/W value. Some devices only read, some only write. The device might
not ACK if you supply an incorrect R/W.

>...

Is I2C slave operation a TBD? Could such a note be included in the API ?

>...

Cheers,
-g