cpu/stm_common: Refactor and fix implementation for i2c_2 #10608

MrKevinWeiss · 2018-12-13T12:16:28Z

Contribution description

This PR fixes many bugs discovered by the HiL.

Refactors i2c_2 to match the structure of i2c_1 better.
Corrects functionality issues (such as nack recovery).
Allows the common implementation of read_regs and write_regs.
Documents constraints of hardware.
Reduces code size (about 180 bytes)
Matches error messages with API.

Testing procedure

Test on stm32f1 f2 f4 or l1 board
One can run the robot-test from the HiL branch or run term in tests/periph_i2c/
Or run manually against your favorite i2c device. No everything will be supported but everything should at least give correct error messages and not lock up the bus.
*If nucleo-f103rb is used it needs external pullups.

Basic read regs with 1,2,3 bytes
Basic write regs with 1,2,3 bytes
Test split frame (NOSTOP, NOSTOP|NOSTART, NOSTART) reads
Test split frame writes
Repeated start write (write_bytes+NOSTOP, write_bytes)
Getting an address nack then read a proper register.

Issues/PRs references

helps with some stm32 based boards for #3366
checks some boxes in #9518

maribu

 cpu/stm32_common/include/periph_cpu_common.h |   5 +
 cpu/stm32_common/periph/i2c_2.c              | 383 ++++++++++-----------------
 2 files changed, 143 insertions(+), 245 deletions(-)

Very nice! :-)

maribu · 2018-12-13T12:31:12Z

cpu/stm32_common/periph/i2c_2.c

    _i2c_init(i2c, i2c_config[dev].clk, ccr);

 #if defined(CPU_FAM_STM32F4)
    /* make sure the analog filters don't hang -> see errata sheet 2.14.7 */
    if (i2c->SR2 & I2C_SR2_BUSY) {
-        DEBUG("[i2c] init: line busy after reset, toggle pins now\n");


Why do you remove all the DEBUG() messages from i2c_init()? They don't produce any machine code unless ENABLE_DEBUG is set to true. So I don't see any harm in having them, and they could be extremely valuable when hunting a bug during I2C initialization.

This is because if you do enable debug it doesn't actually work and seems to screw up the shell, I think it has something to do with the uart being initialized after the i2c but I didn't really look into it. I wrote something above the enable debug to indicate that. It may only have been for the nucleo-f103rb though.

I think the problem is also there with F4 and the workaround could be, as you noticed, to initialize uart before i2c. That's what I did when debugging this driver some months ago.

It would be nice for debugging to have stdio working as soon as possible during boot. But that is unrelated to this PR. Keeping those DEBUG statements will bite the next person trying to debug I2C unless someone would change the boot up process to initialize stdio sooner. So I agree that removing them is the better alternative.

Noted, I opened an issue #10614 to document this concern. I also did write that comment explaining it, so hopefully it would be more a nibble than a bite.

maribu · 2018-12-13T12:46:23Z

cpu/stm32_common/periph/i2c_2.c

@@ -187,9 +187,6 @@ int i2c_release(i2c_t dev)
 {
    assert(dev < I2C_NUMOF);

-    uint16_t tick = TICK_TIMEOUT;
-    while ((i2c_config[dev].dev->SR2 & I2C_SR2_BUSY) && tick--) {}


Let my try to understand this.

Previously i2c_release() would have for TICK_TIMEOUT = 0xffff loop iterations for the device to become idle. If the devices remained busy, the bus was released anyway.

i2c_release() should only be called from the thread currently that has the mutex locked via i2c_acquire().

Any data transfer is done blocking, so the thread that acquired the device will only be able to call i2c_release(), when the bus is idle anyway

Thus, this loop should never spin at all and can be safely removed. Right?

And also: If the relatively slow I2C bus was busy doing some reasonable stuff (e.g. a data transfer), a delay of 0xffff loop iterations won't be enough for that transfer to complete anyway. So there would be no benefit of waiting in any case, right?

If my understanding is right, I agree that removing those line is a good thing.

If the bus was still transferring the delay should actually allow it to finish (depending on clock speeds). However, this implementation ensures that, when an i2c stop is issued, the function will only end if the bus is free. This removes the requirement to check if the line is busy before doing anything (which is needed for repeated start calls).

So yes, it is essentially not doing anything valuable (assuming the api is being followed correctly) and thus should be removed.

MrKevinWeiss · 2018-12-13T14:53:34Z

@maribu Thanks for the review, I did want some feedback on the removing of the busy timeout in the release function. It is a good clarification.

maribu · 2018-12-13T17:00:29Z

I can confirm that the SHT3X driver (using I2C) on the bluepill (STM32F103) just works fine with your I2C implementation :-) The driver does not use every function the I2C does define (e.g. the convenience functions for register access are not used) - so this test does not cover all your code.

MrKevinWeiss · 2018-12-14T08:08:56Z

Thank you very much for testing, I can also volunteer @leandrolanzieri for more test too 😆

MrKevinWeiss · 2018-12-14T08:19:22Z

if the stm32f1 fix gets merged I will have to rebase again on top of that. Just a note to self.

leandrolanzieri · 2018-12-14T12:01:29Z

cpu/stm32_common/periph/i2c_2.c


-#define ENABLE_DEBUG    (0)
+/* DEBUG statements in the init causes the code to lock up */


This could be removed if #10615 is merged.

Yup, will do if it is merged before this one, otherwise I will remove it with the other PR.

@MrKevinWeiss any chance to remove this today? And then we can merge.

Thought I forgot something, done!

leandrolanzieri · 2018-12-14T15:17:25Z

Tested on nucleo-f410rb and works as intended:

Basic read regs with 1,2,3 bytes
Basic write regs with 1,2,3 bytes
Test split frame (NOSTOP, NOSTOP|NOSTART, NOSTART) reads
Test split frame writes
Repeated start write (write_bytes+NOSTOP, write_bytes)
Getting an address nack then read a proper register.

MrKevinWeiss · 2018-12-17T08:32:03Z

@aabadie or @maribu, well now that all the testing has been done...

toonst · 2018-12-18T01:27:30Z

cpu/stm32_common/periph/i2c_2.c

+            clocked out on the line however they get ignored in the firmware.*/
+            if ((i2c->SR1 & I2C_SR1_RXNE) && (length == 1)) {
+                ((uint8_t*)data)[i] = i2c->DR;
+                ret = _stop(i2c);


this can be made return _stop(i2c); because ret is returned anyway

Agreed, the less lines the better

aabadie · 2018-12-18T10:31:00Z

Can you provide the corresponding shell commands of tests/periph_i2c for testing:

1. Basic read regs with 1,2,3 bytes
2. Basic write regs with 1,2,3 bytes
3. Test split frame (NOSTOP, NOSTOP|NOSTART, NOSTART) reads 
4. Test split frame writes 
5. Repeated start write (write_bytes+NOSTOP, write_bytes)
6. Getting an address nack then read a proper register.

I'd like to give a try to this one on nucleo-l152re

MrKevinWeiss · 2018-12-18T10:40:09Z

@aabadie The thing is you need a sensor or something to interface to. If you have a nucleo-103rb you can flash the PHiLIP firmware, drag and drop the firmware and hook up the scl and sda pins.

If you want to use a sensor then let me know what it is and I can tell you the commands.

MrKevinWeiss · 2018-12-18T10:58:52Z

We can generalize certain things for PHiLIP = 85 and = 152, for a sensor is the address of the sensor and would be a register in the sensor (preferable a few) that is readable and writable. Then we could go:

i2c_acquire 0
i2c_read_regs 0 <addr> <reg> 1 0
i2c_read_regs 0 <addr> <reg> 2 0
i2c_read_regs 0 <addr> <reg> 3 0
i2c_write_regs 0 <addr> <reg> 0 42
i2c_write_regs 0 <addr> <reg>+1 0 41 40
i2c_write_regs 0 <addr> <reg>+3 0 39 38 37
/* To confirm read that the register have changed
i2c_read_regs 0 <addr> <reg> 6 0

i2c_read_bytes 0 <addr> 1 4
i2c_read_bytes 0 <addr> 1 12
i2c_read_bytes 0 <addr> 1 8

i2c_read_bytes 0 <addr> 1 4
i2c_read_bytes 0 <addr> 1 12
i2c_read_bytes 0 <addr> 1 8

i2c_write_bytes 0 <addr> 4 <reg>
i2c_write_bytes 0 <addr> 12 35
i2c_write_bytes 0 <addr>  8 34
/* To confirm read that the register have changed
i2c_read_regs 0 <addr> <reg> 2 0

/* Repeated start write */
i2c_write_bytes 0 <addr> 4 <reg>
i2c_write_bytes 0 <addr> 0 33
/* This shouldn't fail but you may not get the correct information unless the sensor supports it */

i2c_read_regs 0 <addr>+1 <reg> 1 0
/* expect nack */
i2c_read_regs 0 <addr> <reg> 1 0 
/* expect success */

That is the basics of it. There is also a test.py in the folder but to use it you should pip3 install riot_pal and there may be some other issues like setting the python path correctly. Or you can get robot framework pushed though and just call

BOARD=nucleo-l152re PHILIP_PORT=<port of PHiLIP> make robot-test -C tests/periph_i2c

aabadie

Tested on a nucleo-l152re and it works. I found small code style issues and I also had an assertion error with one of the commands you gave in your previous comment. See below.

aabadie · 2018-12-18T14:22:39Z

cpu/stm32_common/periph/i2c_2.c

-static inline int _stop(I2C_TypeDef *dev);
-static inline int _wait_ready(I2C_TypeDef *dev);
+static int _start(I2C_TypeDef *dev, uint8_t address_byte, uint8_t flags,
+    size_t length);


Code style: this should be aligned with I2C_TypeDef.

aabadie · 2018-12-18T14:23:02Z

cpu/stm32_common/periph/i2c_2.c

-int i2c_write_regs(i2c_t dev, uint16_t address, uint16_t reg, const void *data,
-                   size_t length, uint8_t flags)
+static int _start(I2C_TypeDef *i2c, uint8_t address_byte, uint8_t flags,
+    size_t length)


Code style: this should be aligned with I2C_TypeDef.

Thanks will do

aabadie · 2018-12-18T14:27:27Z

cpu/stm32_common/periph/i2c_2.c

    assert(i2c != NULL);
+    assert((i2c->SR2 & I2C_SR2_BUSY) || !(flags & I2C_NOSTART));


I got a failed assertion on this line when brainlessly testing using tests/periph_i2c on nucleo-l152re with an hts221 sensor plugged. With the following commands list:

> i2c_acquire 0 > i2c_read_bytes 0 0x5f 1 12

as suggested in #10608 (comment)

Would it make more sense to have a return with a dedicated error value instead of crashing the application in this case ?

Yup I don't mind that, I was trying to keep the binary size down but I can see how that would be annoying when using it (it happened to me a few times)

MrKevinWeiss · 2018-12-18T14:46:59Z

@aabadie All your comments have been addressed!

toonst · 2018-12-18T23:41:15Z

cpu/stm32_common/periph/i2c_2.c


-    if ((flags & I2C_REG16) || (flags & I2C_ADDR10)) {
+    if ((flags & I2C_ADDR10) ||
+        (!(i2c->SR2 & I2C_SR2_BUSY) && (flags & I2C_NOSTART)) {


missing ) here, gives compilation error

...shhhhhh, nothing happened here:sweat_smile:

Refactors i2c_2 to match the structure of i2c_1 better. Corrects functionality issues. Allows the common implementation of read_regs and write_regs. Documents constraints of hardware. Matches error messages with API.

aabadie

Reviewed and tested. Thanks for updating @MrKevinWeiss.

ACK

MrKevinWeiss · 2018-12-20T13:28:18Z

Thanks to everyone for reviewing, testing & merging!

MrKevinWeiss requested review from haukepetersen, aabadie, maribu and dylad December 13, 2018 12:16

MrKevinWeiss self-assigned this Dec 13, 2018

MrKevinWeiss force-pushed the pr/fix/stmi2c2 branch from c1a5609 to 52ee76c Compare December 13, 2018 12:16

MrKevinWeiss mentioned this pull request Dec 13, 2018

cpu/stm32_common: Bug fixes for stm i2c #10540

Closed

maribu reviewed Dec 13, 2018

View reviewed changes

MrKevinWeiss mentioned this pull request Dec 13, 2018

cpu/stm_common: Refactor and cleanup i2c_1 #10610

Merged

MrKevinWeiss mentioned this pull request Dec 14, 2018

DEBUG stops shell if used in i2c init #10614

Closed

MrKevinWeiss requested a review from leandrolanzieri December 14, 2018 08:09

MrKevinWeiss force-pushed the pr/fix/stmi2c2 branch from 52ee76c to b75c9b1 Compare December 14, 2018 09:02

MrKevinWeiss mentioned this pull request Dec 14, 2018

cpu/stm32_common/uart: Prevent uart from sending if not initialized #10615

Merged

leandrolanzieri reviewed Dec 14, 2018

View reviewed changes

leandrolanzieri added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 14, 2018

toonst reviewed Dec 18, 2018

View reviewed changes

MrKevinWeiss force-pushed the pr/fix/stmi2c2 branch from b75c9b1 to 32e2416 Compare December 18, 2018 08:24

aabadie requested changes Dec 18, 2018

View reviewed changes

MrKevinWeiss force-pushed the pr/fix/stmi2c2 branch from 32e2416 to 8e782d2 Compare December 18, 2018 14:46

toonst reviewed Dec 18, 2018

View reviewed changes

MrKevinWeiss force-pushed the pr/fix/stmi2c2 branch from 8e782d2 to 8a5069c Compare December 19, 2018 08:48

aabadie added this to the Release 2019.01 milestone Dec 19, 2018

cpu/stm_common: Refactor and fix implementation for i2c_2

a2e059d

Refactors i2c_2 to match the structure of i2c_1 better. Corrects functionality issues. Allows the common implementation of read_regs and write_regs. Documents constraints of hardware. Matches error messages with API.

MrKevinWeiss force-pushed the pr/fix/stmi2c2 branch from 8a5069c to a2e059d Compare December 20, 2018 11:01

aabadie approved these changes Dec 20, 2018

View reviewed changes

aabadie merged commit ab061c4 into RIOT-OS:master Dec 20, 2018

MrKevinWeiss deleted the pr/fix/stmi2c2 branch January 8, 2019 12:51


		#define ENABLE_DEBUG (0)
		/* DEBUG statements in the init causes the code to lock up */

		assert(i2c != NULL);
		assert((i2c->SR2 & I2C_SR2_BUSY) \|\| !(flags & I2C_NOSTART));

cpu/stm_common: Refactor and fix implementation for i2c_2 #10608

cpu/stm_common: Refactor and fix implementation for i2c_2 #10608

Conversation

MrKevinWeiss commented Dec 13, 2018

Contribution description

Testing procedure

Issues/PRs references

maribu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrKevinWeiss Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrKevinWeiss commented Dec 13, 2018

maribu commented Dec 13, 2018

MrKevinWeiss commented Dec 14, 2018

MrKevinWeiss commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrolanzieri commented Dec 14, 2018

MrKevinWeiss commented Dec 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Dec 18, 2018

MrKevinWeiss commented Dec 18, 2018

MrKevinWeiss commented Dec 18, 2018

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrKevinWeiss commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie left a comment

Choose a reason for hiding this comment

MrKevinWeiss commented Dec 20, 2018

maribu left a comment •

edited

Loading

MrKevinWeiss Dec 14, 2018 •

edited

Loading