Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

I2C on STM32 #71

Closed
dewagter opened this Issue Oct 17, 2011 · 19 comments

Comments

Projects
None yet
5 participants
Owner

dewagter commented Oct 17, 2011

I2C lowlevel driver update needed on STM32.

@ghost ghost assigned dewagter Oct 17, 2011

Owner

dewagter commented Oct 20, 2011

A lot of progress was made. One last big issue causes a lot of efficiency loss:

datasheet: " ... when the STOP or START bits are set, the software must not write to CR1 before the bits are cleared by hardware. Otherwise there is a risk of setting a second STOP or START ... "

sometimes (typically after a bus or ack error) a second stop condition is generated (even if only 1 was asked for) which means that no more interrupts are generated (stop=finished). If there were still packets on the I2C stack then the interrupt routine had to start the next. No more interrupts means the next packet is not started.

Here is the solution proposed from the "reference i2c code from www.st.com" :

        /* Make sure that the STOP bit is cleared by Hardware before CR1 write access */
        while ((I2Cx->CR1&0x200) == 0x200);

Current solution:

i2c_event() // instead of having the interrupt routine start the next transaction at the end: efficiency loss at high bitrates: 10%
{
if (i2c_idle) // checks the busy and stop flags
start_next()
}

Owner

flixr commented Jan 8, 2012

@dewagter what is the status on the stm_i2c branch? ready to merge into the dev branch?
I had lisa/m 1.0 with aspirin and mkk controllers running with this for over 6h without problems...

Owner

dewagter commented Jan 9, 2012

Hi Felix,

Thanks a lot for testing the code. The driver is not perfect/ready yet as I
heard of several combinations that do not work yet. But it seems to work in
other combinations than Antoine's driver.

The driver probably could be improved a whole lot by adding the planned
watchdog timer, as the biggest problem is not bad communication but
stalling (e.g. on STM not detecting a stop condition that did occur). But I
got stuck on which system timer to use, and did not add this yet as I still
tried to solve the "hangs" in a more proper way so they would not occur.
Most of the code for the watchdog is already written.

I'm also in the process of getting some more inside information from ST
themselves.

The good things about the driver:
a) it runs using much less interrupts than the previous driver, it runs at
any bitrate from 2k to 500kbps, and there is not a single known unexpected
behaviour...
b) there are many error conditions that can occur at many moments (> 5 * 12

    1. and the combined event + error interrupts together with volatile
      atomic registers that get changed upon read are know to lead to issues.

-Christophe

On Sun, Jan 8, 2012 at 1:43 AM, Felix Ruess <
reply@reply.github.com

wrote:

@dewagter what is the status on the stm_i2c branch? ready to merge into
the dev branch?
I had lisa/m 1.0 with aspirin and mkk controllers running with this for
over 6h without problems...


Reply to this email directly or view it on GitHub:
#71 (comment)

Owner

flixr commented Jan 9, 2012

Hi Christophe,

I was happy yesterday when I let lisa/m 1.0 with aspirin 1.5 running on table for a 6h and it didn't get stuck because of i2c anymore, but now I noticied that the gyro data is total crap. Not sure what is read, definitely not the gyro data. The mag data is just fine... so it might be some weirdness in the aspirin driver. Did you test that yet?

I wouldn't have thought that the STM misses stop conditions!
Regarding the system timer to use: I started integrating the new system timers from Antoine (see systime branch), where you can now register "soft timers" based on the sys_time timer (with SYSTIME_RESOLUTION that currently defaults to 1./1024). But that resolution might be a bit too coarse..

Owner

dewagter commented Jan 9, 2012

Did not test aspirin with new driver. But I did read a discussion about an
"extra byte" once in a while !? Would be nice to test the "aspirin_i2c"
subsystem, cause I know that driver better.

STM measures the I2C bus to see if what is expected happens. When there is
some interference or capacitance problem there is no nice 0 - 3.3Volt bit
on the bus anymore. And < 0.3 to 0.4 Vcc is low, and > 0.6 to 0.7 Vcc is
high but in between is undefined. So when the scope makes a 1 to 2 volt
bit, the behaviour of I2C is undefined. This causes the "misses a stop
condition".

I think I need to set the delays a bit faster than 1/1024 sec
unfortunately... (depending on bitrate)

On Mon, Jan 9, 2012 at 1:58 PM, Felix Ruess <
reply@reply.github.com

wrote:

Hi Christophe,

I was happy yesterday when I let lisa/m 1.0 with aspirin 1.5 running on
table for a 6h and it didn't get stuck because of i2c anymore, but now I
noticied that the gyro data is total crap. Not sure what is read,
definitely not the gyro data. The mag data is just fine... so it might be
some weirdness in the aspirin driver. Did you test that yet?

I wouldn't have thought that the STM misses stop conditions!
Regarding the system timer to use: I started integrating the new system
timers from Antoine (see systime branch), where you can now register "soft
timers" based on the sys_time timer (with SYSTIME_RESOLUTION that currently
defaults to 1./1024). But that resolution might be a bit too coarse..


Reply to this email directly or view it on GitHub:
#71 (comment)

Owner

flixr commented Jan 27, 2012

@dewagter
The aspirin driver (aspirin_v1.x subsystem) does not work anymore, looks like we get totally random readings from the gyro. Looks a bit like hugely amplified noise.
Maybe imu-3000 initialization issue?

Owner

flixr commented Jan 28, 2012

Quickly hacked modules/sensors/imu_ppzuav.c to work with rotorcraft firmware (that needs renaming and some cleanup btw) and tested with aspirin_i2c subsystem.
Now the gyros are working nicely, but I don't get any measurements from the adxl345. Do we need to change some wiring or something to use aspirin on lisa/m in i2c only mode?
Have you tested any aspirin drivers with the new i2c code already?

Owner

dewagter commented Jan 30, 2012

Ho Felix, the reason I put aspirin_i2c in fixedwing only is the limited
bandwidth. Gyros are read first so I van imagine the acc does not fit
anymore.
On Jan 28, 2012 4:26 AM, "Felix Ruess" <
reply@reply.github.com>
wrote:

Quickly hacked modules/sensors/imu_ppzuav.c to work with rotorcraft
firmware (that needs renaming and some cleanup btw) and tested with
aspirin_i2c subsystem.
Now the gyros are working nicely, but I don't get any measurements from
the adxl345. Do we need to change some wiring or something to use aspirin
on lisa/m in i2c only mode?
Have you tested any aspirin drivers with the new i2c code already?


Reply to this email directly or view it on GitHub:
#71 (comment)

Owner

flixr commented Jan 30, 2012

Would be good if you could look into the issues with the aspirin driver, as I don't know enough about the internals there.
We really need to get this working asap...

Owner

dewagter commented Mar 15, 2012

Aspirin driver "Fixed", but potential other drivers might suffer from the same flaw: bf2b29a

Owner

flixr commented May 27, 2012

I think we should use the I2CTransceive macro in all of these cases, so you don't accidentally forget to set the transaction type to TxRx again.

The question is if we should always use the I2CTransmit, I2CReceive and I2CTransceive macros instead of calling i2c_submit directly? It would be more consistent...
But probably in cases where you only transmit and set the transaction type, length, etc. once at the beginning it is a bit of a waste to set these again every time.

Also, is there any reason to keep these macros as macros? Or can we turn them into static inline functions which are nicer for debugging...

Owner

flixr commented Jun 1, 2012

@dewagter In current master with Lisa/M 2 and Aspirin 2.1 it get's immediately stuck at a while loop checking the i2c status:
while (baro_trans.status == I2CTransPending || baro_trans.status == I2CTransRunning);
If you disable the baro it will just get stuck at a similar while loop of the hmc mag a bit later...
While loops without timeouts like that are bad...

Owner

dewagter commented Jun 3, 2012

Hi Felix,

While loops are very bad, but failing to configure the devices is even much
much worse, so it hangs on purpose. That is why some while loops are left
in init functions only. If you see any while anywhere that might not be an
init, please tell me and I'll make it a top priority. We have this problem
too on lisa-m when the eagletree sensor for instance.

I'll try to fix it in the locm3 branch.

C.

On Friday, June 1, 2012, Felix Ruess wrote:

@dewagter In current master with Lisa/M 2 and Aspirin 2.1 it get's
immediately stuck at a while loop checking the i2c status:
while (baro_trans.status == I2CTransPending || baro_trans.status == I2CTransRunning);
If you disable the baro it will just get stuck at a similar while loop of
the hmc mag a bit later...
While loops without timeouts like that are bad...


Reply to this email directly or view it on GitHub:
#71 (comment)

-Christophe

Do we still have an issue here with the support of i2c on stm32 or is it only in peripherals that things could be improved ?
If the driver itself is working, maybe we could close this issue.

Owner

flixr commented Jun 15, 2012

It is still missing the watchdogs... since this is purely interrupt driven, if you loose one for whatever reason it could get stuck.

gautierhattenberger added a commit that referenced this issue Feb 7, 2013

[actuators/asctec] disable the i2c hack for the general case
It can be activated for lisa/m boards with the flag:
USE_I2C_ACTUATORS_REBOOT_HACK

This issue should be handled at the i2c level (#71)
Contributor

OpenUAS commented Dec 17, 2013

I think this generic milestone could be closed and a specific issue opened: improve I2C driver robustness. Otherwise we are left hanging with milestones forever.

Owner

flixr commented Dec 17, 2013

Removed this issue from the libopencm3 milestone which was basically reached quite a while ago.

Member

podhrmic commented Dec 17, 2013

this is related to #531

@flixr flixr closed this in #662 Apr 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment