i2c master and slave

[lxuxx]

No description provided.

[rusty1968]

Run cargo fmt.

[lxuxx]

Ok

[rusty1968]

This should be a const method.

[rusty1968]

Can you add me as a reviewer?

[rusty1968]

This is a very sophisticated I2C controller hardware with multiple modes (DMA, polling, interrupt). We need a more layered approach. We should formalize a I2CHardware trait and build the higher level embedded_hal::i2c , I2CTarget traits on top of it.

[rusty1968]

Seems like there are lots of clippy warnings. Do you want to convert this to a draft?

[rusty1968]

Consider using RAII (Resource Acquisition is Initialization) patterns for resource management

Suggested change

	impl<I2C: Instance, I2CT: I2CTarget> Drop for I2cController<'_, I2C, I2CT> {
	fn drop(&mut self) {
	// Disable I2C controller
	self.i2c.i2cc00().write(|w| unsafe { w.bits(0) });
	// Clear any pending interrupts
	self.i2c.i2cm14().write(|w| unsafe { w.bits(0xffffffff) });
	self.i2c.i2cs24().write(|w| unsafe { w.bits(0xffffffff) });
	}
	}

[rusty1968]

Asserts create panic. In embedded systems, panics can be problematic due to limited debugging capabilities and the need for high reliability. Instead of using assert!, it's often better to handle errors gracefully.

pub fn new(buf_idx: usize) -> Result<Self, AspeedI2cError> {
        if buf_idx < I2C_TOTAL {
            Ok(Self {
                // Initialize the struct here
            })
        } else {
            // Error
        }

}

[rusty1968]

Suggested change

	pub fn i2c_wait_completion(&mut self) {
	pub fn i2c_wait_completion(&mut self) -> Result<(), CmdErr> {
	let mut delay = DummyDelay {};
	let mut timeout = 1_000_000;
	while timeout > 0 && !self.i2c_data.completion {
	self.aspeed_i2c_master_irq();
	delay.delay_ns(100_000);
	timeout -= 1;
	}
	if !self.i2c_data.completion {
	self.i2c_data.cmd_err = CmdErr::ErrTimeout;
	return Err(CmdErr::ErrTimeout);
	}
	Ok(())
	}

[rusty1968]

The current implementation of aspeed_new_i2c_recover_bus returns a boolean to indicate success or failure. While this approach is straightforward, it lacks the ability to provide detailed error information.

Suggestion: Consider using a Result type to return more descriptive errors, which is more idiomatic in Rust. This change will allow the caller to handle different error cases appropriately and improve overall error handling.

Example:

pub fn aspeed_new_i2c_recover_bus(&mut self) -> Result<(), ASpeedI2cError> {
    // Your implementation here
    // Return Ok(()) on success or Err(I2cError::SpecificError) on failure
}

Benefits:

• Enhances code readability and maintainability by clearly communicating the possible outcomes of the function.
• Provides more detailed error information, allowing for better error handling and debugging.

[rusty1968]

With idiomatic error handling, we can simplify the code by using the ? operator here. This operator propagates errors, making the code cleaner and easier to read.

Suggestion: Refactor self.i2c_wait_completion() to return a Result type. This will allow us to use the ? operator to handle errors more idiomatically.

Example:

self.i2c_wait_completion()?;

Benefits:

• Simplifies error handling by propagating errors automatically.
• Enhances code readability and maintainability.
• Aligns with Rust's idiomatic practices for error handling.

[rusty1968]

Suggested change

	let buf_ref: &'a mut [u8] = &mut I2C_BUF[buf_idx];
	let buf_ref = I2C_BUF.get_mut(buf_idx)
	.ok_or(AspeedError::BufferIndexOutOfBounds)?;

This line of code ensures safety in the following ways:

Bounds Checking:

The method I2C_BUF.get_mut(buf_idx) is used to access the buffer at the specified index.
This method performs an internal check to ensure that buf_idx is within the valid range of indices for I2C_BUF.
If buf_idx is out of bounds, get_mut returns None, preventing any out-of-bounds memory access.

Error Handling:

The .ok_or(AspeedError::BufferIndexOutOfBounds)? part converts the Option returned by get_mut into a Result.
If get_mut returns None (indicating an out-of-bounds index), ok_or converts it to Err(AspeedError::BufferIndexOutOfBounds).
The ? operator then propagates this error, causing the function to return early with the appropriate error, thus preventing further execution with an invalid buffer reference.

Immutable Borrowing:

By using get_mut, the code ensures that the buffer is borrowed mutably only if the index is valid.
This prevents any undefined behavior that could arise from accessing invalid memory locations.

Clear Error Reporting:

The use of AspeedError::BufferIndexOutOfBounds provides a clear and specific error message, making it easier to diagnose and handle the error appropriately.

[rusty1968]

Suggested change

	trait I2cHardware {
	type Config: Clone + core::fmt::Debug;
	type Error;
	fn reset(&mut self);
	fn configure(&mut self, config: &Self::Config) -> Result<(), Self::Error>;
	fn enable_interrupts(&mut self, mask: u32);
	fn clear_interrupts(&mut self, mask: u32);
	fn start_transfer(&mut self, state: &TransferState) -> Result<(), Self::Error>;
	fn handle_interrupt(&mut self) -> InterruptStatus<Self::Error>;
	fn is_bus_busy(&self) -> bool;
	fn recover_bus(&mut self) -> Result<(), Self::Error>;
	}

[rusty1968]

Suggested change

	pub struct I2cConfigBuilder {
	xfer_mode: I2cXferMode,
	multi_master: bool,
	smbus_timeout: bool,
	manual_scl_high: u8,
	manual_scl_low: u8,
	manual_sda_hold: u8,
	smbus_alert: bool,
	clk_src: u32,
	mode: Mode,
	}
	impl I2cConfigBuilder {
	/// Creates a new builder with default values
	pub fn new() -> Self {
	Self {
	xfer_mode: I2cXferMode::DmaMode,
	multi_master: false,
	smbus_timeout: false,
	manual_scl_high: 0,
	manual_scl_low: 0,
	manual_sda_hold: 0,
	smbus_alert: false,
	clk_src: 0,
	mode: Mode::Standard,
	}
	}
	/// Sets the transfer mode
	pub fn xfer_mode(mut self, mode: I2cXferMode) -> Self {
	self.xfer_mode = mode;
	self
	}
	/// Sets multi-master mode
	pub fn multi_master(mut self, enabled: bool) -> Self {
	self.multi_master = enabled;
	self
	}
	/// Sets SMBus timeout
	pub fn smbus_timeout(mut self, enabled: bool) -> Self {
	self.smbus_timeout = enabled;
	self
	}
	/// Sets manual SCL high timing
	pub fn manual_scl_high(mut self, value: u8) -> Self {
	self.manual_scl_high = value;
	self
	}
	/// Sets manual SCL low timing
	pub fn manual_scl_low(mut self, value: u8) -> Self {
	self.manual_scl_low = value;
	self
	}
	/// Sets manual SDA hold timing
	pub fn manual_sda_hold(mut self, value: u8) -> Self {
	self.manual_sda_hold = value;
	self
	}
	/// Sets SMBus alert
	pub fn smbus_alert(mut self, enabled: bool) -> Self {
	self.smbus_alert = enabled;
	self
	}
	/// Sets the clock source
	pub fn clk_src(mut self, value: u32) -> Self {
	self.clk_src = value;
	self
	}
	/// Sets the I2C mode (Standard/Fast/FastPlus)
	pub fn mode(mut self, mode: Mode) -> Self {
	self.mode = mode;
	self
	}
	/// Builds and returns the I2cConfig
	pub fn build(self) -> I2cConfig {
	I2cConfig {
	xfer_mode: self.xfer_mode,
	multi_master: self.multi_master,
	smbus_timeout: self.smbus_timeout,
	manual_scl_high: self.manual_scl_high,
	manual_scl_low: self.manual_scl_low,
	manual_sda_hold: self.manual_sda_hold,
	smbus_alert: self.smbus_alert,
	clk_src: self.clk_src,
	mode: self.mode,
	}
	}
	/// Creates a configuration for standard I2C mode (100kHz)
	pub fn standard() -> I2cConfig {
	Self::new().mode(Mode::Standard).build()
	}
	/// Creates a configuration for fast I2C mode (400kHz)
	pub fn fast() -> I2cConfig {
	Self::new().mode(Mode::Fast).build()
	}
	/// Creates a configuration for fast plus I2C mode (1MHz)
	pub fn fast_plus() -> I2cConfig {
	Self::new().mode(Mode::FastPlus).build()
	}
	/// Creates a configuration with multi-master support enabled
	pub fn multi_master_config() -> I2cConfig {
	Self::new().multi_master(true).build()
	}
	/// Creates a configuration with SMBus features enabled
	pub fn smbus_config() -> I2cConfig {
	Self::new()
	.smbus_timeout(true)
	.smbus_alert(true)
	.build()
	}
	}

[rusty1968]

Consider using the Builder pattern for I2C Configuration.