Discussion:
[nuttx] Opinions needed on state of F3 I2C bottom half
dwvcfii@yahoo.com [nuttx]
2015-12-30 15:56:42 UTC
Permalink
I'm working with the F3 discovery board (STM32F303VC) and am presently trying to get I2C working on it.

Unfortunately, it appears that in its infinite wisdom, ST made the register map slightly different between the F1,F2,F4 vs. the F3. Hence the reason nuttx has separate drivers for these devices.

I have also noticed that we have two different drivers for the F1/F2/F4, "stm32_i2c.c" and "stm32_i2c_alt.c". It would appear that the alt driver is a newer / tweaked version of the other driver, but as the Kconfig description implies it's not sufficiently tested on other platforms to justify inclusion as the primary driver. Fine.

I spent the last couple of days learning the F3 I2C peripheral and stepping through the existing F3 driver (stm32f3xxx_i2c.c), particularly the ISR, and it's painfully clear this is an unfinished work. The reads / writes work in the context that they produce:

[Start/Address][Subaddress][Restart/Address][Data]

on the wire and don't hang the device, but my slave devices do not tolerate / respond to restarts on writes...they expect:

[Start/Address][Subaddress][Data]

....so I went down the rabbit hole of attempting to activate the no-restart flag (which the I2CTool does not, coincidently). Long story short: this results in a trace overflow because the ISR state machine is not correctly handling the state of the device, it gets stuck and just keeps looping doing nothing useful until the trace buffer overflows.

While debugging I noticed a few other undesirable properties, including simple stuff like the local / private copy of the status register not being updated at the top of the ISR, so the state of the device was always in question. Not surprisingly, upon reviewing the stm32_i2c_alt.c driver I found this was properly done. And further review of the ISR in that driver reveals that it's MUCH better written. Long story short, the F3 driver ISR is incomplete and broken in some cases.

I'm not sure of the chronology here exactly (I'm being lazy), but my guess is the F3 driver was forked from the primary stm32_i2c driver some time ago and then someone came along and revamped the ISR in the primary / alt driver, because while there are many similarities between the drivers, the ISRs are completely different.

So I'm considering integrating the alt driver ISR with the existing F3 driver, with the intent to publish the final result.

Anyone have any objections / concerns about this path? Greg -- what do you think?
spudarnia@yahoo.com [nuttx]
2015-12-30 16:38:48 UTC
Permalink
I have never really been deeply involved in the STM32 I2C driver. I am not the primary author (although I did fix a few bugs years ago). And have not use it (or any STM32) for several years other than some minor testing. I am not the expert here.


My understanding is that the "alt" version was "forked" from the original version to handle an F103 Errata, but I don't know for sure. Maybe there is some historical information in these forum messages. F4 does not use it and I don't think other F's need it (but I don't know for sure). F3 tends to be more like F1, but I have no knowledge at all on this subject of whether the errata is required or not.


PX4 also carries additional I2C changes that are mentioned in this forum but have never been merged upstread. They periodically threaten to re-write the STM32 I2C driver.


Sorry, I can't help you here since I am not involved with STM32 I2C. But there are others who are. Perhaps they can enlighten us.


Greg
max.kriegleder@yahoo.com [nuttx]
2016-01-03 20:05:01 UTC
Permalink
I think I can help out a bit here: We were the ones contributing the changes that eventually made it to the stm32_i2c_alt implementation as we had noted an issue with the original implementation (I cannot remember the exact circumstances, but I think there was an issue with receiving only one or two bytes on the F1 chip). My colleague determined the source of the problem and starting from the original implementation came up with something that worked and at the same time he cleaned up the ISR. We proposed this patch to Greg, but as we did not test it on any other chip than the F1, we agreed to leave it as an 'alternative' implementation that ideally would be tested by more people on different chips (F4,F2,..) and integrated in a 'one-fits-all' solution.

Now looking back and having had similar issues with other lower-half drivers, I think the solution that spans different chips (like F1-F4) is not ideal unless they are really the same. There are often errata and unique cases for the different chips and it seems to me that the current solution having #ifdef all over the place treating the special cases for each chip is not a practical solution. I think Greg even brought this issue up himself, but of course it would require somebody to dive into this and separate a lot of code. However, this could at least be a starting point where we separate the STM F1 I2C implementation from the STM F3 I2C implementation and so forth.

Max
spudarnia@yahoo.com [nuttx]
2016-01-03 20:16:15 UTC
Permalink
What we really need is someone to volunteer to be the "owner" of the STM32 driver architecture. Issues like these that span multiple STM32 parts and platforms just don't resolve themselves via grass roots, bottom-up development.


Any volunteers?


Greg
spudarnia@yahoo.com [nuttx]
2016-01-03 20:30:37 UTC
Permalink
In addition to the stm32_i2c_alt.c work-around, there is also the known STM32 fix at: https://github.com/PX4/NuttX/issues/54 that has never been incorporated into the NuttX I2C drivers.

STM32 I2C is pretty much a mess.
dwvcfii@yahoo.com [nuttx]
2016-01-06 15:14:54 UTC
Permalink
After several days of working on this, comparing drivers, reading datasheets and eratta, I think I share the opinion of maxikrie. We need separate drivers for each chip. I think it's foolish to think that any one person could possibly build and realistically maintain a driver that supports every feature, every model of device, and accommodates all the silicon-revision-specific errata. Most people work with one particular flavor of the device and get to know that one device well. So the drivers need to be separate and each driver needs a maintainer.

That said, I've been working the last few days on a new F3 driver and I'm just about to begin testing. That could go well...or not. We'll see. My hope is that this updated driver will be able to replace stm32f30xxx_i2c.c and if all goes well I plan to submit the updated driver to Greg once I'm confident it works on a reference design like the STM32 F3 Discovery board. At that point it should serve as a more stable base on which others can add new functionality for the F3. Time permitting (I'm running a startup after all), I'm willing to update this driver with any changes / bug fixes I make, but I'm not sure I can go much beyond that level of support for it. Fortunately my driver will be FAR better documented than the current version so it should be a lot easier for others to learn about the F3 implementation and pick up where I have left off.

In other news, misleading / incorrect comments cut-and-pasted from other device drivers are worse than no comments. Grrrrr.
max.kriegleder@yahoo.com [nuttx]
2016-10-19 14:34:29 UTC
Permalink
Hi All,

I have written what I believe is a proper implementation of the stm32 F4 i2c bottom half as this is not handled correctly in the current implementation (see also https://github.com/PX4/NuttX/issues/54 https://github.com/PX4/NuttX/issues/54). My implementation is very similar to the F1 I2C bottom half, which a colleague of mine has written. The changes almost exclusively affect the ISR. I have attached for people to test or for Greg to pull. With this we should at least have a separate working bottom half for the F1 and F4 chip, maybe this could be reflected by options in the configuration utility?

Max
spudarnia@yahoo.com [nuttx]
2016-10-19 14:46:30 UTC
Permalink
Can people using F4 please review and comment. I don't use F4 but I am willing to slam dunk the code in place if other F4 users concur.
david_s5@usa.net [nuttx]
2016-10-19 16:22:37 UTC
Permalink
Hi,

I would like if we could vet this on a branch.


Greg would it be ok to add a branch with this replacement so we can review the diff and test it?
spudarnia@yahoo.com [nuttx]
2016-10-19 17:48:14 UTC
Permalink
Okay, done. Here is the branch: https://bitbucket.org/nuttx/nuttx/branch/stm32i2c

I have not tried to build it and I will not be involved in any changes or testing of any kind. Please submit patches or PRs against the branch if you have any issues.

I guess David S. is the point man. But others of you should check on other platforms.

This purports to be an F4 change but the changes are in a common I2C file that probably will effect other family members as well.

Greg
david_s5@usa.net [nuttx]
2016-10-19 17:50:21 UTC
Permalink
Thank you Greg!

I will work in testing ASAP.
max.kriegleder@yahoo.com [nuttx]
2016-10-20 13:16:07 UTC
Permalink
Ideally, as discussed before, there will be a particular driver for each family member as they all have different errata and specifications in their protocol. As of now we have the existing solution and the F1 and F4 solution we committed.

I propose to leave the existing solution as is (for other family members currently not supported by a specific solution) and add the F1 and F4 solution as the designated driver for the F1 and F4, respectively. Then, we don't have to check all platforms and we can converge on a solution that works for the F1 and F4 accommodating all errata and specifications. Ideally, in the future others commit their driver for the F3 and so on.

Max
david_s5@usa.net [nuttx]
2016-10-20 13:43:35 UTC
Permalink
Hi Max,

I totally agree and support the model.


I will test your changes on PX4 and report back as soon as I can.


David
Sebastien Lorquet sebastien@lorquet.fr [nuttx]
2016-10-20 14:07:49 UTC
Permalink
I also think that this structure makes sense.

Sébastien Lorquet
Post by ***@usa.net [nuttx]
Hi Max,
I totally agree and support the model.
I will test your changes on PX4 and report back as soon as I can.
David
david_s5@usa.net [nuttx]
2016-10-21 03:37:43 UTC
Permalink
Hi,

Max. This is really clean and very well documented. Thank you!


Initial testing with all the devices I have is working!


Did you capture timing on a logic analyzer or should I look at that as a final check?


David
david_s5@usa.net [nuttx]
2016-10-21 18:20:31 UTC
Permalink
I have captured the timing. It all looks good! So I am good with it being merged to master.

Anyone else have input to share?
spudarnia@yahoo.com [nuttx]
2016-10-21 18:24:54 UTC
Permalink
Before this can be merged into the master, it must also be verified on L1, F1, and F2.


Or, as was suggested, rename the file to stm32f4xx_i2c.c and restore the old stm32_i2c.c for the untested platforms.


Greg
david_s5@usa.net [nuttx]
2016-10-21 18:43:02 UTC
Permalink
I agree. What if the rename done now for each of the


STM32L15XX
STM32F10XX
STM32F20XX
STM32F40XX



essentially 4 copies of the same new file and Kconfig choice that defaults to using the new driver under STM32F40XX and the old under STM32L15XX,STM32F10XX,STM32F20XX


This way, as tested, the default can be changed for each and any fixes only applicable to one will not couple, but common bugs will have to be merged across all?
max.kriegleder@yahoo.com [nuttx]
2016-10-24 09:31:08 UTC
Permalink
I think this makes sense. Please be aware that there exists already a working solution for the F1 under the name stm32_i2c_alt.c.

Max
spudarnia@yahoo.com [nuttx]
2016-10-24 12:31:42 UTC
Permalink
The stm32_i2c_alt.c file is another unknown I2C file that we never decided what to do with. It was submitted as a fix for an errate in the F1 performance line parts only. There was no indication or testing that it worked with any other F1 parts. The file is select based a configuration option:

arch/arm/src/stm32/Make.defs:

ifeq ($(CONFIG_STM32_I2C_ALT),y)
CHIP_CSRCS += stm32_i2c_alt.c
else
...

It can be selected for any STM32 part other than STM32 F3. But it defaults to enabled only for F1 performance line parts:

arch/arm/src/stm32/Kconfig:
config STM32_I2C_ALT
bool "Alternate I2C implementation"
default n if !STM32_PERFORMANCELINE
default y if STM32_PERFORMANCELINE
depends on !STM32_STM32F30XX

There are currently eight STM32 F1 configurations that have I2C enabled:

$ find configs -name defconfig | xargs grep -l STM32F10XX=y | xargs grep STM32_I2C[0-9]=
configs/fire-stm32v2/nsh/defconfig:CONFIG_STM32_I2C1=y
configs/maple/nx/defconfig:CONFIG_STM32_I2C1=y
configs/maple/nx/defconfig:CONFIG_STM32_I2C2=y
configs/olimexino-stm32/can/defconfig:CONFIG_STM32_I2C2=y
configs/olimexino-stm32/composite/defconfig:CONFIG_STM32_I2C2=y
configs/olimexino-stm32/nsh/defconfig:CONFIG_STM32_I2C2=y
configs/stm3210e-eval/nsh2/defconfig:CONFIG_STM32_I2C1=y
configs/stm3210e-eval/nx/defconfig:CONFIG_STM32_I2C1=y

All of these select the alternate configuration:

$ find configs -name defconfig | xargs grep -l STM32F10XX=y | xargs grep -l STM32_I2C[0-9]= | xargs grep I2C_ALT=y
configs/fire-stm32v2/nsh/defconfig:CONFIG_STM32_I2C_ALT=y
configs/maple/nx/defconfig:CONFIG_STM32_I2C_ALT=y
configs/olimexino-stm32/can/defconfig:CONFIG_STM32_I2C_ALT=y
configs/olimexino-stm32/composite/defconfig:CONFIG_STM32_I2C_ALT=y
configs/olimexino-stm32/nsh/defconfig:CONFIG_STM32_I2C_ALT=y
configs/stm3210e-eval/nsh2/defconfig:CONFIG_STM32_I2C_ALT=y
configs/stm3210e-eval/nx/defconfig:CONFIG_STM32_I2C_ALT=y

And these are all performance line parts:

$ find configs -name defconfig | xargs grep -l STM32F10XX=y | xargs grep -l STM32_I2C[0-9]= | xargs grep PERFORMANCE
configs/fire-stm32v2/nsh/defconfig:CONFIG_STM32_PERFORMANCELINE=y
configs/maple/nx/defconfig:CONFIG_STM32_PERFORMANCELINE=y
configs/olimexino-stm32/can/defconfig:CONFIG_STM32_PERFORMANCELINE=y
configs/olimexino-stm32/composite/defconfig:CONFIG_STM32_PERFORMANCELINE=y
configs/olimexino-stm32/nsh/defconfig:CONFIG_STM32_PERFORMANCELINE=y
configs/stm3210e-eval/nsh2/defconfig:CONFIG_STM32_PERFORMANCELINE=y
configs/stm3210e-eval/nx/defconfig:CONFIG_STM32_PERFORMANCELINE=y

It is not selected in any other configurations.
max.kriegleder@yahoo.com [nuttx]
2016-10-24 13:39:58 UTC
Permalink
I just wanted to point out that if we decide to accept my F4 version and to do what we discussed (have a specific driver for each platform) then there also already exists a F1 specific version (the stm32_i2c_alt.c), which my colleague and I wrote in the same manner as the F4 version.
max.kriegleder@yahoo.com [nuttx]
2016-10-24 13:45:09 UTC
Permalink
Yes, that is correct. I just wanted to point out that we already committed a F1 specific version (the stm32_i2c_alt.c) and this should be considered the designated version for the F1 performance line if we decided to split up the i2c drivers for the STM32 (David, I guess, was not aware of this as he proposed to use again the existing default driver, stm32_i2c.c, for the F1 if we split up the drivers).
spudarnia@yahoo.com [nuttx]
2016-10-24 22:38:55 UTC
Permalink
I have included Max's STM32 F4 I2C changes with commit 1d502593581a27d96be5c247debf5d03a6fc73c6. I did the minimal, safe integration: I simply renamed the Max's new file to stm32f40xx_i2c.c and build that version for all F4's. Nothing else has changed.


I have removed the stm32i2c branch. Thanks for the review and verification,


David.

Loading...