[#90] I2C slave driver

[ToddG]

Draft PR.

Closes #90.

[ToddG]

@Rahix -- I left a bunch of random questions in here as to the current I2C Master implementation.

[ToddG]

I added a bunch of datasheet excerpts as comments as a way to help me understand what's going on. Personally, I like lot's of explanatory comments, but of course, it's up to you to decide if you want to keep them in the final PR.

[Rahix]

I quite like what the kernel has to say about commenting: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

I.e. the HOW should be obvious from the code and shouldn't need a datasheet excerpt alongside it. For tricky stuff, mentioning the relevant section of the datasheet might make sense but I'd never copy the contents into the code -> if I need them, I can just open the data-sheet.

[ToddG]

Funny, I was coming around to this opinion myself in the past few days. Adding in the comments helped me think about the problem for awhile, but then they started getting in the way. It would be nice if the datasheets were deep-linkable, so that I could reference the line number of the spec. I'll check in my latest changes in a few minutes.

[Rahix]

It would be nice if the datasheets were deep-linkable

Yeah, it really would ... Best one can do is name the section and revision of the datasheet (because some lunatics think it is a good idea to renumber the sections from time to time ...)

Adding in the comments helped me think about the problem for awhile, but then they started getting in the way.

In the C world some people have adopted a nice method: Use // comments for any temporary in-development comments while thinking/developing/debugging. Use /* ... */ comments for "production" comments that are meant to stay. Then, before submitting changes, remove all // comments.

In Rust, this doesn't really work because people just use // for all comments everywhere. So I personally have started to just use some comment prefix to mark 'dev-comments', e.g. // WIP: ....

[ToddG]

+1 on the // WIP : ... I typically add either // TODO ... or // BUGBUG ... but I'll adopt WIP for temp in dev comments going forward.

[ToddG]

Added a static method for initializing TWI.

[Rahix]

This is a good idea! Though maybe put it into the existing impl block further down instead of adding a second one up top? You could also take the pattern from the serial module and make the initializer a method instead of a static function:

avr-hal/avr-hal-generic/src/serial.rs

Lines 139 to 173 in 7d851d5

	pub fn new(
	p: $USART,
	rx: $rxmod::$RX<$crate::port::mode::Input<RX_MODE>>,
	tx: $txmod::$TX<$crate::port::mode::Output>,
	baud: Baudrate<CLOCK>,
	) -> $Usart<CLOCK, RX_MODE> {
	let mut usart = $Usart {
	p,
	rx,
	tx,
	_clock: ::core::marker::PhantomData,
	};
	usart.initialize(baud);
	usart
	}
	fn initialize(&mut self, baud: Baudrate<CLOCK>) {
	self.p.[<ubrr $n>].write(|w| unsafe { w.bits(baud.ubrr) });
	self.p.[<ucsr $n a>].write(|w| w.[<u2x $n>]().bit(baud.u2x));
	// Enable receiver and transmitter but leave interrupts disabled.
	self.p.[<ucsr $n b>].write(|w| w
	.[<txen $n>]().set_bit()
	.[<rxen $n>]().set_bit()
	);
	// Set frame format to 8n1 for now. At some point, this should be made
	// configurable, similar to what is done in other HALs.
	self.p.[<ucsr $n c>].write(|w| w
	.[<umsel $n>]().usart_async()
	.[<ucsz $n>]().chr8()
	.[<usbs $n>]().stop1()
	.[<upm $n>]().disabled()
	);
	}

[ToddG]

Cool, will do.

[ToddG]

Why do we need the wait() here?

[Rahix]

Register bits don't necessarily act like memory cells. The following things which you might assume from a memory cell do not always apply for register bits:

• Reading a register twice without any writes in between will yield the same value.

  Counter Examples:

  • A FIFO register will return the next byte from the internal buffer on each read. I.e. the first read will yield the first byte, the second read will yield the second byte, etc.
  • Sometimes reading a bit will reset it back to zero.

• Writing a value to a register and reading it back will give back the same value.

  Counter Examples: In hardware, the logic that receives data written to a register is entirely disjunct from the logic presenting data when the register is read. There are many different ways in which this shows:

  • Sometimes only a subset of bits in the register are writeable (=read-only). All other bits from the written value will be ignored.
  • Some bits are write-only and reading will always return 0.
  • Sometimes writing a 1 to a bit will reset it back to zero.
  • Sometimes a bits value is entirely dependent on hardware state and writing it has a completely different effect than what reading is for (bits are "reused" for two different purposes).

---

In this specific case, we write twint to 1 which will clear the flag:

The TWINT Flag must be cleared by software by writing a logic one to it.

Then we wait() until hardware sets it again.

[ToddG]

Great comment. Do you mind if I capture this in a Newbie FAQ?

[Rahix]

Well, this is true for the entirety of embedded software development so I don't think avr-hal is the right place for it. But if you want to post it somewhere else, please go a head :)

[ToddG]

Gotcha. I'll probably post a summary of this experience on my blog. No one reads it, but perhaps it'll help someone someday...probably me in 6 months :-)

[ToddG]

I like the class state machine pattern in rust. Just showing what lpc8xx does here. Perhaps we can adopt this pattern?

[ToddG]

Curious as to the bifurcation when writing the byte array. Why is $twea not set for the final byte? I thought the final byte required an ACK as well.

[Rahix]

The datasheet text you insterted below explains it: After the last byte, master send NACK and then a stop condition.

[ToddG]

I see why I was confused now.

So we have read the last byte in read_data(), and we set TWINT and TWEN high:

avr-hal/avr-hal-generic/src/i2c.rs

Line 397 in 9f9d697

self.p.$twcr.write(|w| w.$twint().set_bit().$twen().set_bit());

Then call wait() which waits for TWINT to be cleared:

avr-hal/avr-hal-generic/src/i2c.rs

Line 357 in 9f9d697

while self.p.$twcr.read().$twint().bit_is_clear() { }

Then wherever read_data() was invoked from can invoke a STOP, like here in blocking_read():

avr-hal/avr-hal-generic/src/i2c.rs

Line 477 in 9f9d697

self.stop();

The stop() is here:

avr-hal/avr-hal-generic/src/i2c.rs

Lines 423 to 425 in 9f9d697

	.$twen().set_bit()
	.$twint().set_bit()
	.$twstop().set_bit()

My question has evolved to:

1. (minor question) I don't see a restart method on the API... is that intentional?
2. (main question) The datasheet defines the STOP as being invoked exactly as in the stop() method shown. My question is why the intervening writes? Here's what I see:

a) write TWINT TWEN high
b) wait for TWINT to clear
c) write TWINT TWEN TWSTO high

Why do we need a and b?

[Rahix]

a) write TWINT TWEN high

this clears the TWINT bit, as I described in the other comments

b) wait for TWINT to clear

actually, the line

avr-hal/avr-hal-generic/src/i2c.rs

Line 357 in 9f9d697

while self.p.$twcr.read().$twint().bit_is_clear() { }

waits for it to become set again ("while bit is cleared, loop").

[ToddG]

Additional question:

I have read up on PhantomData here and here, but I'm still curious how it is being used here:

avr-hal/avr-hal-generic/src/i2c.rs

Line 208 in 9f9d697

_clock: ::core::marker::PhantomData<CLOCK>,

• Why is there a phantomdata marked type here at all?
• Why is the phantom field named _clock?

[Rahix]

Why is there a phantomdata marked type here at all?

Because you cannot have a generic type parameter (CLOCK) which is not used. The PhantomData<CLOCK> is a zero-sized type which counts as a "use" of the type paramter, thus it is legal.

Why is the phantom field named _clock?

To indicate that the field does not really hold any use, I've prefixed it with an underscore.

[ToddG]

Regarding the CLOCK phantom... yeah, I've been playing around with how it's used and re-reading the docs, etc. I think I understand it now.

Thx!

[ToddG]

Looking for API feedback.

[ToddG]

I wrote a simplified version of what lpc8xx-hal has. I may eventually end up with exactly what they wrote, but I'm just trying to build up from scratch with the simplest API possible. I researched other popular rust state machine patterns, and settled on this.

I like the idea of using the compiler and type checker to validate the state transitions, but this looks ugly to me:

https://hoverbear.org/blog/rust-state-machine-pattern/

I like the all transitions listed in one place as here:

https://dev.to/werner/practical-rust-web-development-state-machine-2jlc

But I prefer to have use types with explicit methods on them as here:

 TypeA.fooMethod() -> TypeB
 TypeB.barMethod() -> TypeC

etc.

I'm just stubbing the methods out here to get your thoughts before I flesh out the implementation.

[ToddG]

Couple ways to go here...

Slave.init() -> the SlaveState::Initialized

or

SlaveState::Unitialized.init(slave) -> SlaveState::Initialized

[ToddG]

Ok, I'm getting close. I've encapsulated the data sheet status codes in a series of state transitions.

I need some feedback regarding the client experience/api? You mentioned signaling/polling an atomic variable at one point.

[Rahix]

I'm going to take a closer look later today!

One thing we need to take care of is that this PR now contains many changes that are so to speak "orthogonal" to each other (e.g. "removing the $macro-tokens", "renaming $I2c to $I2c Master", and "adding the I2C slave implementation"). Ideally, each one should go into a single separate commit so that is becomes easy to trace what is changed each time.

I know that splitting out a changeset into such cleanly separated commits is quite cumbersome so I can offer to take care of this by pulling out all the "undisputed" changes (i.e. everything except the slave implementation itself, I think) into commits that I'll merge manually prematurely (of course attributing them to you!). Once that is done, you can rebase this PR ontop of them and it will only contain the I2c slave driver changeset which is then much easier to look at in more detail.

Would that be okay for you?

[ToddG]

@Rahix Somehow I missed your last message. My apologies for taking 6 days to respond. Yes, I was getting concerned about orthogonal changes drifting into this PR, too. Sure thing, splitting them out and rebasing sounds great, thank you for taking care of that.

[ToddG]

@Rahix Ok, so I've added some triggers and helper methods based on your suggestions here.

Couple of thoughts...

• A fully fleshed example using the I2C Master API would really help me understand the api. The i2cdetect just doesn't give my brain enough to work with.
• Once I understand the pattern for master, I think this will unblock me. I'll wrap up by writing an example using the I2C Slave API as I finish writing that api.
• My final goal is to write a master <-> slave example of two arduinos talking to each other over the I2C bus

[Rahix]

Sure thing, splitting them out and rebasing sounds great, thank you for taking care of that.

Okay, will do as soon as I have time!

A fully fleshed example using the I2C Master API would really help me understand the api. The i2cdetect just doesn't give my brain enough to work with.

Hmm, in the end, any embedded-hal I2C code will look the same. So just take any existing I2C device driver (from awesome embedded rust) to see how the bus would be used.

My final goal is to write a master <-> slave example of two arduinos talking to each other over the I2C bus

That makes a lot of sense! Let's include the code for those two examples here as well!

[Rahix]

Ok, so I've added some triggers and helper methods based on your suggestions here.

Hmm, I would say these changes are going in the wrong direction. A HAL driver should not contain any statics as this will mess with composability. Such things should be done solely on application level. I'm also not quite sure what you added them for? If it's interrupts: they are irrelevant here, they are also something to be managed on application level (note how none of the other HAL drivers have any interrupt-specific code).

[ToddG]

Ok, I'll back that static stuff out and examine the I2C driver in awesome embedded rust and see how that's implemented and used and then go from there.

[ToddG]

Ok, so this example doesn't compile yet, but this is my take on the API for the i2c slave:

#![no_std]
#![no_main]

use arduino_uno::prelude::*;
use panic_halt as _;

#[arduino_uno::entry]
fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600,
    );
    let mut i2c = arduino_uno::I2c::new(
        dp.TWI,
        pins.a4.into_pull_up_input(&mut pins.ddr),
        pins.a5.into_pull_up_input(&mut pins.ddr),
        50000,
    );
    let mut slave = arduino_uno::I2cSlave::new(
        i2c,
        pins.a1.into_pull_up_input(&mut pins.ddr),
        pins.a2.into_pull_up_input(&mut pins.ddr),
        b01111111,
        false,
    );

    // statemachine
    let mut state = slave.start()?;

    // data to write
    let mut data = 0;

    loop {
        ufmt::uwriteln!(&mut serial, "I2C slave starting...\r").void_unwrap();
        state = match(state){
            Ok(arduino_uno::I2c::I2cSlaveState::Uninitialized)
                =>{ state.init() }
            Ok(arduino_uno::I2c::I2cSlaveState::Initialized)
                =>{ state.wait() }
            Ok(arduino_uno::I2c::I2cSlaveState::AddressMatched)
                =>{
                    ufmt::uwriteln!(&mut serial, "I2C slave address: {}\r", state.address()).void_unwrap();
                    state.next()
            }
            Ok(arduino_uno::I2c::I2cSlaveState::RxReady)
                =>{
                    ufmt::uwriteln!(&mut serial, "I2C slave received: {}\r", state.read()).void_unwrap();
                    state.ack().next()
            }
            Ok(arduino_uno::I2c::I2cSlaveState::TxReady)
                =>{
                    ufmt::uwriteln!(&mut serial, "I2C slave writing: {}\r", data).void_unwrap();
                    state.write(data);
                    state.next()
            }
            Err(arduino_uno::I2c::Error)
                => {
                    ufmt::uwriteln!(&mut serial, "I2C slave error\r").void_unwrap();
                    state.init()
            }
        }
    }
}

[Rahix]

@ToddG, can you quickly review #102 so I can move forward with that?

[ToddG]

@Rahix #102 approved. I'm not sure what's going on with github notifications. I'm online, logged into github, all day every day for work...yet somehow I'm not getting notifications on PR's and such. I'll look at my settings closely...my apologies for the delays.

[Rahix]

No problem, thanks for the reply! I've now merged #102. Please rebase this branch ontop of latest master now and make sure only the actual I2C slave driver changes remain here.