Discussion:
yet another stm32f427 i2c problem in nuttx 7.23+
(too old to reply)
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-01-22 17:30:38 UTC
Permalink
Hello all, specifically including David Sidrane ( ;-) ),

While upgrading our codebase from 7.20 to 7.23, i2c has stopped working. We have
a PCA9555 on the bus and also an EEPROM, all running at 400 khz

The behaviour is as follow:

* normal case: I2C_TRANSFER fails with -62 (ETIMEDOUT)

* add some i2c_debug messages: it works.

So, something is going TOO FAST and that triggers a timeout indication. Or that
is the symptom. I know that nothing is simple in stm32 i2c.


After loosing time finding what damn i2c driver is really used for the
stm32f427, I noticed a lot of changes recently in stm32f40xxx_i2c.c

Important: Reversing to the driver shipped in nuttx 7.20 fixed the project.


I have no time yet to bisect the problem, but I invite anyone to be aware that
in certain conditions, in the master branch, this i2c driver is broken.

I would be glad if anyone had a starting clue about what could have caused it to
break...

Sebastien
spudarnia@yahoo.com [nuttx]
2018-01-22 17:50:49 UTC
Permalink
A lot of people change I2C. You might try 'git bisect' to nail it.
a.oryshchenko@yahoo.com [nuttx]
2018-01-23 22:30:24 UTC
Permalink
Hello,

Yes, I got the same problem too. With the same error. But this happens in polling mode. In interrupt mode it still works.

Alexander
pn_bouteville@yahoo.fr [nuttx]
2018-01-23 22:47:45 UTC
Permalink
Hello,

I m using old versions of nuttx because I was noted a big regression and/or instaility when dma or iterrupt mode was added, and now I have not time to search why. Because I switched on bus can. I suggest to split i2c file of stm32 in one file per mode.

Pierre
Ps I found also sone many case where Stm32 I2C finish in dead lock ! And only a power on/off unlock I2C, no stm32 reset neither slave resest unlock I2C.

Envoyé de mon téléphone Windows 10

De : ***@yahoo.com [nuttx]
Envoyé le :mardi 23 janvier 2018 23:30
À : ***@yahoogroups.com
Objet :[nuttx] Re: yet another stm32f427 i2c problem in nuttx 7.23+

 
Hello,

Yes, I got the same problem too. With the same error. But this happens in polling mode. In interrupt mode it still works. 

Alexander
Gregory Nutt spudarnia@yahoo.com [nuttx]
2018-01-23 23:21:48 UTC
Permalink
Post by ***@yahoo.com [nuttx]
Yes, I got the same problem too. With the same error. But this happens
in polling mode. In interrupt mode it still works.
Possibly because interrupt mode is slower?  Sebastien also said that
slowing things down with debug output also eliminates the problem.

I don't do STM32 these days, so I am not going to contribute to this. 
But it would be great if someone could isolate the problem.

Greg
a.oryshchenko@yahoo.com [nuttx]
2018-01-24 00:08:59 UTC
Permalink
Looks that problem was introduced on next commit:
Revision: a3364b5bd94009119a64c0e178d22bfa0aced902
Author: David Sidrane <***@nscdg.com>
Date: 21.09.2017 23:02:05
Message:
Merged in david_s5/nuttx/master_stm32_f4_i2c (pull request #490)


stm32:stm32f40xxx I2C ensure proper isr handling


Injecting data errors that causes a STOP to be perceived by the
driver, will continually re-enter the isr with SB not set and BTF
and RxNE set. This changes allows the interrupts to
be cleared and propagates a I2C_SR1_TIMEOUT to the waiting task.


Approved-by: Gregory Nutt <***@nuttx.org>



Replacing those changes to:
if (priv->dcnt == -1 && priv->msgc != 0 && (status & I2C_SR1_SB) == 0)
{
#if defined(CONFIG_STM32_I2C_DMA) || defined(CONFIG_I2C_POLLED)
return OK;
#else
priv->status |= I2C_SR1_TIMEOUT;
goto state_error;
#endif
}


Solves the problem


But I'll re-check


Alexander
Gregory Nutt spudarnia@yahoo.com [nuttx]
2018-01-24 00:44:30 UTC
Permalink
Unless someone objects, I will apply your changes in the morning (it is
late here).
a.oryshchenko@yahoo.com [nuttx]
2018-01-24 01:18:19 UTC
Permalink
No, still need more investigation
a.oryshchenko@yahoo.com [nuttx]
2018-01-24 08:15:11 UTC
Permalink
Attached patches: 1. I2C
2. RTC (makes DS3231 compilable for STM32F40xxx)
3. at24c02 - fix incorrect page size
4. STM32F4 SPI - removed unnecessary (and incorrect) speed limitation


Alexander
spudarnia@yahoo.com [nuttx]
2018-01-24 12:59:43 UTC
Permalink
All committed. Thanks!


Greg
a.oryshchenko@yahoo.com [nuttx]
2018-01-24 14:15:21 UTC
Permalink
Unfortunately, I2C in interrupt mode still have the same problem (I was incorrect on the beginning). With the same changes as I made for polling it start working too, but comment for David's changes needs more investigation (for polling it is not a problem, but for ISR can be a problem) :(


Alexander
david_s5@usa.net [nuttx]
2018-01-24 14:44:09 UTC
Permalink
I am sorry for not responding sooner I am moving.

Here is the background on the ISR fix.


Initial bug report:
https://github.com/PX4/Firmware/issues/7951 https://github.com/PX4/Firmware/issues/7951



More sophisticated test hardware.
https://github.com/PX4/Firmware/issues/7968 https://github.com/PX4/Firmware/issues/7968



The post here suggest to me that error state may be related to the way the (order and timing) the registers are read. This may have been the root cause of continually re-entering the isr with SB not set and BTF and RxNE set.


I will not be able to retest this until late February as all my possessions are on a boat :(
spudarnia@yahoo.com [nuttx]
2018-01-24 19:37:24 UTC
Permalink
Should I revert the change? Or should we wait until there is some resolution.


You are basically saying that STM32 F4 I2C is basically broken in all modes.
a.oryshchenko@yahoo.com [nuttx]
2018-01-25 06:36:41 UTC
Permalink
If you mean my last changes - then no, you shouldn't, they have influence to polling mode only. And for it described situation is not a problem (in any case we call this function repeatedly).

Alexander
rajanjg89@gmail.com [nuttx]
2018-01-25 18:34:55 UTC
Permalink
Hey Alexander,

I can take a look as well, but I'm not able to reproduce the problem in 7.23 using i2c in isr mode (with i2c frequency of 400khz, all debug msgs off). Do you have any suggestions on how to reproduce the problem?

Rajan
a.oryshchenko@yahoo.com [nuttx]
2018-01-25 21:46:00 UTC
Permalink
I have it easy reproducible with DS3231 driver. But probably no sense for you to spend time, I'm working on it now.

Alexander
a.oryshchenko@yahoo.com [nuttx]
2018-02-09 10:04:32 UTC
Permalink
Hello,

Sorry for big timeout, had many work...
Attached my vision of changes to driver.

I've tested it with DS3231 only and it will be good if someone can do it with other devices.


Alexander
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-09 10:15:32 UTC
Permalink
Greg,

Can we put that i2c work in a branch?

I am bringing up my hn70ap board, nuttx boots, I will want to test an i2c eeprom
soon.

Sebastien
Post by ***@yahoo.com [nuttx]
 
Hello,
Sorry for big timeout, had many work...
Attached my vision of changes to driver.
I've tested it with DS3231 only and it will be good if someone can do it with
other devices.
Alexander
 
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-13 09:46:01 UTC
Permalink
I am writing the i2c eeprom driver and I have problems.


I can only read the EEPROM byte per byte (reading multiple bytes fails, I get
all FFs  or garbage, but this is an EEPROM with a MAC address, so I know it does
not contains all FFs)

I can only write once. That error is probably because I need to do an ACK poll
to check for end of write, but I dont know if I can just send the
start/address/ack/stop sequence without any data message.

Also, I2C_M_NORESTART seems not/partially supported on stm32, in any of the 3
i2c drivers.

Polling mode and alternate driver do not help.


See my attempt in drivers/eeprom/i2c_ee_xx24xx.c in this remote/branch:

https://bitbucket.org/slorquet/nuttx/src/4b12152882b95cfd86f121f6ba3844439a6ab6df/drivers/eeprom/i2c_xx24xx.c?fileviewer=file-view-default

associated header:

https://bitbucket.org/slorquet/nuttx/src/4b12152882b95cfd86f121f6ba3844439a6ab6df/include/nuttx/eeprom/i2c_xx24xx.h?fileviewer=file-view-default


(I did this because the existing at24 driver is exposing a mtd device, which is
not practical for simple eeprom use).


Sebastien
Post by Sebastien Lorquet ***@lorquet.fr [nuttx]
 
Greg,
Can we put that i2c work in a branch?
I am bringing up my hn70ap board, nuttx boots, I will want to test an i2c eeprom
soon.
Sebastien
Post by ***@yahoo.com [nuttx]
 
Hello,
Sorry for big timeout, had many work...
Attached my vision of changes to driver.
I've tested it with DS3231 only and it will be good if someone can do it with
other devices.
Alexander
 
a.oryshchenko@yahoo.com [nuttx]
2018-02-13 10:02:52 UTC
Permalink
Hi Sebastian,

I saw your other topic and trying to use installed at24c32 on my board, I didn't have a plan to use it, but trying... And it doesn't work for me too. But, as for me implementation of this driver is very strange (reading at24c32 documentation). Was it tested earlier?


Did you test PCA9555?


About I2C_M_NORESTART - it is supported only for transmit sequences (look to stm32f40xxx_i2c.c).


Other chips you mentioned, like stm32l4, stm32f7, have completely different I2C module and drivers for it much easier.


I don't use "Alternative" and don't remember what is this. For stm32f4 devices is used stm32f40xxx_i2c.c.


With ds3231 it works fine for reading/writing, I tested it with this in pooling, interrupt and interrupt/dma configs.
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-13 10:23:28 UTC
Permalink
hello

The PCA9555 I use is on a production board so we reintegrated the previously
working i2c driver, since the problems are caused by regressions.

My driver should support the AT24C32, do you want some direct email support for
testing it?

Some changes made in the 32l4/32f7 drivers could, maybe, be ported to the
stm32f4 line. I dont know if it's doable or not.

Regarding your second email: You have to enable filesystem debug (finfo ferr
functions). Not the best choice I admit.

Thanks for notiching the bad shift direction, will change it.

Note that the read and write kludges are enabled for the moment.

sebastien
Post by ***@yahoo.com [nuttx]
 
Hi Sebastian,
I saw your other topic and trying to use installed at24c32 on my board, I
didn't have a plan to use it, but trying... And it doesn't work for me too.
But, as for me implementation of this driver is very strange (reading at24c32
documentation). Was it tested earlier?
Did you test PCA9555?
About I2C_M_NORESTART - it is supported only for transmit sequences (look to
stm32f40xxx_i2c.c).
Other chips you mentioned, like stm32l4, stm32f7, have completely different
I2C module and drivers for it much easier.
I don't use "Alternative" and don't remember what is this. For stm32f4 devices
is used stm32f40xxx_i2c.c.
With ds3231 it works fine for reading/writing, I tested it with this in
pooling, interrupt and interrupt/dma configs.
 
a.oryshchenko@yahoo.com [nuttx]
2018-02-13 10:31:02 UTC
Permalink
Sebastian, yes, lets go to mail.... a.oryshchenko@ on yahoo Give me tips to add your driver to my build tree, please
spudarnia@yahoo.com [nuttx]
2018-02-13 14:08:04 UTC
Permalink
When some one figures out what parts of the changes to the I2C driver need to be backed out, please send me a patch.


That driver has a lot of mileage on it and in the recent past was reliable. So I would prefer not to redesign it but rather to remove whatever broke it.


I know that David Sidrane added a lot of changes to the driver based on benchtop driving I2C with random inputs. Some of those are the most likely culprit I would imagine.


Greg
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-13 14:21:53 UTC
Permalink
Hello

I can now confirm, by means of testing with my EEPROM, that the changes
suggested by Aleksandr Oryshchenko, that never made it to the list because of
yahoo ooops, allows correct behaviour of the i2c driver on stm3f4.

The issue I identified is that:


-without his changes I can never read the EEPROM at once, eg the 2-message
transaction:

START/ADDRESS_W/MEMADDR/RESTART/ADDRESS_R/DATA[n]/STOP does not work for n > 1

I can only read the EEPROM byte per byte, eg repeating multiple times the 2-msg
transaction for increased addresses:

START/ADDRESS_W/MEMADDR/RESTART/ADDRESS_R/DATA[1]/STOP

If I attempt to read more than one byte in the second part of the transaction I
get garbage, mostly FFs but not always.


-with his changes, I can read the entire memory array at once without problems.

There are still problems with the I2C_NO_RESTART flag, but thats probably more a
non-feature than a bug.

I have reattached the patch that works for me. I suggest applying it to the
master branch.


The EEPROM driver still needs a bit of polish before submission.


Sebastien
Post by ***@yahoo.com [nuttx]
 
When some one figures out what parts of the changes to the I2C driver need to
be backed out, please send me a patch.
That driver has a lot of mileage on it and in the recent past was reliable. 
So I would prefer not to redesign it but rather to remove whatever broke it.
I know that David Sidrane added a lot of changes to the driver based on
benchtop driving I2C with random inputs.  Some of those are the most likely
culprit I would imagine.
Greg
spudarnia@yahoo.com [nuttx]
2018-02-13 14:52:25 UTC
Permalink
Committed... Greg
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-13 14:59:32 UTC
Permalink
Thanks for merging and thanks to Aleksandr for the useful fix.

Sebastien
Post by ***@yahoo.com [nuttx]
 
Committed... Greg
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-13 16:54:38 UTC
Permalink
Greg,

This additional patch is required to make the I2C_M_NORESTART mode work.

Change is from Alexander, not me, this is important !

Very important bug fixes, thanks Alexander!

Sebastien
Post by Sebastien Lorquet ***@lorquet.fr [nuttx]
 
Thanks for merging and thanks to Aleksandr for the useful fix.
Sebastien
Post by ***@yahoo.com [nuttx]
 
Committed... Greg
spudarnia@yahoo.com [nuttx]
2018-02-13 17:44:57 UTC
Permalink
Committed and pushed...


A question: I think that most drivers handle the restart by issuing a STOP on the last byte of the previous messages, then START on the next messages.


Is that correct? I am looking at the I2C waveforms of a part that I am studying now (FTDI FT800). On a write operation, it shows the RESTART on the readback, but no STOP at the end of the address write operation.


Greg
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-13 17:53:07 UTC
Permalink
hello

no, this is not correct, "but":

start and stop act as a critical section on the shared i2c bus.

if the i2c bus only has one master as is generally the case, there is no problem.

but on a complex system with multiple masters, the stop condition used to simulate a restart introduces a possible race condition if a secondary master has a pending transaction and detects that the bus has become free.

the repeated start has been defined to avoid this. no stop means the bus was not freed and the original transaction is not interrupted.

so it should be changed to implement a real restart. this is possible on the stm32 and on the prehistoric pic18. probably also on other chips :)

Sébastien
Post by ***@yahoo.com [nuttx]
Committed and pushed...
A question: I think that most drivers handle the restart by issuing a
STOP on the last byte of the previous messages, then START on the next
messages.
Is that correct? I am looking at the I2C waveforms of a part that I am
studying now (FTDI FT800). On a write operation, it shows the RESTART
on the readback, but no STOP at the end of the address write operation.
Greg
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma briÚveté.
spudarnia@yahoo.com [nuttx]
2018-02-13 20:21:52 UTC
Permalink
Exclusive I2C access is enforced by the driver. But there is still an error in the semantics. Some devices might be sensitive to it.


I think that there are limitations in the current flag definitions. There should probably be a NOSTOP flag too to distinguish the cases. Someday, maybe.
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2018-02-13 22:18:40 UTC
Permalink
I meant access to the bus by different hardware i2c masters connected on
the same bus. This is not an ability of the nuttx driver...

A multimaster i2c bus is quite unusual in our common environment but
this is allowed by the spec. eg. for smbus. Imagine two stm32 on the
same i2c bus trying to access a common i2c slave (RTC, etc)

You have to guarantee transaction atomicity when a master sends a
write/read transaction. If you send a stop/start between the write and
the read part of the transaction, then it breaks its atomicity, because
another i2c master (eg, the second stm32) can start a transfer as soon
as the stop is seen. A true restart avoid signaling the bus as free.

(i2c masters have electronic circuits that monitor start and stop
conditions and deduce a bus utilization flag)

This can be as simple as avoiding the stops in the middle of the
transaction sequences. Just send start again before each part of the
transaction.

Sebastien
 
Exclusive I2C access is enforced by the driver.  But there is still an
error in the semantics.  Some devices might be sensitive to it.
I think that there are limitations in the current flag definitions. 
There should probably be a NOSTOP flag too to distinguish the cases. 
Someday, maybe.
'David S. Alessio' david.s.alessio@gmail.com [nuttx]
2018-02-13 23:45:34 UTC
Permalink
Quite right, Sebastien.

What that means is that there are three types of I2C transactions that need to be implemented at the driver level (of NuttX, of any other RTOS). Those are:
Read n bytes to device ID x (start/read, stop)
Write n bytes to device ID x (start/write, stop)
Write n bytes to device ID x followed by a read of m bytes (start/write, restart, stop)

Only the third can assure atomicity in a multi-master system.

Regards,
-david
I meant access to the bus by different hardware i2c masters connected on the same bus. This is not an ability of the nuttx driver...
A multimaster i2c bus is quite unusual in our common environment but this is allowed by the spec. eg. for smbus. Imagine two stm32 on the same i2c bus trying to access a common i2c slave (RTC, etc)
You have to guarantee transaction atomicity when a master sends a write/read transaction. If you send a stop/start between the write and the read part of the transaction, then it breaks its atomicity, because another i2c master (eg, the second stm32) can start a transfer as soon as the stop is seen. A true restart avoid signaling the bus as free.
(i2c masters have electronic circuits that monitor start and stop conditions and deduce a bus utilization flag)
This can be as simple as avoiding the stops in the middle of the transaction sequences. Just send start again before each part of the transaction.
Sebastien
Post by ***@yahoo.com [nuttx]
Exclusive I2C access is enforced by the driver. But there is still an error in the semantics. Some devices might be sensitive to it.
I think that there are limitations in the current flag definitions. There should probably be a NOSTOP flag too to distinguish the cases. Someday, maybe.
a.oryshchenko@yahoo.com [nuttx]
2018-02-13 10:14:18 UTC
Permalink
About your source, did you enable log messages? Any info from it?

btw: n.i. for 24xx02, but mostake: maddr[0] = memaddr<<8;
Loading...