drivers/periph: reworked I2C driver #4926

haukepetersen · 2016-02-29T09:55:56Z

Analog to #4780: This PR brings the I2C driver up-to-date with our current discussions. But as in #4780, we need to agree on this concept until it makes sense to touch the existing implementations...

As analyzed in #4758: the i2c peripheral is a shared bus allowing atomic access, implying that it needs to be acquired/released before/after usage and should be powered off when not in use.

Main changes:

removed read_byte and read_reg
moved bus speed parameter to i2c_acquire
made parameters overridable
added fixed return codes
added default type
removed (unused) slave mode (-> should go into its own interface)

TODO:

doxygen needs to be updated, e.g. describe that devices are powered on/off when calling acquire/release and more
adapt all existing implementations

jnohlgard · 2016-02-29T10:31:43Z

Related: #3366

jnohlgard · 2016-02-29T10:35:32Z

drivers/include/periph/i2c.h

 *
 * @param[in]  dev          I2C peripheral device
- * @param[in]  address      bus address of the target device
+ * @param[in]  addr         7-bit device address (right-aligned)


👍 good clarification

lebrush · 2016-03-02T12:50:17Z

drivers/include/periph/i2c.h

@@ -140,147 +145,110 @@ int i2c_init_master(i2c_t dev, i2c_speed_t speed);
 * @param[in] dev       I2C device to access
 *


missing @param[in] speed. What's the purpose of adding the speed to the i2c_acquire?

as the bus is shared, we have the situation that multiple devices use the bus with different clock speeds. By moving the clock selection into the acquire function, this is now possible.

Nice! good catch! 👍

haukepetersen · 2016-03-03T13:15:16Z

So is everyone ok with these interface changes? Then I would to and adapt the existing implementations so we can test them and bring this into the release...

@gebart, @PeterKietzmann, @kYc0o, @jfischer-phytec-iot, @lebrush, @LudwigKnuepfer: ping.

kYc0o · 2016-03-03T13:20:37Z

I'm OK with the changes, it adds flexibility and the code it's really understandable. It will break all the drivers using i2c right?

jnohlgard · 2016-03-03T14:47:04Z

I would say that moving the speed to the acquire function is even more signs that it would make sense to introduce a state struct for keeping the hardware settings between calls. Are the periph drivers expected to precompute all possible speed settings inside the init function and then use a lookup to find the right set of register values depending on the speed setting when a driver acquires the bus?

jnohlgard · 2016-03-04T05:27:09Z

It may not be the best idea to mix speeds on the i2c bus since it does not provide a discrete Chip Select signal.. The slowest devices may undersample the bus and read the aliased signal as valid data and try to reply.

Can we keep the bus speed selection in the init function?

See: http://electronics.stackexchange.com/questions/62365/is-i2c-mixed-frequency-possible

PeterKietzmann · 2016-03-07T08:55:11Z

I agree with @gebart that mixed bus speeds could be dangerous for i2s slaves as there's no dedicated CS lines like in SPI. Though I never tested a setup like this... Other than that, your proposal looks good to me.

LudwigKnuepfer · 2016-03-07T18:04:30Z

I don't see the valid (!) I²C answer NACK defined as a return value. Also it's a bit vague what address/data error could mean.
Edit: "answer" as in no answer ;)

haukepetersen · 2016-03-08T09:05:03Z

@gebart: you are completely right, using different bus speeds for different devices is not an option - I don't really know what I was thinking there...

So I have a new, even simpler proposal: why don't we just specify the speed valued in the periph_conf.h? So one as developer must just think about the devices that are used on the bus and has to specify the speed value globally for all of them. This value can easily adjusted if needed. The device drivers only need to specify there allowed speed values in their API. In their initialization we test the correct connection to a device anyway, and if chosen an invalid speed value, the init function of the device will return with an error in any case.

haukepetersen · 2016-03-08T09:09:03Z

@LudwigKnuepfer: not sure I understand your comment, why do you want to have NACK as return value?

We can group errors in two major groups: errors during addressing and errors during the actual data transfer -> hence I2C_ERR_ADDR and I2C_ERR_DATA. For drivers using the i2c interface, these two error conditions should be enough. Also this abstraction of error conditions is needed, as different MCUs have different (internal) error conditions, and it does not makes sense to export them all through this API.

LudwigKnuepfer · 2016-03-08T20:21:54Z

@LudwigKnuepfer: not sure I understand your comment, why do you want to have NACK as return value?

In case the slave does not reply with an ACK (replies with a NACK), this is not an error per se.
The driver might need to handle the absence of an ACK differently from "errors". Whatever they may be.

We can group errors in two major groups: errors during addressing and errors during the actual data transfer -> hence I2C_ERR_ADDR and I2C_ERR_DATA. For drivers using the i2c interface, these two error conditions should be enough. Also this abstraction of error conditions is needed, as different MCUs have different (internal) error conditions, and it does not makes sense to export them all through this API.

I still don't know what qualifies as a data/address error.

For example:
If no slave acknowledges the address given during START - is that an address error?
Or is it only an error if a hardware problem is detected?

In my understanding of I2C a NACK is a valid communication pattern, not an error.

haukepetersen · 2016-03-10T08:37:23Z

If no slave acknowledges the address given during START - is that an address error?

this would be an I2C_ERR_ADDR. Naming this an error depends on how we define error in this context, we might also name it exception if that suits you better - I absolutely dont care.

LudwigKnuepfer · 2016-03-10T17:43:19Z

IC, thank you for the clarification. As a NACK is a well-defined and named part of regular communication I would not call it error or an exception, but a NACK.

haukepetersen · 2016-03-11T10:58:28Z

The problem I see with NACK is, that it is very specific, but there could be, dependent on the hardware, also other error cases. Having a more generic return value would make the interface easier to implement.

DipSwitch · 2016-03-11T17:18:15Z

cpu/stm32f4/include/periph_cpu.h

+    i2c_speed_t speed;      /**< I2C bus speed */
+    uint8_t rcc_bit;        /**< bit in the rcc register */
+    uint8_t irqn;           /**< IRQ channel */
+}


Incomplete code ;)

} i2c_conf_t; /** @} */

I know :-) Will fix as soon as we decide for the interface changes

LudwigKnuepfer · 2016-03-11T17:59:19Z

dependent on the hardware, also other error cases

That's what I thought - so define ACK, NACK and a generic other error case.

haukepetersen · 2016-03-14T12:17:04Z

Ok, again: just returning NACK makes no sense, was the NACK on address, on data byte. I say that one exception return value for the address byte and one for data bytes fully sufficient for device drivers that build on top of the interface.

LudwigKnuepfer · 2016-03-14T17:39:26Z

I agree that the two types of NACK should be differentiated.

haukepetersen · 2016-03-14T21:03:06Z

@LudwigKnuepfer: how does this look now?

ant9000 · 2016-12-13T12:20:26Z

@haukepetersen , thanks for your feedback.

I've published a PR with my code as #6205 - the code should merge cleanly, but needs to be reviewed.

In the future, I would happily participate in remodeling the implementation - even if my contribution is indeed minimal.

haukepetersen · 2016-12-13T13:03:03Z

@ant9000 great! Can't promise anything but I will try to test your PR this week. And just keep a watch on this PR so you can jump in when we move on - your help is much appreciated!

ant9000 · 2016-12-14T08:34:51Z

@haukepetersen, glad to be of help - count me in for the future.

Hope that my PR can be useful, waiting for a tip from you when you get the time for it.

Antonio

smlng · 2018-01-12T09:47:15Z

postponed, due to high impact changes

miri64 · 2018-02-15T16:17:32Z

No significant progress for over a year and it seems @haukepetersen gave up on this task. Closing as memo for now.

PeterKietzmann · 2018-02-15T17:25:24Z

@miri64 I tend to reopen this one. Even though @haukepetersen might have "given up on this task" it is still on our TODO list. This PR acts as place to showcase and discuss the actual API proposal. Objections?

miri64 · 2018-02-15T17:51:30Z

@miri64 I tend to reopen this one. Even though @haukepetersen might have "given up on this task" it is still on our TODO list. This PR acts as place to showcase and discuss the actual API proposal. Objections?

Mh.. I'd rather prefer that you open an issue for that. Having an open (and stale) PR that serves as a show-case seems like a bad idea.

haukepetersen · 2018-02-26T08:56:38Z

I know this has been stale for too long, but IMHO the proposed interface changes are still valid and the base for the (at some time) coming remodeling. So in this case I disagree with closing this PR!

haukepetersen · 2018-02-26T09:09:52Z

sorry, I mixed up PR numbers. #6576 is the one that should remain open, so I agree with closing this PR.

haukepetersen assigned jnohlgard Feb 29, 2016

haukepetersen added this to the Release 2016.03 milestone Feb 29, 2016

jnohlgard reviewed Feb 29, 2016
View reviewed changes

haukepetersen mentioned this pull request Feb 29, 2016

periph/i2c: handle NACK #3366

Closed

lebrush reviewed Mar 2, 2016
View reviewed changes

haukepetersen force-pushed the opt_periph_i2c2 branch from b981dae to b286cd8 Compare March 8, 2016 09:00

DipSwitch reviewed Mar 11, 2016
View reviewed changes

DipSwitch mentioned this pull request Mar 11, 2016

board: EMW3162 #4498

Closed

haukepetersen force-pushed the opt_periph_i2c2 branch from b286cd8 to 53adfc5 Compare March 14, 2016 21:02

PeterKietzmann modified the milestones: Release 2017.04, Release 2017.01 Jan 13, 2017

haukepetersen mentioned this pull request Jan 30, 2017

cpu/drivers: remove deprecated lm75a+lpc2387 I2C drivers #6521

Merged

This was referenced Feb 8, 2017

periph/i2c: remodeling part 1: deprecate existing API #6575

Closed

periph/i2c: remodeling part 2: introduce new API #6576

Closed

kaspar030 modified the milestones: Release 2017.04, Release 2017.07 Apr 21, 2017

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng modified the milestones: Release 2018.01, Release 2018.04 Jan 12, 2018

miri64 closed this Feb 15, 2018

miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Feb 15, 2018

haukepetersen reopened this Feb 26, 2018

haukepetersen closed this Feb 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/periph: reworked I2C driver #4926

drivers/periph: reworked I2C driver #4926

haukepetersen commented Feb 29, 2016

jnohlgard commented Feb 29, 2016

jnohlgard Feb 29, 2016

lebrush Mar 2, 2016

haukepetersen Mar 2, 2016

lebrush Mar 2, 2016

haukepetersen commented Mar 3, 2016

kYc0o commented Mar 3, 2016

jnohlgard commented Mar 3, 2016

jnohlgard commented Mar 4, 2016

PeterKietzmann commented Mar 7, 2016

LudwigKnuepfer commented Mar 7, 2016

haukepetersen commented Mar 8, 2016

haukepetersen commented Mar 8, 2016

LudwigKnuepfer commented Mar 8, 2016

haukepetersen commented Mar 10, 2016

LudwigKnuepfer commented Mar 10, 2016

haukepetersen commented Mar 11, 2016

DipSwitch Mar 11, 2016

haukepetersen Mar 14, 2016

LudwigKnuepfer commented Mar 11, 2016

haukepetersen commented Mar 14, 2016

LudwigKnuepfer commented Mar 14, 2016

haukepetersen commented Mar 14, 2016

ant9000 commented Dec 13, 2016

haukepetersen commented Dec 13, 2016

ant9000 commented Dec 14, 2016

smlng commented Jan 12, 2018

miri64 commented Feb 15, 2018

PeterKietzmann commented Feb 15, 2018

miri64 commented Feb 15, 2018

haukepetersen commented Feb 26, 2018

haukepetersen commented Feb 26, 2018

		@@ -140,147 +145,110 @@ int i2c_init_master(i2c_t dev, i2c_speed_t speed);
		* @param[in] dev I2C device to access
		*

drivers/periph: reworked I2C driver #4926

drivers/periph: reworked I2C driver #4926

Conversation

haukepetersen commented Feb 29, 2016

jnohlgard commented Feb 29, 2016

jnohlgard Feb 29, 2016

Choose a reason for hiding this comment

lebrush Mar 2, 2016

Choose a reason for hiding this comment

haukepetersen Mar 2, 2016

Choose a reason for hiding this comment

lebrush Mar 2, 2016

Choose a reason for hiding this comment

haukepetersen commented Mar 3, 2016

kYc0o commented Mar 3, 2016

jnohlgard commented Mar 3, 2016

jnohlgard commented Mar 4, 2016

PeterKietzmann commented Mar 7, 2016

LudwigKnuepfer commented Mar 7, 2016

haukepetersen commented Mar 8, 2016

haukepetersen commented Mar 8, 2016

LudwigKnuepfer commented Mar 8, 2016

haukepetersen commented Mar 10, 2016

LudwigKnuepfer commented Mar 10, 2016

haukepetersen commented Mar 11, 2016

DipSwitch Mar 11, 2016

Choose a reason for hiding this comment

haukepetersen Mar 14, 2016

Choose a reason for hiding this comment

LudwigKnuepfer commented Mar 11, 2016

haukepetersen commented Mar 14, 2016

LudwigKnuepfer commented Mar 14, 2016

haukepetersen commented Mar 14, 2016

ant9000 commented Dec 13, 2016

haukepetersen commented Dec 13, 2016

ant9000 commented Dec 14, 2016

smlng commented Jan 12, 2018

miri64 commented Feb 15, 2018

PeterKietzmann commented Feb 15, 2018

miri64 commented Feb 15, 2018

haukepetersen commented Feb 26, 2018

haukepetersen commented Feb 26, 2018