{"action":"created","issue":{"url":"https://api.github.com/repos/spark/firmware/issues/1042","repository_url":"https://api.github.com/repos/spark/firmware","labels_url":"https://api.github.com/repos/spark/firmware/issues/1042/labels{/name}","comments_url":"https://api.github.com/repos/spark/firmware/issues/1042/comments","events_url":"https://api.github.com/repos/spark/firmware/issues/1042/events","html_url":"https://github.com/spark/firmware/issues/1042","id":161687844,"number":1042,"title":"More i2c transaction errors following 0.5.1 release","user":{"login":"pomplesiegel","id":1450352,"avatar_url":"https://avatars.githubusercontent.com/u/1450352?v=3","gravatar_id":"","url":"https://api.github.com/users/pomplesiegel","html_url":"https://github.com/pomplesiegel","followers_url":"https://api.github.com/users/pomplesiegel/followers","following_url":"https://api.github.com/users/pomplesiegel/following{/other_user}","gists_url":"https://api.github.com/users/pomplesiegel/gists{/gist_id}","starred_url":"https://api.github.com/users/pomplesiegel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pomplesiegel/subscriptions","organizations_url":"https://api.github.com/users/pomplesiegel/orgs","repos_url":"https://api.github.com/users/pomplesiegel/repos","events_url":"https://api.github.com/users/pomplesiegel/events{/privacy}","received_events_url":"https://api.github.com/users/pomplesiegel/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-06-22T13:49:45Z","updated_at":"2016-06-22T18:19:40Z","closed_at":null,"body":"We noticed an uptick in the amount of i2c transactional errors following the 0.5.1 release. After speaking with @technobly, it became clear that `SINGLE_THREADED_BLOCK` or `CriticalSection` are no longer protecting the actual i2c transaction calls, `Wire.endTransmission()` and `Wire.requestFrom()`. We believe these protections (and possibly even ATOMIC_BLOCK() to disable interrupts) are necessary for a full-featured multi-threaded program to behave normally over time. \r\n\r\nHere is a test program which can be refined and added as a regression test, as i2c changes really should be thoroughly tested against real hardware before packaged and released. \r\n\r\n#### Test program \r\n\r\nThis program keeps track of the number of i2c transactional errors which occur when constantly communicating with a MCP23017 port expander, while publishing irrelevant data. To check the number of errors, `get` the variable `i2cErrors`.\r\n\r\n````cpp\r\n#include \"application.h\"\r\n#include \"Adafruit_MCP23017.h\"\r\n\r\nSYSTEM_MODE(SEMI_AUTOMATIC);\r\nSYSTEM_THREAD(ENABLED);\r\n\r\nAdafruit_MCP23017 myPE; \r\n\r\nstatic int peState = false; \r\n\r\nint numI2cErrorsSoFar = 0;\r\n\r\nvoid PointlessPublish()\r\n{\r\n  static unsigned long millisOfLastPublish = 0;\r\n\r\n  if( Particle.connected() )\r\n  {\r\n    if( ( millis() - millisOfLastPublish ) > random(260, 500) )\r\n    {\r\n      millisOfLastPublish = millis();\r\n      Serial.println(\"Publishing bull....\");\r\n      Particle.publish(\"bull\", \" fjksdas \" + String(millisOfLastPublish) + String(peState) );\r\n    }\r\n  }\r\n}\r\n\r\nTimer pointlessThread(2, PointlessPublish);\r\n\r\nint Uptime( String unused )\r\n{\r\n  return millis() / 1000;\r\n}\r\n\r\nvoid setup()\r\n{\r\n  Serial.begin(9600);\r\n\r\n  WiFi.on();\r\n  WiFi.connect(); \r\n  Particle.connect();\r\n\r\n  //i2c and PE stuff\r\n  Wire.begin();\r\n\r\n  myPE.begin(0);\r\n  myPE.pinMode(0,OUTPUT);\r\n  myPE.pinMode(1,INPUT);\r\n\r\n  Particle.variable(\"i2cErrors\", numI2cErrorsSoFar);\r\n\r\n  Particle.function(\"uptime\", Uptime);\r\n  \r\n  Serial.println(\"Example program with publishes + i2c\");\r\n\r\n  pointlessThread.start();\r\n}\r\n\r\n\r\nvoid loop()\r\n{\r\n  Particle.process();\r\n\r\n  PointlessPublish();\r\n\r\n  peState = myPE.digitalRead(1);\r\n  myPE.digitalWrite(0,peState); \r\n}\r\n````\r\n\r\n##### Partially modified MCP23017 driver implementation for this test\r\n\r\n````cpp\r\n/*************************************************** \r\n This is a library for the MCP23017 i2c port expander\r\n\r\n These displays use I2C to communicate, 2 pins are required to\r\n interface\r\n Adafruit invests time and resources providing this open source code,\r\n please support Adafruit and open-source hardware by purchasing\r\n products from Adafruit!\r\n\r\n Written by Limor Fried/Ladyada for Adafruit Industries.\r\n BSD license, all text above must be included in any redistribution\r\n ****************************************************/\r\n\r\n\r\n#include \"Adafruit_MCP23017.h\"\r\n\r\n...\r\n\r\nextern int numI2cErrorsSoFar; \r\n\r\nvoid ReportI2cError()\r\n{\r\n\tnumI2cErrorsSoFar++;\r\n\tSerial.println(\"numI2cErrorsSoFar = \" + String(numI2cErrorsSoFar) );\r\n}\r\n\r\n...\r\n\r\n/**\r\n * Reads a given register\r\n */\r\nuint8_t Adafruit_MCP23017::readRegister(uint8_t addr){\r\n\t\r\n\tstatic int result = 0;\r\n\t// read the current GPINTEN\r\n\tWire.beginTransmission(MCP23017_ADDRESS | i2caddr);\r\n\twiresend(addr);\r\n\r\n\tSINGLE_THREADED_BLOCK()\r\n\t{ \r\n\t\tresult = Wire.endTransmission();\r\n\t}\r\n\r\n\tif(result != 0)\r\n\t{\r\n\t\tSerial.println(\"readRegister endTransmission(): \" + String(result) );\r\n\t\tReportI2cError();\r\n\t\tWire.reset(); \r\n\t}\r\n\r\n\tSINGLE_THREADED_BLOCK()\r\n\t{\r\n\t\tresult = Wire.requestFrom(MCP23017_ADDRESS | i2caddr, 1);\r\n\t}\r\n\r\n\tif(result == 0)\r\n\t{\r\n\t\tSerial.println(\"readRegister requestFrom() data: \" + String(result) );\r\n\t\tReportI2cError();\r\n\t\tWire.reset(); \r\n\t}\r\n\r\n\treturn wirerecv();\r\n}\r\n\r\n\r\n/**\r\n * Writes a given register\r\n */\r\nvoid Adafruit_MCP23017::writeRegister(uint8_t regAddr, uint8_t regValue){\r\n\t\r\n\tstatic int result = 0;\r\n\r\n\t// Write the register\r\n\tWire.beginTransmission(MCP23017_ADDRESS | i2caddr);\r\n\twiresend(regAddr);\r\n\twiresend(regValue);\r\n\r\n\tSINGLE_THREADED_BLOCK()\r\n\t{\r\n\t\tresult = Wire.endTransmission();\r\n\t}\r\n\r\n\tif(result != 0)\r\n\t{\r\n\t\tSerial.println(\"writeRegister endTransmission(): \" + String(result) );\r\n\t\tReportI2cError();\r\n\t\tWire.reset(); \r\n\t}\r\n}\r\n````\r\nTo try different levels of protection, comment out `SINGLE_THREADED_BLOCK` or replace it with `ATOMIC_BLOCK`. \r\n\r\n#### Results for different levels of protection\r\n\r\n1. No protection (commenting out `SINGLE_THREADED_BLOCK` or `ATOMIC_BLOCK`): 7 errors after 30 minutes\r\n2. `SINGLE_THREADED_BLOCK`: 0 errors after 932 minutes\r\n3. `ATOMIC_BLOCK`: 0 errors after 960 minutes   \r\n\r\n#### Within a real program    \r\n\r\nWe observed that while `SINGLE_THREADED_BLOCK()` may be sufficient for most applications in preventing i2c transactional errors, within our company's program (which uses i2c, SPI, Serial, WiFi, swTimers...) all together, it still resulted in occasional errors. However, when we changed to `ATOMIC_BLOCK()` these errors disappeared in all overnight tests on multiple units. The devices appear to be running happily, even with interrupts disabled during i2c transactions. \r\n\r\n---\r\n\r\nCompleteness:\r\n- [x] Minimum test case added\r\n- [x] Device, system and user firmware versions stated\r\n- [ ] Particle confirmed"},"comment":{"url":"https://api.github.com/repos/spark/firmware/issues/comments/227832356","html_url":"https://github.com/spark/firmware/issues/1042#issuecomment-227832356","issue_url":"https://api.github.com/repos/spark/firmware/issues/1042","id":227832356,"user":{"login":"technobly","id":2249224,"avatar_url":"https://avatars.githubusercontent.com/u/2249224?v=3","gravatar_id":"","url":"https://api.github.com/users/technobly","html_url":"https://github.com/technobly","followers_url":"https://api.github.com/users/technobly/followers","following_url":"https://api.github.com/users/technobly/following{/other_user}","gists_url":"https://api.github.com/users/technobly/gists{/gist_id}","starred_url":"https://api.github.com/users/technobly/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/technobly/subscriptions","organizations_url":"https://api.github.com/users/technobly/orgs","repos_url":"https://api.github.com/users/technobly/repos","events_url":"https://api.github.com/users/technobly/events{/privacy}","received_events_url":"https://api.github.com/users/technobly/received_events","type":"User","site_admin":false},"created_at":"2016-06-22T18:19:40Z","updated_at":"2016-06-22T18:19:40Z","body":"Wow!  Thanks for the great detail @pomplesiegel :)\r\n\r\nWe have confirmed that if a thread blocks context switching such that the 100ms timers in i2c_hal.c are exceeded before the events become true, the transaction will fail.\r\n\r\nWe had a bit of trouble with your example, I think possibly because of the excessive publishing rate (260-500ms).  As a test, would you try replacing your PointlessPublish() routine with this one?\r\n\r\n```\r\nvoid i2cThreadBlocker() {\r\n    uint32_t block = random(0, 300);\r\n    {\r\n        SINGLE_THREADED_SECTION();\r\n        uint32_t startMillis = millis();\r\n        while ((millis() - startMillis) < block);\r\n    }\r\n}\r\n```\r\n\r\na simple fix would be altering the i2c_hal.c timer checks as follows:\r\n```\r\nif (EVENT_TIMEOUT < (HAL_Timer_Get_Micro_Seconds() - _micros) &&\r\n  I2C_GetFlagStatus(i2cMap[i2c]->I2C_Peripheral, I2C_FLAG_BTF) == RESET) { // i2c sw reset }\r\n```\r\n\r\nso that as another thread is blocking the ability to check the flag, when it returns to the i2c_hal thread the timeout qualifies the flag again as well.  The flag should be SET and the i2c peripheral will not be reset due to the timer being expired already."}}
{"actor":{"display_login":"technobly"}}