I2C with interrupts #198
I2C with interrupts #198
Conversation
a tx ready interrupt with no write buffer
without following read
There was a problem hiding this comment.
Thank you really much for your contribution. This looks promising. Code looks pretty clean, which I like.
Small nit's here and there. I just have a few suggestions about the design for now.
But I think we can improve on it further. For that, I have to read into the code a little more in detail.
I'm not so sure, if it is a good idea to keep internal tx and rx (ring) buffers. I'd rather wish this implementation would be a thin wrapper around the State function, so that the user can get the received byte by themselves and put it into their preferred buffer type, like heapless::spsc::Queue.
This would look something like this:
fn i2cinterrupt() {
interrupt_type = cx.i2cinstance.get_interrupt_type();
match interrupt_type {
Some(TxReady) => cx.i2cinstance.read(),
_ => ...
}
}where get_interrupt_type() is
pub fn get_interrupt_type(&self) -> Option<State> { ... }or even provide a function like:
pub fn read_on_rx(&self) -> Option<[u8]> {
if self.state == State::RxReady {
Some(self.dev.i2c.rxdr.read().rxdata().bits())
} else {
None
}
}Pretty much all in all a thin wrapper on the I2cInstance itself.
Maybe we could even implement the embedded hal traits (at least Read and Write for it)
Much rambling. I hope it helped, but maybe you have better ideas :)
src/i2cint.rs
Outdated
| if isr.nackf().bit() { | ||
| self.dev.i2c.icr.write(|w| w.nackcf().bit(true)); | ||
| self.state = State::TxNack; |
There was a problem hiding this comment.
I would prefer an if expression to change self.state. It is more readable IMO
| if isr.nackf().bit() { | |
| self.dev.i2c.icr.write(|w| w.nackcf().bit(true)); | |
| self.state = State::TxNack; | |
| self.sate = if isr.nackf().bit() { | |
| self.dev.i2c.icr.write(|w| w.nackcf().bit(true)); | |
| State::TxNack |
| } | ||
|
|
||
| /// Debug function which returns the content of the ISR register. | ||
| pub fn get_isr(&self) -> u32 { |
There was a problem hiding this comment.
While I see, that this is useful, I would like to see that this function returns a type (or list of types e.g. enums) instead of an u32.
What exactly does u32 mean for a potential user of this function? This does not seem clear right now.
| /// | ||
| /// # Errors | ||
| /// * `I2cError::DeviceBusy` if the device is already busy. | ||
| pub fn stop(&mut self, addr: u8) { |
There was a problem hiding this comment.
When specifying I2C addresses, always note, if the LSB is still part of the address or assumed to be the R/W bit, to void confusion. In this case, it is the latter.
| tx_ind: usize, | ||
| tx_buf: Option<&'static [u8]>, | ||
| rx_ind: usize, | ||
| rx_buf: [u8; 256], // for the moment use a static buffer here. |
There was a problem hiding this comment.
In the long term, I'd like to see a user configurable slice (e.g. Option<&'static [u8]> as for tx_buf) or something similar, so that the user themselves can choose, which kind of buffer he needs (size wise and type wise).
| } | ||
|
|
||
| /// I2c1Int provides interface to communicate with i2c devices using interruptions. | ||
| pub struct I2cInt<I2C, PINS> { |
There was a problem hiding this comment.
A free function is missing, so that the user can regain access to the pins, if I2c is no longer needed:
Lines 201 to 204 in 9e29ec6
src/i2cint.rs
Outdated
|
|
||
| /// This function must be called when there is an interruption on i2c device. | ||
| /// It will compute the current state based on ISR register and execute work based on the state. | ||
| pub fn interrupt(&mut self) { |
There was a problem hiding this comment.
interrupt sound like, this function would return the type of the interrupt, which got triggered rather than reaction on the interrupt. I'd look for a more descriptive name. Maybe react_on_interrupt()? Better suggestions are welcome.
src/i2cint.rs
Outdated
| } | ||
|
|
||
| /// Get the state of the device. | ||
| pub fn get_tx_state(&self) -> State { |
There was a problem hiding this comment.
State does include RX and TX states. Therefor this function name does not make sense?
| pub fn get_tx_state(&self) -> State { | |
| pub fn get_state(&self) -> State { |
| /// # Returns | ||
| /// true if busy, false if not. | ||
| pub fn is_busy(&mut self) -> bool { |
There was a problem hiding this comment.
As you already have chosen a perfect name for a boolean function, I find this description redundant:
| /// # Returns | |
| /// true if busy, false if not. | |
| pub fn is_busy(&mut self) -> bool { | |
| pub fn is_busy(&mut self) -> bool { |
| } | ||
|
|
||
| /// Enables all interrupts for i2c device. | ||
| pub fn enable_interrupts(&mut self) { |
There was a problem hiding this comment.
This should be more flexible in its configuration, I guess 🤔
wite is finished and must read the rx buffer when transfer is completed.
It is a first attempt to implement I2C communications using interrupts. I know that this is not ready for merge but I wanted to have a feedback on my work on I2C and see if I am going into the right direction. Thruthfully, I am not an experienced embedded developer.
As of now, I don't think there is someting in embedded-hal define to support i2c communication with interrupts.
The interrupt function is responsible for computing the state of the I2C device on every interruption. For the moment, calling this function is the responsability of the consumer.
You can find an example of use here: example. The example is using cortex-m-rtic. In this example the interrupt function is called for I2C1_EV_EXTI23 and I2C1_ER.