I2C driver implementation (Master mode) for the SAMR21 #2016

biboc · 2014-11-15T16:56:43Z

Implementation of the I2C driver (Master mode) for the SAMR21 including:

periph_conf.h: I2C configuration added
i2c.c: I2C_0, implementation of all functions but slave mode

I do some "return;" when I detect an error which is not that good. Which solution do I have?

biboc · 2014-11-17T08:29:34Z

@thomaseichinger , Why did Travis CI build fail?

miri64 · 2014-11-17T08:53:59Z

@bapclenet cppcheck found some issues:

cpu/samd21/periph/i2c.c:52: style (unassignedVariable): Variable 'pin_scl' is not assigned a value.
cpu/samd21/periph/i2c.c:53: style (unassignedVariable): Variable 'pin_sda' is not assigned a value.
cpu/samd21/periph/i2c.c:55: style (unassignedVariable): Variable 'clock_source_speed' is not assigned a value.

They are triggered since the variables in question are only assigned in the #if I2C_0_EN build path.

thomaseichinger · 2014-11-17T08:54:39Z

Travis fails because cppcheck complains.

cpu/samd21/periph/i2c.c:52: style (unassignedVariable): Variable 'pin_scl' is not assigned a value.
cpu/samd21/periph/i2c.c:53: style (unassignedVariable): Variable 'pin_sda' is not assigned a value.
cpu/samd21/periph/i2c.c:55: style (unassignedVariable): Variable 'clock_source_speed' is not assigned a value.

Cppcheck isn't smart regarding preprocessor defines and and if statements. You could try initializing with 0.

thomaseichinger · 2014-11-17T08:56:02Z

boards/samr21-xpro/include/periph_conf.h

+#define I2C_SDA    	  PIN_PA16
+#define I2C_SCL	   	  PIN_PA17
+#define I2C_0_PINS    (PORT_PA16 | PORT_PA17)
+#define I2C_0_REF_F	  (8000000UL) // Default Clock Source on reset OSC8M - 8MHz 


Please align the defines and use /* C-style comments */

miri64 · 2014-11-17T08:58:22Z

OT: cppcheck IS smart regarding preprocessor defines, checking every possible build path ;-). If you leave a variable unset in a possible build path it could lead to errors; if you do it intentionally you should very carefully reason why.

thomaseichinger · 2014-11-17T08:59:46Z

cpu/samd21/periph/i2c.c

+			port_group = &I2C_0_PORT;
+			pin_scl = I2C_SDA;
+			pin_sda = I2C_SCL;
+			i2c_port = I2C_0_PINS;


assigning I2C_0_PINS to i2c_port is quite confusing. Maybe rename s/i2c_port/i2c_pins/ ?

thomaseichinger · 2014-11-17T09:18:01Z

@bapclenet Awesome work! Some first style comments above and please change to spaces instead of tabs for indentation. Will test it in the afternoon.

thomaseichinger · 2014-11-20T10:54:23Z

@bapclenet Please rebase. Also you still have some tabs in your code. https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#indentation-and-braces

biboc · 2014-11-20T12:56:39Z

How do I rebase?
I did "git rebase" in my master branch but I don't see any differences.

BytesGalore · 2014-11-20T13:18:17Z

@bapclenet sorry that was wrong.
try git fetch upstream && git rebase upstream/master in master,
then change to this branch and use git rebase master

biboc · 2014-11-20T13:41:21Z

Rebase done, thank you @BytesGalore!

thomaseichinger · 2014-11-21T10:44:13Z

Travis failing is unrelated to this PR. Will be solved by #2064.

wiredsource · 2014-11-28T07:28:28Z

@bapclenet

26 files changed...? This is not only a I2C PR anymore :-)

biboc · 2014-11-28T07:32:23Z

A problem occured with my rebase apparently...
I will repair that this afternoon

I don't know how I did that.
How can I repair it?
Should I create another pull request?

thomaseichinger · 2014-12-10T10:48:17Z

@haukepetersen could you take another look?

haukepetersen · 2014-12-11T13:34:21Z

code looks good to me, though I don't have an I2C device here for testing. I also kicked Travis once more.

wiredsource · 2014-12-11T13:50:07Z

I have had a logic analyzer connected to it and it seems all right...
I was trying to implement the SHT21 driver without success... But that the SHT21 drivers fault not this PR.

PeterKietzmann · 2014-12-12T07:34:18Z

I will test it with one of the ultrasonic sensors

PeterKietzmann · 2014-12-12T09:19:19Z

I tested this PR with #1998 and #2179. I2C seems to work but for this test if get STATUS_ERR_TIMEOUT at any point. On stm32f4discovery this test ran really stable

biboc · 2014-12-12T09:25:54Z

How do you test it ? Which test do you use ? I may have a look this
week-end to solve the problem.
You always receive STATUS ERR TIMEOUT ?
Le 12 déc. 2014 10:19, "Peter Kietzmann" [email protected] a écrit
:

I tested this PR with #1998 #1998
and #2179 #2179. I2C seems to work
but for this test if get STATUS_ERR_TIMEOUT at any point. On
stm32f4discovery this test ran really stable

—
Reply to this email directly or view it on GitHub
#2016 (comment).

biboc · 2014-12-12T09:57:51Z

Two other questions :
Which frequency do you use ?
Are you sure about your address device ? STATUS ERROR TIME OUT mainly occurs when the I2C device doesn't answer so either the address is wrong or the speed or something else.

PeterKietzmann · 2014-12-12T10:22:43Z

Hi! In #2179 is a test for the srf08 supersonic ranger which is an i2c device. The address is correct as the same application runs stable on stm32f4discovery. Also it runs stable for a while on samr21-xpro but then it gets stuck and one can see the debug message STATUS_ERR_TIMEOUT. I'm using I2C_SPEED_NORMAL which should lead to ~100kbps. I'll try another i2c ranger

PeterKietzmann · 2014-12-12T10:50:59Z

I tested this with the srf02 ultrasonic ranger and it seems to work stable. I assume i2c is fine. Do you have another i2c device to test with?

PeterKietzmann · 2014-12-12T12:28:17Z

Ok just do clarify the situation a bit: I tested this i2c implementation with tests/drivers_srf02 and tests/drivers_srf08 (PR #2179). For both sensors the i2c-based device drivers do work. But when testing the srf08 ranger, I got the problems described above, when I have a logic analyzer in parallel. These problems didn't occur when testing with srf02. However, I tried to reproduce this a several times and it occured again and again.

biboc · 2014-12-12T14:02:13Z

Ok, so basically, you try watch the I2C frame with the logic analyzer ?
And that works with the srf02 but not with the srf08 (and you fall every time into the debug error, in start, read or write function ? )
That's a it weird if it works for one and not another.
I use the SI7013 with an logic analyzer in parrallel and it works.
What do you want me to do then?

PeterKietzmann · 2014-12-12T14:11:03Z

Nothing more :-) ! I give my ACK for the functionality as this behavior is probably an electrical one. Good to here that is is also fine with your SI7013

biboc · 2014-12-13T10:10:59Z

Good to here that :-)
Thanks

What's wrong with Travis CI build?

PeterKietzmann · 2014-12-15T07:54:55Z

Code looks good to me and I also ACKed the functionality. As I am not too deep into that board, is someone else willing to click merge?

thomaseichinger · 2014-12-15T11:57:56Z

Kicked Travis, will merge when passed.

biboc · 2014-12-15T12:51:12Z

What should I change for Travis test to pass?
"
failed: riot_and_cpp, vtimer_msg_diff
"

thomaseichinger · 2014-12-15T15:57:53Z

@bapclenet Nothing, Travis seems to have some random fails. Built locally without errors so let's do this.

ACK & Go

cpu/samd21: I2C driver implementation (Master mode)

thomaseichinger added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT swp-telematic-sose14 and removed swp-telematic-sose14 labels Nov 16, 2014

thomaseichinger added this to the Release NEXT MAJOR milestone Nov 16, 2014

thomaseichinger self-assigned this Nov 16, 2014

thomaseichinger reviewed Nov 17, 2014
View reviewed changes

thomaseichinger added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 27, 2014

cpu/samd21: add i2c periph implementation

54fb0f0

thomaseichinger removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 11, 2014

PeterKietzmann mentioned this pull request Dec 15, 2014

drivers/srf08: improved and adapted ultrasonic ranger driver to general i2c interface #2179

Merged

thomaseichinger added a commit that referenced this pull request Dec 15, 2014

Merge pull request #2016 from bapclenet/master

29c5822

cpu/samd21: I2C driver implementation (Master mode)

thomaseichinger merged commit 29c5822 into RIOT-OS:master Dec 15, 2014

I2C driver implementation (Master mode) for the SAMR21 #2016

I2C driver implementation (Master mode) for the SAMR21 #2016

Uh oh!

Conversation

biboc commented Nov 15, 2014

Uh oh!

biboc commented Nov 17, 2014

Uh oh!

miri64 commented Nov 17, 2014

Uh oh!

thomaseichinger commented Nov 17, 2014

Uh oh!

thomaseichinger Nov 17, 2014

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 17, 2014

Uh oh!

thomaseichinger Nov 17, 2014

Choose a reason for hiding this comment

Uh oh!

thomaseichinger commented Nov 17, 2014

Uh oh!

thomaseichinger commented Nov 20, 2014

Uh oh!

biboc commented Nov 20, 2014

Uh oh!

BytesGalore commented Nov 20, 2014

Uh oh!

biboc commented Nov 20, 2014

Uh oh!

thomaseichinger commented Nov 21, 2014

Uh oh!

wiredsource commented Nov 28, 2014

Uh oh!

biboc commented Nov 28, 2014

Uh oh!

thomaseichinger commented Dec 10, 2014

Uh oh!

haukepetersen commented Dec 11, 2014

Uh oh!

wiredsource commented Dec 11, 2014

Uh oh!

PeterKietzmann commented Dec 12, 2014

Uh oh!

PeterKietzmann commented Dec 12, 2014

Uh oh!

biboc commented Dec 12, 2014

Uh oh!

biboc commented Dec 12, 2014

Uh oh!

PeterKietzmann commented Dec 12, 2014

Uh oh!

PeterKietzmann commented Dec 12, 2014

Uh oh!

PeterKietzmann commented Dec 12, 2014

Uh oh!

biboc commented Dec 12, 2014

Uh oh!

PeterKietzmann commented Dec 12, 2014

Uh oh!

biboc commented Dec 13, 2014

Uh oh!

PeterKietzmann commented Dec 15, 2014

Uh oh!

thomaseichinger commented Dec 15, 2014

Uh oh!

biboc commented Dec 15, 2014

Uh oh!

thomaseichinger commented Dec 15, 2014

Uh oh!

Uh oh!