Made improvements to polling i2c driver #102
Conversation
3 improvements made to driver: write_read embedded_hal implementation was not properly performing a repeated start between the write and read phases of the transaction. This was because write_read was implemented in terms of write() and read() traits. This has been refactored so that the write_read trait is smart enough to do a repeated start, and skip the write and read sections of the transaction if their corresponding slice arguments are empty (length zero). write() and read() are now also implemented in terms of the write_read() trait. Currently, there is no checking for error conditions while the i2c driver is waiting for access to the bus or available space in transmit buffers. This causes problems since if a bus error occurs while a transaction is occuring the hardware I2C device will abort the current transaction. This means that it is possble to wait forever for a transmit buffer to be empty while there are no transactions going on. This was fixed by updating all of the polling loops to check for i2c errors and abort if a problem occurs. If the address byte of the i2c transaction is NACKed, then the I2C peripheral will abort the transaction while there is still data in the transmit buffer. This means that future i2c transactions will use this "old" data as the first byte of there new transaction. This was fixed by flushing the tx buffers before a new transaction starts.
| return Ok(()); | ||
| } | ||
| // check for any errors | ||
| return self.check_errors(); |
| self.i2c.icr.write(|w| w.nackcf().set_bit()); | ||
| return Err(Error::Nack); | ||
| } | ||
| return Ok(()); |
There was a problem hiding this comment.
No need for return, in fact the whole if can be transformed to not include return statement.
This isn't a policy in this crate, and there's lots of existing code that doesn't follow this convention. I'm open to changing that, but someone would have to put in the legwork. Specifically, I'd like to see two things:
|
I see. Enforcing it in CI and reformatting the current code is doable. The second part is bit of work but I can create a PR for that if you want. I would like to see the fmt rules enforced. |
To be clear, I'm not a fan of rustfmt and would prefer if things stayed as they ar. I can see that I'm in the minority though, and don't intend to stand in the way of progress So I'm not asking you (or anyone) to do this. I'm just saying, if it's going to be done, that's what it takes to get me to agree :-) |
Hi, I discovered a couple of issues with the blocking I2C driver while playing around with an 32L0538DISCOVERY board. This board has a custom I2C slave peripheral to measure the current of the main STM32 controller, and while writing a driver for it I stumbled upon some bugs in the HAL driver.
Could someone take a look at these changes and see if they can make it into the library?