Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I2C fixes #856

Merged
merged 9 commits into from Mar 14, 2016

Conversation

@avtolstoy
Copy link
Member

commented Feb 1, 2016

Fixes #854

@m-mcgowan m-mcgowan added this to the 0.4.10 milestone Feb 1, 2016

@tlangmo

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2016

I tried the changed file but it actually breaks the i2c communication. I use an MPU9250 Invensense chip on develop branch.
I commented the changes out one by one, and everything went back to normal when
line 426 while(i2cMap[i2c]->I2C_Peripheral->CR1 & I2C_CR1_STOP || I2C_GetFlagStatus(i2cMap[i2c]->I2C_Peripheral, I2C_FLAG_BUSY))
stays as it was.
Interestingly, this is the only instance where the I2C_GenerateSTOP() call is actually conditional (on the last byte only). Can you please double check that?

I am excited about this fix (I had a lot of instabilities with I2C) but maybe in that one code instance it breaks things?

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2016

@tlangmo Thanks for reporting this! I'm still testing this and you are correct, that place looks a bit strange. There is no point in waiting for I2C_CR1_STOP to clear after every data byte and waiting on I2C_FLAG_BUSY will definitely cause a timeout.

@avtolstoy avtolstoy assigned avtolstoy and unassigned avtolstoy Feb 2, 2016

@technobly

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

The reason we initially started waiting for the STOP bit to be sent, is that a user app may try to start a new transmission before the STOP bit has a change to actually send. This resulted in I2C slaves not responding anymore IIRC.

Maybe you meant to use && busy instead of || busy in the while() loop? However it sounds like to fix #854 we only need to add while(i2cMap[i2c]->I2C_Peripheral->CR1 & I2C_CR1_STOP) in endTransmission() as you've done.

@avtolstoy avtolstoy force-pushed the avtolstoy:fix-i2c-wait-stop branch from 22974ca to f018347 Feb 5, 2016

@avtolstoy avtolstoy changed the title Wait for I2C_CR1_STOP to clear. Additionally wait for I2C_FLAG_BUSY I2C fixes Feb 5, 2016

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2016

Updated PR.

Apart from fixing #854 by waiting for I2C_CR1_STOP to clear in HAL_I2C_End_Transmission, PR also includes:

  • Rewrite of HAL_I2C_Request_Data basing on ST AN2824
  • Fix for transactions with repeated START condition between them
  • Removed SingleThreadedSection from TwoWire
  • Tests for consecutive transactions with various combinations of STOP/repeated START conditions inbetween
@technobly

This comment has been minimized.

Copy link

commented on hal/src/stm32f2xx/i2c_hal.c in 292f9aa Feb 7, 2016

How are we pre-configuring this without testing how many bytes are to be received? We have seen in the past that it's too late to test for quantity after the I2C_Send7bitAddress() is called for 1 byte, so I2C_AcknowledgeConfig(i2cMap[i2c]->I2C_Peripheral, DISABLE); done later can be too late and an ACK gets sent.

@avtolstoy

This comment has been minimized.

Copy link
Owner Author

commented on hal/src/stm32f2xx/i2c_hal.c in 292f9aa Feb 7, 2016

@technobly I2C hardware will not continue to clock SCL (and in turn will not schedule ACK) until EV6 state is cleared by clearing ADDR bit in SR1. The proper sequence to clear it according to reference manual is:

Bit 1 ADDR: Address sent (master mode)/matched (slave mode)
This bit is cleared by software reading SR1 register followed reading SR2, or by > hardware when PE=0.

Previously, I2C_CheckEvent was used to wait for EV6 which indeed reads both SR1 and SR2 and clears ADDR bit, so ACK/NACK has to be configured prior to this call. Here we only check SR1 register with I2C_GetFlagStatus for I2C_FLAG_ADDR and completely clear it only after setting ACK/NACK configuration and reading SR2.

This comment has been minimized.

Copy link

replied Feb 7, 2016

Sounds good @avtolstoy 👍 Thanks for explaining that.

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2016

@technobly
STM32F205 reference manual states the same thing as AN2824:

For 2-byte reception:
• Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
• Set ACK low, set POS high
• Clear ADDR flag

@tlangmo

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2016

I am not sure how helpful this is, but I tested the new i2c_hal.c and noticed that I now have a lot of intermittent 'stalls' when I use i2c in a thread. The stalls are likely related to the EVENT_TIMEOUT, being ~100ms.
The old code did not have that problem. I already suspend the scheduler for i2c calls, so I have no clue why multithreading would have an impact. There are also no other calls which use i2c, so no race condition. Please let me know if I can do a more specific test...

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2016

@tlangmo Thanks for testing this. Could you provide a code sample so I can try to replicate this issue?

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

Updated PR.

  • Apparently N > 2 Master receiver sequence is a bit different for STM32F2. 15816d4 should fix the issue with I2C when multi-threading is enabled.
m-mcgowan added a commit that referenced this pull request Mar 14, 2016

@m-mcgowan m-mcgowan merged commit b439f15 into particle-iot:develop Mar 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.