rambo / TinyWire

My modifications to TinyWire Arduino libs
284 stars 121 forks source link

Surprise, as of f0eb04f the communication doesn't work anymore, only 255 response #44

Open Seeelefant opened 6 years ago

Seeelefant commented 6 years ago

Surprise, as of f0eb04f the communication doesn't work anymore. Master (Arduino Micro):

#include <Wire.h>

void setup() {
  Wire.begin();
  Serial.begin(9600);
}

void loop() {
  Wire.requestFrom(10, 4);    // request 4 bytes

  while (Wire.available()) {
    uint8_t c = Wire.read();
    Serial.print(c, HEX);
  }
  Serial.println();

  delay(500);
}

Slave ATtiny85

#include <TinyWireS.h>

byte own_address = 10;

volatile uint8_t i2c_regs[] =
{
  0xDE, 
  0xAD, 
  0xBE, 
  0xEF, 
};
volatile byte reg_position = 0;
const byte reg_size = sizeof(i2c_regs);

void setup() {
    TinyWireS.begin( own_address );
    TinyWireS.onRequest( onI2CRequest );
  TinyWireS.onReceive( onI2CReceive );
}

void loop() {

}

void onI2CRequest() {
  TinyWireS.send(i2c_regs[reg_position]);
  reg_position++;
  if (reg_position >= reg_size)
  {
    reg_position = 0;
  }
}

void onI2CReceive(uint8_t howMany) {
  if (howMany < 1)
  {
    return;
  }
  if (howMany > reg_size+1)
  {
    return;
  }

  reg_position = TinyWireS.receive();
  howMany--;
  if (!howMany)
  {
    return;
  }
  while(howMany--)
  {
    i2c_regs[reg_position] = TinyWireS.receive();
    reg_position++;
    if (reg_position >= reg_size)
    {
      reg_position = 0;
    }
  }
}

Output with most recent commit (returns always 255 aka "nothing"): image Output with f0eb04f (returns first byte correctly, the remaining bytes are wrong): image

See also https://arduino.stackexchange.com/questions/48625/always-255-response-in-i2c-between-attiny85-8mhz-and-arduino-uno and https://forum.pjrc.com/threads/48887-HELP-Teensy-3-5-I2C-ATTiny85-20PU-(TinyWireS-OnRequest-not-triggering!)

gpsgui commented 6 years ago

I'm getting the same issue, when I request 2 bytes (from the arduino master) i got 255 and -1(?).

OlavGullaksen commented 6 years ago

I have exactly the same issue - it looks like the TinyWireS.onRequest function is not triggered at the Attiny85 having software serial running at the Attiny85 for debugging.

OlavGullaksen commented 6 years ago

Using an older version from here: https://github.com/rambo/TinyWire/tree/860ad85572dbe835391f99bc3f72a13504bbfc4e makes my simple test run and the onRequest is triggered when called from master as it should be. Conclusion: the newest TinyWireS library on Github is corrupt.

Seeelefant commented 6 years ago

Hard to believe, but seems to be the truth....unfortunately. Take that https://github.com/nadavmatalon/TinyWireS, checked it yesterday, it´s working .

rambo commented 6 years ago

I made branch rollback on commit 860ad85572dbe835391f99bc3f72a13504bbfc4e the default branch for now. Haven't had the time to properly test everything so I've been trusting the PRs for new features don't break old ones...

Seeelefant commented 6 years ago

Hi rambo, thanks for your efforts. Isnt´t there an option to include unit tests like here https://github.com/iNavFlight/inav/tree/master/src/test/unit? I really spent many hours on that issue.

rambo commented 6 years ago

Unit tests for external communication ? By definition no. Now integration tests, those would be a good idea, they'd need some sort of jig though (one with standard arduino running the tests and and attiny as the Device Under Test) and of course the code for the tests themselves (including something [raspberry pi?] to flash different test firmwares on both MCUs to test different things). Integration testing is always a bit complicated to automate...

Seeelefant commented 6 years ago

Hi rambo, well of course in the strong sense you are absolutely right, but wouldn´t it be an option to use an AVR simulator like https://sourceforge.net/projects/avrsimu/ , https://www.mikrocontroller.net/articles/AVR-Simulation or the one inside ATMEL Studio?

rambo commented 6 years ago

Maybe, if someone has implemented a simulator of the USI peripheral on the ATTiny (with all it's ~bugs~ features true to life). I expect that for me at least it would be easier and faster to build the real integration test jig than to learn all about those simulators and verify the simulated peripherals are accurate and then figure out the automation (especially atmel studio is probably not remote-controllable for automatic testing without adding some external RPA components to the mix)

Of course everyone is welcome to try and do it, I unfortunately won't have the time at least before my summer vacations (july).

Seeelefant commented 6 years ago

Hi Rambo, completely understood, unfortunately I am new to the world of microcontrollers. The analysis of the bug took me quite some time, but I think the discussion was highly valuable. Maybe another person has the right knowledge? Drop me a line if I can help!

dersimn commented 6 years ago

So what’s the latest status? The rollback branch works if you don’t use software serial for debugging, or?

bmbeyers commented 5 years ago

I had this problem too trying to request data from ATtiny85 over I2C, and (eventually) noticed that the USI_REQUEST_CALLBACK() function was only being called if the master sent an ACK after the requested data was supposedly sent in the usiTwiSlave.c file. If you move this line from the USI_SLAVE_CHECK_REPLY_FROM_SEND_DATA case in the USI Overflow ISR to the top of the USI_SLAVE_SEND_DATA case, it should work. Or at least it did for me. Note that if your onRequest routine takes some time, you might need to implement some sort of hold on the SCL line before you are ready.

dersimn commented 5 years ago

Can you create a fork fixing this problem?

nablabla commented 5 years ago

Hi I downloaed master and I got the corrupted version (with USI_REQUEST_CALLBACK() down and always receiving 255 and requestFrom does not work) So your revertind did not work, or your new fix does not work or you new release contains the old corrupted code. So this issue is still not fixed

rambo commented 5 years ago

argh can anyone point out a tested-to-work -commit I can roll back to ? I don't have the time to play around with this in the foreseeable future.

nablabla commented 5 years ago

Well, now requests don't work at all, you can not make it any worse. If you just follow bmbeyers suggestion it does work. I tested it with my arduino and attiny85@8mhz

i.e. undo what happens here: https://github.com/rambo/TinyWire/commit/f0eb04fb699615d83fac3ead610cade7608d9b9e

dersimn commented 5 years ago

How about generating a pull request out of your changes? @bmbeyers, @nablabla

rambo commented 5 years ago

PR would be even better than a commit-id for rollback.

samw3 commented 4 years ago

Did anyone make a PR? Has this been fixed. Just discovered this open issue after being frustrated for days.

bmbeyers commented 4 years ago

Can't say I have ever made a pull request before, but I can give it the ol' college try. I'll take a look and see what I did 2 years ago to get it to work. If you need a quick fix, see my earlier comment and adjust the source code. I believe it is just moving a single function call from one select case option to another.

ChrisHul commented 2 years ago

I've spent some time looking at this problem because I was unable to make ANY TinyWire work. The main issue is that I2C specification does not allow SCL stretching between the data byte and ACK. This means the ATTiny needs to act very quickly to setup the ACK reading after the data byte has been shifted. Even a minor change in the software may have an impact on performance. I also discovered that the clock was released concurrently with the USI counter loading which in my view is compromising. I adapted a master bitbang software to accept SCL stretching at any time which allows sending streams of data in both directions. It still has a failing ratio of 0.1 % in master to slave transfer, but it should be ok with an extra layer of CRC and handshaking on top. I'm unable to explain why these errors occur. For anyone interested in this setup: https://github.com/ChrisHul/TinyWireSetup

Seeelefant commented 2 years ago

Thank you Chris, maybe you can take a look at https://github.com/nadavmatalon/TinyWireS, this was working in my tests.

ChrisHul commented 2 years ago

Ok, glad that you got your setup working. I tried a few libraries with no success. Looking into them, they are all pretty similar. I'm running the internal 8 MHz clock in the attiny. That may explain the trouble. Still think it's a timing issue which can be triggered by small software changes.

ghost commented 2 years ago

@ChrisHul, timing is certainly something that needs careful consideration and the ATtiny definitely has its quirks but I've managed to run things reliably (across multiple devices running for months continuously) down to 1MHz on an ultra-low-power (lots of sleeping) met project with wind-vector averaging. I think in the end, for me, a timeout was the way to go.

ChrisHul commented 2 years ago

@acicuc, 1MHz certainly sounds challenging to me, but I guess it's a matter of bit-rate, clock-stretch detection and delays. It isn't clear to me if the I2c protocol allows clock-stretch on START condition event, which would be useful on processor wake-up. The common TinyWire library will support it though.

ghost commented 2 years ago

@ChrisHul, I should add I had to modify (timeout+other mods) the samd (master-side) arduino SERCOM library to get things humming nicely for my project. I submitted a pull for that but I guess it may have looked a bit much to digest in one go. I had also started on testing (the ATtiny85) with a pi but was diverted to other tasks and then didn't end up getting back to it. But the ability of the master to handle clock stretching is a must when using the ATtiny as a slave - especially when running way down at 1MHz!

ChrisHul commented 2 years ago

Ok @acicuc. The timeout extension probably solved the wake-up delay issue. The main culprit is still the data/ack transition though. Should you ever pick up the "pi" project again, going bitbang may be an option for you. Larry Bank's repository provides support for that and my upload has the clock stretch enabling mods.