You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by pa...@apache.org on 2016/04/20 05:49:15 UTC
incubator-mynewt-core git commit: I2C interface supporting blocking
read and write
Repository: incubator-mynewt-core
Updated Branches:
refs/heads/develop 9131d1e4c -> 598b91010
I2C interface supporting blocking read and write
changes to support start and stop conditions
Project: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/commit/598b9101
Tree: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/tree/598b9101
Diff: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/diff/598b9101
Branch: refs/heads/develop
Commit: 598b91010431f175394c03f34b7cd2429b66248e
Parents: 9131d1e
Author: Paul Dietrich <pa...@yahoo.com>
Authored: Thu Apr 14 16:55:12 2016 -0700
Committer: Paul Dietrich <pa...@yahoo.com>
Committed: Tue Apr 19 20:48:12 2016 -0700
----------------------------------------------------------------------
hw/hal/include/hal/hal_i2c.h | 89 +++++++++++++++++++++++++++++++++++
hw/hal/include/hal/hal_i2c_int.h | 54 +++++++++++++++++++++
hw/hal/src/hal_i2c.c | 78 ++++++++++++++++++++++++++++++
3 files changed, 221 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/598b9101/hw/hal/include/hal/hal_i2c.h
----------------------------------------------------------------------
diff --git a/hw/hal/include/hal/hal_i2c.h b/hw/hal/include/hal/hal_i2c.h
new file mode 100644
index 0000000..82546d4
--- /dev/null
+++ b/hw/hal/include/hal/hal_i2c.h
@@ -0,0 +1,89 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef HAL_I2C_H
+#define HAL_I2C_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <inttypes.h>
+#include <bsp/bsp_sysid.h>
+
+/* 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 */
+ uint8_t *buffer; /* buffer space to hold the transmit or receive */
+ uint16_t len; /* length of buffer to transmit or receive */
+};
+
+/* 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);
+
+/* 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);
+
+/* 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
+ * success, negative on failure */
+int
+hal_i2c_master_read(struct hal_i2c*, struct hal_i2c_master_data *pdata);
+
+/* 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*);
+
+/* 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*);
+
+
+/* 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);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* HAL_I2C_H */
http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/598b9101/hw/hal/include/hal/hal_i2c_int.h
----------------------------------------------------------------------
diff --git a/hw/hal/include/hal/hal_i2c_int.h b/hw/hal/include/hal/hal_i2c_int.h
new file mode 100644
index 0000000..59593e0
--- /dev/null
+++ b/hw/hal/include/hal/hal_i2c_int.h
@@ -0,0 +1,54 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+
+#ifndef HAL_I2C_INT_H
+#define HAL_I2C_INT_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <hal/hal_i2c.h>
+#include <inttypes.h>
+
+struct hal_i2c;
+
+struct hal_i2c_funcs {
+ int (*hi2cm_write_data) (struct hal_i2c *pi2c, struct hal_i2c_master_data *ppkt);
+ int (*hi2cm_read_data) (struct hal_i2c *pi2c, struct hal_i2c_master_data *ppkt);
+ int (*hi2cm_probe) (struct hal_i2c *pi2c, uint8_t address);
+ int (*hi2cm_start) (struct hal_i2c *pi2c);
+ int (*hi2cm_stop) (struct hal_i2c *pi2c);
+};
+
+struct hal_i2c
+{
+ const struct hal_i2c_funcs *driver_api;
+};
+
+struct hal_i2c *
+bsp_get_hal_i2c_driver(enum system_device_id sysid);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* HAL_I2C_INT_H */
+
http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core/blob/598b9101/hw/hal/src/hal_i2c.c
----------------------------------------------------------------------
diff --git a/hw/hal/src/hal_i2c.c b/hw/hal/src/hal_i2c.c
new file mode 100644
index 0000000..bd20620
--- /dev/null
+++ b/hw/hal/src/hal_i2c.c
@@ -0,0 +1,78 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include <bsp/bsp_sysid.h>
+#include <hal/hal_i2c.h>
+#include <hal/hal_i2c_int.h>
+
+struct hal_i2c *
+hal_i2c_init(enum system_device_id sysid)
+{
+ return bsp_get_hal_i2c_driver(sysid);
+}
+
+int
+hal_i2c_master_write(struct hal_i2c *pi2c, struct hal_i2c_master_data *ppkt)
+{
+ if (pi2c && pi2c->driver_api && pi2c->driver_api->hi2cm_write_data )
+ {
+ return pi2c->driver_api->hi2cm_write_data(pi2c, ppkt);
+ }
+ return -1;
+}
+
+int
+hal_i2c_master_read(struct hal_i2c *pi2c, struct hal_i2c_master_data *ppkt)
+{
+ if (pi2c && pi2c->driver_api && pi2c->driver_api->hi2cm_read_data )
+ {
+ return pi2c->driver_api->hi2cm_read_data(pi2c, ppkt);
+ }
+ return -1;
+}
+
+int
+hal_i2c_master_probe(struct hal_i2c *pi2c, uint8_t address)
+{
+ if (pi2c && pi2c->driver_api && pi2c->driver_api->hi2cm_probe )
+ {
+ return pi2c->driver_api->hi2cm_probe(pi2c, address);
+ }
+ return -1;
+}
+
+int
+hal_i2c_master_start(struct hal_i2c *pi2c)
+{
+ if (pi2c && pi2c->driver_api && pi2c->driver_api->hi2cm_start )
+ {
+ return pi2c->driver_api->hi2cm_start(pi2c);
+ }
+ return -1;
+}
+
+
+int
+hal_i2c_master_stop(struct hal_i2c *pi2c)
+{
+ if (pi2c && pi2c->driver_api && pi2c->driver_api->hi2cm_stop )
+ {
+ return pi2c->driver_api->hi2cm_stop(pi2c);
+ }
+ return -1;
+}
Re: incubator-mynewt-core git commit: I2C interface supporting
blocking read and write
Posted by 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