MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.18k stars 19.21k forks source link

[BUG] Small (<32Kb) I2C EEPROM doesn't work #21330

Closed jafal99 closed 3 years ago

jafal99 commented 3 years ago

Description

Current HAL code has support I2C EEPROM but if connected chip has size less then 32Kb (4KB) it can't be accessed. This is due the fact that smaller chips use only one byte addressing. I have prepared small fix for HAL code to allow using small and large I2C EEPROMs. Detection rely on MARLIN_EEPROM_SIZE

Patch

small_i2c_eeprom.zip

diff --git a/Marlin/src/HAL/shared/eeprom_if_i2c.cpp b/Marlin/src/HAL/shared/eeprom_if_i2c.cpp
index cc22bf7d98..98e71dedb4 100644
--- a/Marlin/src/HAL/shared/eeprom_if_i2c.cpp
+++ b/Marlin/src/HAL/shared/eeprom_if_i2c.cpp
@@ -45,6 +45,24 @@ void eeprom_init() { Wire.begin(); }

 static constexpr uint8_t eeprom_device_address = I2C_ADDRESS(EEPROM_DEVICE_ADDRESS);

+void _beginTransmission(const uint16_t memoryAddress)
+{
+  if (MARLIN_EEPROM_SIZE > 0x4000)     //use two byte addressing for eeproms >16kb
+  {
+     Wire.beginTransmission(eeprom_device_address);
+    // Address High Byte
+    Wire.write((memoryAddress >> 8));
+  }
+  else
+  {
+    uint8_t addr = eeprom_device_address | (byte)((memoryAddress >> 8) & 0x07);
+    Wire.beginTransmission(addr);
+  }
+
+   // Address Low Byte (or only byte for chips 16Kb  or smaller (24C02, 24C04, 24C08,24C16)that only have one-word addresses)
+   Wire.write((memoryAddress & 0xFF));
+}
+
 // ------------------------
 // Public functions
 // ------------------------
@@ -52,9 +70,7 @@ static constexpr uint8_t eeprom_device_address = I2C_ADDRESS(EEPROM_DEVICE_ADDRE
 void eeprom_write_byte(uint8_t *pos, unsigned char value) {
   const unsigned eeprom_address = (unsigned)pos;

-  Wire.beginTransmission(eeprom_device_address);
-  Wire.write(int(eeprom_address >> 8));   // MSB
-  Wire.write(int(eeprom_address & 0xFF)); // LSB
+  _beginTransmission(eeprom_address);
   Wire.write(value);
   Wire.endTransmission();

@@ -66,11 +82,14 @@ void eeprom_write_byte(uint8_t *pos, unsigned char value) {
 uint8_t eeprom_read_byte(uint8_t *pos) {
   const unsigned eeprom_address = (unsigned)pos;

-  Wire.beginTransmission(eeprom_device_address);
-  Wire.write(int(eeprom_address >> 8));   // MSB
-  Wire.write(int(eeprom_address & 0xFF)); // LSB
+  _beginTransmission(eeprom_address);
   Wire.endTransmission();
-  Wire.requestFrom(eeprom_device_address, (byte)1);
+
+  int addr = eeprom_device_address;
+  if(MARLIN_EEPROM_SIZE <= 0x4000){ //for eeproms <=16Kb lower adres bits ar used for 2Kb page address
+     addr = eeprom_device_address | (byte)((eeprom_address >> 8) & 0x07);
+  }
+  Wire.requestFrom(addr, (byte)1);
   return Wire.available() ? Wire.read() : 0xFF;
 }
thinkyhead commented 3 years ago

Thanks! I've made this into a PR on your behalf.

thinkyhead commented 3 years ago

I see that we have a separate EEPROM implementation for BL24Cxx that accounts for the address different, so maybe that can be unified with this more general i2c-based EEPROM code at some point in the future.

jafal99 commented 3 years ago

I did quick look on BL24CXX:

The questions are:

  1. how to replace global instance of Wire with new one,
  2. Shall be separate instance of I2C used only for EEPROM? (I dont't think so)

The possibility of specifying which I2C controller (harware1,hardware1alt,hardware2, software) shall be used can be useful in general, not only for Ender. To use second controller I need to alter core library

thinkyhead commented 3 years ago

Apparently this change breaks EEPROM functionality for many boards. I will revert the changes from #21337 until we can figure out where the error lies.

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.