TWI/I2C Problems

Discussions relating to development and use of the OpenServo software/firmware.

Moderators: jharvey, Secondary Admin, Admins

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

TWI/I2C Problems

Post by mpthompson » Wed Jan 25, 2006 9:53 pm

Andy sent me the following over IM:
Andrew Lippitt: I've been looking very closely at the read write errors that I'm getting on the I2C bus. The reads from the servo often yield data where at some random bit, all remaining bits are 1. So when I'm expecting 0x0200 I receive 0x02FF. While this particular one is the most frequent, it seems like it might be able to start on any bit, not just byte boundries. I've seen 0x0201, 0x05FF. As for the writes, there is a different problem. The value that is written is often bit shifted right by 1. I seen other values I can't make sense of, but may be things like addresses or other data I've not recognized yet that has been shifted by some other amount. This happens with both the old TWI code and the new. I've varied the clock speed quite a bit and introduced inter-byte delays to no effect.
Any one else have thoughts on this.

We tracked down an issue with the SDA pull-up resistor being enabled on the servo that we thought could cause problems, but further testing from Andy indicated the problem lies elsewhere.

Andy is going to continue to poke at this to try and find the culprit. I'll start working on this on Thursday if Andy hasn't found anything by then.

My leading theory is line noise caused by the h-bridge/motor as I've noticed similar issues when the motor is active. However, Andy's testing indicates the problem exists even when PWM is turned off. Maybe we are chasing multiple issues.

-Mike

andylippitt
Site Admin
Posts: 155
Joined: Mon Jan 02, 2006 11:19 pm
Location: Denver, CO
Contact:

Post by andylippitt » Wed Jan 25, 2006 10:27 pm

At Mike's suggestion I poked around with the ADC interrupts. By poking around I mean completely and totally breaking. As he speculated, this cleared up the I2C issues. Seems there's some interrupt priority issue or a peripheral conflict. I'll keep poking (breaking). :)

andylippitt
Site Admin
Posts: 155
Joined: Mon Jan 02, 2006 11:19 pm
Location: Denver, CO
Contact:

Post by andylippitt » Wed Jan 25, 2006 11:37 pm

Ah-HA! We beat it. There's a new twi.c checked in that clears it up.

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

Post by mpthompson » Thu Jan 26, 2006 8:53 am

Andy, I checked in some further enhancements to pwm.c to correctly disable and restore interrupts when setting the PWM signals coming from PortB output pins PB1 and PB4. I'm 99% certain these changes shouldn't cause your I2C problem to reappear, but let me know if you see any issues again.

Thank you for all your help in tracking down this issue.

-Mike

andylippitt
Site Admin
Posts: 155
Joined: Mon Jan 02, 2006 11:19 pm
Location: Denver, CO
Contact:

Post by andylippitt » Fri Jan 27, 2006 6:15 pm

After going back and forth a few times, we settled on what seems to be a pretty solid build. With the motors turned off, there are almost zero errors in communication. With them on, the noise is manageable. To help cope with this noise that I doubt we'll find a way to completely eliminate, I stole back all those image size gains Mike made, by adding read and write transactions with checksums. These live along side the regular addressed reads and writes.

Since I'm making up my I2C protocol graph format here, I'll show standard read/writes for comparison.

A Standard read goes as follows:

Code: Select all

Start, I2CAddress, RegisterAddress, Stop
Start, I2CAddress+READ, Read[0], Read[1] ... Read[Count], Stop
A ""checked read"":

Code: Select all

Start, I2CAddress, TWI_CMD_CHECKED_TXN, ByteCount, RegisterAddress, Stop
Start, I2CAddress+READ, Read[0], Read[1] ... Read[Count], Read[Checksum], Stop
A Standard write:

Code: Select all

Start, I2CAddress, RegisterAddress, Write[0], Write[1] ... Write[Count], Stop
A ""checked write"":

Code: Select all

Start, I2CAddress, TWI_CMD_CHECKED_TXN, ByteCount, RegisterAddress, Write[0], Write[1] ... Write[Count], Write[Checksum], Stop
During the checked write, if the checksum does not match, the servo wil NAK when receiving the checksum byte.

These are simple checksums, start at zero/add/wrap at 256. They are prone to added or dropped zero bytes, byte reordering or multiple errors that cancel each other out. Both checked transactions are limited to 16 data bytes. If the write checksum comparison fails, the registers will not be committed to memory and the buffer discarded.

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

Post by mpthompson » Fri Jan 27, 2006 9:31 pm

I asked Andy to just use 16 bytes for the checked read/write commands for right now as we are getting tight on SRAM -- the ATtiny45 only has 256 bytes of SRAM. We can increase the buffer size once we are able to move to the ATtiny85 with 512 bytes of SRAM.

-Mike

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

Post by mpthompson » Sat Jan 28, 2006 10:10 am

I took Andy's checksum code and rearranged it so that it is now enabled or disabled at compilation time using a TWI_CHECKED_ENABLED define near the top of the file. With the checksum code we are very close to the end of the 3K of application Flash space, but with it disabled we get back nearly 300 bytes for experiments in other parts of the code.

I may have broke something in the code while making these changes. I'll verify them over the weekend and hopefully get fixes in before it causes a problem for anyone.

-Mike

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

Post by mpthompson » Sat Jan 28, 2006 6:16 pm

I backed Andy's original changes back into CVS as I ran into problems with my changes to them. I'll be looking at this today.

-Mike

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

Post by mpthompson » Sat Jan 28, 2006 8:06 pm

OK, I think I was able to fix everything. As best I can determine the stack growing down from the top of SRAM collided with global variables causing all sorts of nastiness. I reduced the size of the global variables by reducing the register array from 64 to 48 bytes and reducing the size of some other buffers. I also made a few other changes to gain back some Flash space as well.

I believe the code checked into CVS at this time should be functioning correctly. I'll be monitoring the forum in case anyone finds anything that still looks broken.

-Mike

andylippitt
Site Admin
Posts: 155
Joined: Mon Jan 02, 2006 11:19 pm
Location: Denver, CO
Contact:

Post by andylippitt » Sat Jan 28, 2006 8:38 pm

I think the problem is in your reworking of my state vars. I was using TWI_DATA_STATE_CHECKED_COUNTING as your revised TWI_DATA_STATE_CHECKED_DATA, but on line 181 in twi_read_data() your using my definition instead of your updated one.

My only other concern is the default register read in twi_read_data(). It will be called with twi_address advanced one beyond the intended read when it comes time to send the checksum. The value is discarded, but it seems like it's waiting to become a problem if more safetys are built into the register reading.

andylippitt
Site Admin
Posts: 155
Joined: Mon Jan 02, 2006 11:19 pm
Location: Denver, CO
Contact:

Post by andylippitt » Sat Jan 28, 2006 8:41 pm

looks like you beat me to it :)

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

Post by mpthompson » Sun Jan 29, 2006 4:52 am

Something I noticed about using checksums for read and write to the registers is that the address field is unprotected. Perhaps we should modify the protocol as follows:

Checked Read

Code: Select all

Start, I2CAddress, TWI_CMD_CHECKED_TXN, ByteCount, RegisterAddress, Checksum, Restart, I2CAddress+READ, Read[0], Read[1] ... Read[Count], Read[Checksum], Stop
Checked Write

Code: Select all

Start, I2CAddress, TWI_CMD_CHECKED_TXN, ByteCount, RegisterAddress, Write[0], Write[1] ... Write[Count], Write[Checksum], Stop
As indicated above, in a checked read the master would checksum the byte count and register address (which is just retransmitting it I guess). For a checked write the checksum would just extend over the byte count and register address as well as the written bytes.

Since we are providing a minimal level of protecting the data we should consider protecting the byte count and address as well.

-Mike

andylippitt
Site Admin
Posts: 155
Joined: Mon Jan 02, 2006 11:19 pm
Location: Denver, CO
Contact:

Post by andylippitt » Sun Jan 29, 2006 5:02 am

Agreed. Are you going to make the changes or would you like me to?

mpthompson
Posts: 651
Joined: Mon Jan 02, 2006 11:38 pm
Location: San Carlos, CA
Contact:

Post by mpthompson » Sun Jan 29, 2006 5:26 am

Why don't you go ahead.

-Mike

andylippitt
Site Admin
Posts: 155
Joined: Mon Jan 02, 2006 11:19 pm
Location: Denver, CO
Contact:

Post by andylippitt » Sun Jan 29, 2006 8:20 pm

Tested and checked in with some differences from Mike's suggestion above. The protocol remains as I originally had it, but the checksums include the bytecount and address values.

Post Reply

Who is online

Users browsing this forum: No registered users and 0 guests