From d6d54392f15cb0430a50ba85e45d746aa09fc6ac Mon Sep 17 00:00:00 2001 From: HybridChild Date: Sun, 10 Aug 2025 10:48:12 +0200 Subject: stm32/i2c_v1: Fix bugs with slave address initialization and missing ACK bit --- embassy-stm32/src/i2c/v1.rs | 128 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 107 insertions(+), 21 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 3aa003fa5..2bc309258 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -408,57 +408,97 @@ impl<'d, M: Mode> I2c<'d, M, Master> { } impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { + /// Enhanced slave configuration with proper v1 address setup pub(crate) fn init_slave(&mut self, config: SlaveAddrConfig) { trace!("i2c v1 slave init: config={:?}", config); + // Disable peripheral for configuration self.info.regs.cr1().modify(|reg| { reg.set_pe(false); }); - - // Configure v1-specific slave settings + + // Configure addresses with proper v1 format self.configure_addresses(config); - // Enable slave mode interrupts and settings - self.info.regs.cr2().modify(|w| { - w.set_itevten(true); // Event interrupts - w.set_iterren(true); // Error interrupts - }); + // Configure general call if requested + if config.general_call { + self.info.regs.cr1().modify(|w| w.set_engc(true)); + trace!("i2c v1 slave: General call enabled"); + } + + // Log final configuration before enabling + let cr1 = self.info.regs.cr1().read(); + let oar1 = self.info.regs.oar1().read(); + let oar2 = self.info.regs.oar2().read(); + trace!("i2c v1 slave: Pre-enable state - CR1={:#x}, OAR1={:#x}, OAR2={:#x}", + cr1.0, oar1.0, oar2.0); + trace!("i2c v1 slave: Address details - OAR1.ADD={:#x}, OAR1.ADDMODE={}, bit14={}", + oar1.add(), oar1.addmode() as u8, (oar1.0 >> 14) & 1); - // Re-enable peripheral self.info.regs.cr1().modify(|reg| { - reg.set_pe(true); + reg.set_pe(true); // Re-enable peripheral + reg.set_ack(true); // Critical for slave to ACK its address }); + + // Verify peripheral is enabled and ready + let cr1_final = self.info.regs.cr1().read(); + trace!("i2c v1 slave: Final state - CR1={:#x}, PE={}", cr1_final.0, cr1_final.pe()); + trace!("i2c v1 slave init complete"); } fn configure_oa1(&mut self, addr: Address) { match addr { Address::SevenBit(addr) => { + trace!("i2c v1 slave: Setting OA1 7-bit address: input={:#x}", addr); self.info.regs.oar1().write(|reg| { - // v1 uses left-shifted 7-bit address in bits [7:1] - // STM32 reference manual says bits 7:1 for address, bit 0 don't care for 7-bit - reg.set_add((addr as u16) << 1); + // For I2C v1, the 7-bit address goes in bits [7:1] of the ADD field + // The ADD field spans bits [9:0], so we put the address in the correct position + let hw_addr = (addr as u16) << 1; // This puts address in bits [7:1], bit [0] = 0 + reg.set_add(hw_addr); reg.set_addmode(i2c::vals::Addmode::BIT7); }); + + // CRITICAL: Set bit 14 as required by the reference manual + // "Bit 14: Should always be kept at 1 by software" + self.info.regs.oar1().modify(|reg| { + reg.0 |= 1 << 14; // Set bit 14 + }); + + let oar1_verify = self.info.regs.oar1().read(); + trace!("i2c v1 slave: OA1 configured - OAR1={:#x}, stored_addr={:#x}, bit14={}", + oar1_verify.0, oar1_verify.add(), (oar1_verify.0 >> 14) & 1); }, Address::TenBit(addr) => { - self.info.regs.oar1().modify(|reg| { - reg.set_add(addr); // Set address bits [9:0] + trace!("i2c v1 slave: Setting OA1 10-bit address: {:#x}", addr); + self.info.regs.oar1().write(|reg| { + reg.set_add(addr); // For 10-bit, full address goes in ADD field reg.set_addmode(i2c::vals::Addmode::BIT10); - // Manually set bit 14 as required by reference manual - reg.0 |= 1 << 14; }); + + // Set required bit 14 for 10-bit mode too + self.info.regs.oar1().modify(|reg| { + reg.0 |= 1 << 14; // Set bit 14 + }); + + let oar1_verify = self.info.regs.oar1().read(); + trace!("i2c v1 slave: OA1 10-bit configured - OAR1={:#x}, bit14={}", + oar1_verify.0, (oar1_verify.0 >> 14) & 1); } } } - + fn configure_oa2_simple(&mut self, addr: u8) { + trace!("i2c v1 slave: Setting OA2 address: {:#x}", addr); self.info.regs.oar2().write(|reg| { - // v1 OA2: 7-bit address only, no masking support - // Address goes in bits [7:1], enable dual addressing - reg.set_add2(addr); - reg.set_endual(i2c::vals::Endual::DUAL); + // For OA2, the address goes in bits [7:1] of the ADD2 field + reg.set_add2(addr); // ADD2 field automatically handles bits [7:1] placement + reg.set_endual(i2c::vals::Endual::DUAL); // Enable dual addressing }); + + let oar2_verify = self.info.regs.oar2().read(); + trace!("i2c v1 slave: OA2 configured - OAR2={:#x}, ADD2={:#x}, ENDUAL={}", + oar2_verify.0, oar2_verify.add2(), oar2_verify.endual() as u8); } fn configure_addresses(&mut self, config: SlaveAddrConfig) { @@ -507,6 +547,52 @@ impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { } } +// Also add a verification function to check address configuration +impl<'d, M: Mode> I2c<'d, M, MultiMaster> { + /// Verify the slave address configuration is correct + pub fn verify_slave_config(&self) -> Result<(), Error> { + let oar1 = self.info.regs.oar1().read(); + let oar2 = self.info.regs.oar2().read(); + let cr1 = self.info.regs.cr1().read(); + + info!("I2C v1 Slave Configuration Verification:"); + info!(" CR1: {:#x} (PE={})", cr1.0, cr1.pe()); + info!(" OAR1: {:#x}", oar1.0); + info!(" ADD: {:#x}", oar1.add()); + info!(" ADDMODE: {} ({})", oar1.addmode() as u8, + if oar1.addmode() as u8 == 0 { "7-bit" } else { "10-bit" }); + info!(" Bit 14: {}", (oar1.0 >> 14) & 1); + info!(" OAR2: {:#x}", oar2.0); + info!(" ADD2: {:#x}", oar2.add2()); + info!(" ENDUAL: {} ({})", oar2.endual() as u8, + if oar2.endual() as u8 == 0 { "Single" } else { "Dual" }); + + // Check critical requirements + if !cr1.pe() { + error!("ERROR: I2C peripheral not enabled (PE=0)"); + return Err(Error::Bus); + } + + if (oar1.0 >> 14) & 1 == 0 { + error!("ERROR: OAR1 bit 14 not set (required by reference manual)"); + return Err(Error::Bus); + } + + // For 7-bit mode, verify address is in correct position + if oar1.addmode() as u8 == 0 { // 7-bit mode + let expected_addr = 0x42u16 << 1; // 0x84 + if oar1.add() != expected_addr { + error!("ERROR: OAR1 address mismatch - expected {:#x}, got {:#x}", + expected_addr, oar1.add()); + return Err(Error::Bus); + } + } + + info!("✓ Slave configuration appears correct"); + Ok(()) + } +} + impl<'d, M: Mode> I2c<'d, M, MultiMaster> { /// Listen for incoming I2C address match and return the command type pub fn blocking_listen(&mut self) -> Result { -- cgit