Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.23k stars 216 forks source link

Attiny85 I2C support #301

Open rhblind opened 1 year ago

rhblind commented 1 year ago

Hello, Please forgive my ignorance, I'm pretty new to Rust and embedded programming as a whole, so this might (or might not) be a silly question.

I'm trying to program an Attiny85 using the Trinket examples. However, I'd like to add I2C Slave support for communication. Looking at the source code for the Attiny chips in attiny-hal directory, I see that the Atmega has a source file for I2C, while the Attiny doesn't.

I have tried comparing with the examples in uno-i2cdetect.rs, to see if I could make it work, but it won't compile.

Is it something fundamentally I don't get, or is I2C simply not supported for the Attiny family?

Best regards :)

Rahix commented 1 year ago

Hi,

unfortunately, I2C is not yet supported in attiny-hal. The ATtiny chips use a different peripheral for I2C communication. It is called USI (Universal Serial Interface) and works differently than the TWI (Two Wire Interace = I2C) peripheral on ATmega chips. So what is needed is someone implementing a driver for it...

rhblind commented 1 year ago

Hello, Right, thank you for the explanation. That makes sense :)

Rahix commented 1 year ago

Let's keep this issue open to track progress on the driver implementation.

Palladinium commented 1 year ago

I've been working on implementing an I2C driver for the Attiny family, and I think that unfortunately the generic traits as defined in avr-hal-generic are not generic enough to work with the USI.

The problem I'm finding is that the I2cOps trait only gives access to implementors to the periperal through the self parameter, but not the pins.

For the Atmega family, this isn't an issue as the SDA pin stays in input mode and is driven separately by the TWI, but for the USI output driver to control the SDA pin the corresponding data direction register bit must be set. In other words, the pin needs to be switched between input and output mode during different steps of I2C communication. In this library, it looks like I should be using the into_output/into_input function on the pin, which aren't accessible from the I2cOps members such as raw_start.

Furthermore, this makes typing and borrowing complicated as the type of the pin structs changes throughout the i2c flow, and while there is a structure for a type-erased runtime pin identifier (Dynamic), there doesn't seem to be a similar structure for the pin mode. into_output and similar functions move the pin, which can't be done through the I2c methods since they take a mutable reference to I2c, and therefore to the pins within.

For more context, see the chapter on USI on any of the Attiny datasheets (I've been using an Attiny84, but it's very similar in the 85), and also compare the application notes AVR310 (Using the USI module as a TWI Master) and AVR315 (Using the TWI Module as I2C Master), as well as their example C source code. Note how AVR315 basically never changes the DDRx register after initialization, while AVR310 sets the SDA bit in DDR_USI (which is just its matching DDRx register) in a few places during communication.

I believe that in order to support I2C on the Attiny family, quite likely breaking changes need to be made to the generic I2C structs in order to better support USI, at least to be able to modify SDA's mode at runtime within I2cOps. Alternatively, a separate type for I2C through USI could be added, though the loss of generality is undesirable. Either way, having a pin mode that type-erases the information to a runtime check rather than compile-time would be very useful too.

Palladinium commented 1 year ago

I've also found an issue where the bit patterns for the USIWM bits in USICR are incorrectly labelled. According to the USIWM_A enum, a value of 2 (USIWM1 set, USIWM0 unset) puts the USI in two-wire slave mode, while a value of 3 (both USIWM1 and USIWM0 set) puts it in two-wire master mode. You can find these values by searching for USIWM_A in the rustdocs.

From my understanding of the datasheet, as well as application notes AVR310 and AVR312 (reference implementations of master and slave i2c respectively), the two patterns are not actually referred to as master or slave modes, however their reference master implementation uses only bit pattern 2 (the one the rust enum calls slave mode), meanwhile their slave implementation uses both bit patterns 2 and 3 during different parts of the communication.

The only difference in behavior between the two modes is that with bit pattern 3 SCL is held low after an overflow of the counter register - something normally done by the slave (the rust code calls this "master" mode).

I think calling these patterns master and slave is a bit misleading in the first place, so perhaps wholly renaming the two variants to more accurate names would be good. Perhaps two_wire and two_wire_with_counter_overflow_hold.

I'm not actually sure where the name itself comes from - the enum is defined in avr-device, but it seems to come from generated code, so I'm not sure where such a change would need to be done.

lopsided98 commented 11 months ago

I just wrote a quick and dirty I2C master driver using USI: https://github.com/lopsided98/acurite-thermometer/blob/main/acurite-thermometer/src/i2c.rs. It is pretty closely based on AVR310, but is missing any and all delays because I only run my ATtiny85 at 1 MHz, making it slow enough already. I also don't wait for SCL to go high, since there is no way to read the PINx value of an output pin with avr-hal. I solved the dynamic pin mode problem using an enum that can store either an input or an output pin, allowing me to move the pin between the two types. This probably has some overhead, but I haven't checked the assembly.

A better solution would be to implement the embedded_hal::digital::v2::IoPin trait, which is currently guarded behind the unproven feature.

I agree that the USIWM bits are incorrectly named. The names come from an SVD patch in avr-device, see: https://github.com/Rahix/avr-device/blob/main/patch/common/tiny/usi.yaml. The original descriptions from the ATDF are correct, if a little terse; I'm not sure why they needed to be changed: https://github.com/Rahix/avr-device/blob/7631be88f686fab89e008dfa39bd13559909aefb/vendor/attiny85.atdf

Palladinium commented 11 months ago

I gave this my best shot a few weeks ago, be warned that this compiles but doesn't work: https://gist.github.com/Palladinium/77e8101efa73777fb8290198c24ebe38 It relies on a small patch to avr-hal to expose the raw pin, which is included in the gist. My implementation is for an Attiny84, but as far as I know it should all be the same for an Attiny85, just with the pin assignments moved.

While from my testing it works fine on its own, as soon as I start doing anything else alongside it (as little as doing IO on other pins or using PWM) the MCR starts behaving very erratically and unpredictably, including restarting itself at arbitrary times, hanging, or not running at all.

The behavior is consistent until a code change and recompile, and changes in completely unrelated ways to what was changed. For instance, one version where I pull some pins up and down a few times before and after I2C transmissions to light some LEDs for debugging will hang during I2C transmission, and after just changing the order the LEDs are lit or the number of times an LED is blinked will turn it into a restart loop at a line before the I2C is even initialized. Changing something else similarly minor will make the whole MCU do nothing at all.

I double-checked and excluded likely sources of inconsistency, like weak power rails, nearby electrical noise, faulty parts, etc. to the best of my (limited) ability. I eventually rewrote my firmware in C using an existing I2C implementation, and it all behaves correctly in the same electrical environment, with the same parts.

This all makes me think that my code is doing something deeply unsound, and unfortunately I can't spot it and I'm not fluent enough in avr assembly to figure out what's happening from looking at the disassembly. Best of luck to anyone attempting this, I might revisit it some time but I'm a bit too burned out on it from staring at my code too long at the moment.