search

technoblogy / tiny-i2c

Minimal I2C master routines for all AVR microcontrollers.
161 stars 39 forks source link

Bug: read fails due to missing parenthesis #3

Closed maxint-rd closed 5 years ago

maxint-rd commented 5 years ago

Hello David,

First I'd like to thank you for making this library. For an ATtiny44A DS3231 LED matrix clock project I needed to use I2C while having only few bytes of the 4K flash available. The ATtinyCore has the Wire library which works fine, but uses too much flash and RAM memory for my project. So looking for a minimal I2C implementation I found your blog and tried your library. It compiled fine (Arduino IDE 1.8.2) but unfortunately it wouldn't read the I2C RTC. Using a logical analyzer I saw that while the initial time setting taken from your example went okay, reading the time failed. It did set the proper device ID to initiate the read, but then no more clock pulses appeared for the actual reading.

After spending way too much time goofing around, I finally looked at your code and noticed a peculiar difference between the read and the write function when enabling SDA as input. The code on line 61 differs just a tiny bit from the similar code on line 83. It seems that parenthesis were missing. I put them back and now everything worked fine! So on line 61 it first read: DDR_USI &= ~1<<PIN_USI_SDA; // Enable SDA as input. I made it into this code: DDR_USI &= ~(1<<PIN_USI_SDA); // Enable SDA as input. (MMOLE: copied parenthesis from write() line #83)

As all other code seems quite well written, I like to believe that while you had it working on your local test environment, you uploaded a cleaned-up version. As I don't have any experience with writing USI code, using your code saved me a lot of time. Thank you once more!

technoblogy commented 5 years ago

Sorry for the delay in responding - I had the notification settings set up incorrectly on GitHub and so didn't get an email notification when you originally posted the Issue (fixed now).

Thank you for finding the bug. I tested the library on the ATtiny85 but on that chip PIN_USI_SDA is 0 so the missing brackets don't cause a problem, and it worked fine. However, as you report it prevents reads working on an ATtiny44/ATtiny84.

I've now checked that the library works with the ATtiny85, ATtiny84, and ATtiny2313, and I believe it should work on the other ATtinys with a USI too.

maxint-rd commented 5 years ago

Great! You're correct. I noticed the problem on an ATtiny44A while making the scrolling LED clock using my TM16xx library. Later I needed more memory and used the modified version on an ATtiny85, but as you said, that probably would have worked fine anyways.

Since you incorporated the fix, I've closed the pull request.

(BTW. That notification issue happened to me as well. Took me several months before I found out...)