avr-hal
avr-hal copied to clipboard
Rahix
[#90] I2C slave driver
Additional question:
I have read up on PhantomData here and here, but I'm still curious how it is being used here:
https://github.com/Rahix/avr-hal/blob/9f9d6972e4ac7c149e2644bbc52c47cca81838f4/avr-hal-generic/src/i2c.rs#L208
- Why is there a phantomdata marked type here at all?
- Why is the phantom field named
_clock?
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.
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!
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.
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?
@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.
@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 APIwould 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 APIas I finish writing that api. - My final goal is to write a
master<->slaveexample of two arduinos talking to each other over the I2C bus
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!
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).
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.
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 #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.
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.