I2C EEPROM read/write/verify

[ian]

Device	Size	Size (bytes)	Page Size	Address Bytes	Block Select Bits
24LC1025	128 KB	131072	128	2	1 in I2C ADDR bit 3
24LC512	64 KB	65536	128	2	0
24LC256	32 KB	32768	64	2	0
24LC128	16 KB	16384	64	2	0
24LC64	8 KB	8192	32	2	0
24LC32	4 KB	4096	32	2	0
24LC16	2 KB	2048	16	1	3
24LC08	1 KB	1024	16	1	2
24LC04	512 B	512	16	1	1
24LC02	256 B	256	8	1	0
24LC01	128 B	128	8	1	0

I’ve delayed making an eeprom read/write/dump command because there are so very many types of eeproms with different page sizes and addressing methods. If we just target the “main set” of 24C EEPROMs from Microchip (née ATMEL) and their major competitor, uh, also Microchip, there is a logical progression.

I didn’t find any existing libraries I was super happy with, but I read through this one.

It seems there is a common method to determine EEPROM size and page size. It is potentially destructive… Basically backup the chip, then write various lengths of data until a roll-over from the end to beginning is detected. I imagine we’ll get to this, but it seems dangerous.

Let’s see if I can make an “elegant” hardware abstraction layer. Temporary documentation started here, but it will move later.

[ian]

Device types are stored in a struct with enough info to handle the size, block address, etc. Here you can see the smaller devices with block addresses in the I2C address progress correctly.

List/specify device type. It should probably just be 24x01 or 24Cx01. Added the progress indicator from bluetag.

Addressing turned out to be not that big a deal, so I’ll continue with save to file and verify against file. After that write, which might be a nightmare of shuffling page sizes, or it might turn out to be surprisingly easy read addressing.

ETA:

Turns out page writes not that hard either!

ETA:

struct eeprom_device_t {
    char name[9];
    uint32_t size_bytes;
    uint8_t address_bytes; 
    uint8_t block_select_bits;
    uint8_t block_select_low_bit_location;
    uint8_t page_bytes; 
};
....
{ "24CM02", 262144, 2, 2, 0, 256 },
{ "24CM01", 131072, 2, 1, 0, 256 },    
{ "24C1026", 131072, 2, 1, 0, 128 },
{ "24C1025", 131072, 2, 1, 3, 128 },

Checked the Microchip I2C EEPROM list and added a few missing larger value chips. All parameters fit within the existing struct.

Also looked at SPI EEPROMs, and i think we will be able to make a similar SPI command with the same basic framework.

[ian]

Read, write, erase, verify EEPROM is “working”. Test, dump still to come.

I’m very unhappy with how I structured the code though, it only works with full chips so the file burned to the EEPROM must be exactly the same size as the EEPROM.

Verify is working.

TODO:

• Verify, test, dump commands
• Specify start byte and length of write (and maybe read, depending on how nasty it is)
• Allow files shorter or longer than EEPROM to be written with a warning about what is happening.
• Unify with a library of SPI EEPROMs

[ian]

Dump is working and looking nice if I may say so myself. This needs to become a library and be implemented all places that have dump functionality.

Right at the end of yesterday I found a nasty bug in the write eeprom function, and tracked it down to a small but nasty bug in the I2C library. That is now fixed and everything is humming.

ETA:

Navigating within the dump with -s (start address) and -b (number of bytes). Took a bit of work, but it aligns to the closest row to start, and adjusts the number of bytes to be complete rows.

EEPROM test complete.

ETA: did a ton of cleanup and made a bunch of stuff reusable to be implemented system wide.

• Verify, test, dump commands
• Specify start byte and length of write (and maybe read, depending on how nasty it is) I’m not doing this, for now.
• Allow files shorter or longer than EEPROM to be written with a warning about what is happening.
• Move to separate source files
• Create reusable progress indicator in ui/ folder
• Create reusable HEX dump display and implement system wide
• Move high level I2C functions to i2c_pio
• Terminal help
• Documentation
• Unify with a library of SPI EEPROMs

[ian]

There are two extremely nasty functions that handle read, write, erase, verify, test. I Know there is a much cleaner pattern for this code, but there is so much on my plate at the moment that I can’t slow down enough to see it. If any of the uber programmers out there suggest a better pattern I’m probably capable of adopting it.

I’m going to continue finishing the help, docs, etc.

Hugely Ugly Code

bool eeprom_write(struct eeprom_info *eeprom, char *buf, uint32_t buf_size, bool write_from_buf) {
   
    //Each EEPROM 8 bit address covers 256 bytes
    uint32_t address_blocks_total=eeprom_get_address_blocks_total(eeprom);
    
    //reset pointers
    uint8_t block_ptr[2] = {0x00, 0x00}; // reset the block pointer

    if(!write_from_buf){
        if(file_open(&eeprom->file_handle, eeprom->file_name, FA_READ)) return true; // create the file, overwrite if it exists, close if fails
    }

    for(uint32_t i = 0; i < address_blocks_total; i++) {
        // 256 bytes at a time, less for smaller devices (128 bytes)
        uint32_t write_size = eeprom_get_address_block_size(eeprom); 
        // calculate the start address for the current 256 byte address page  
        block_ptr[0] = eeprom_get_address_block_start(eeprom, i);
        // set any block select bits in the I2C address
        uint8_t i2caddr_7bit = eeprom_get_address_block_i2c_address(eeprom, i);

        if(!write_from_buf){
            //read the next file chunk into the buffer
            if(file_read(&eeprom->file_handle, buf, write_size))return true;
        }

        // loop over the page size
        uint32_t write_pages = write_size / eeprom->device->page_bytes;

        #if !EEPROM_DEBUG
            print_progress(i, address_blocks_total);
        #else
            printf("Block %d, I2C address 0x%02X, address 0x%02X00, %d write pages, %d bytes\r\n", i, i2caddr_7bit, block_ptr[0], write_pages, eeprom->device->page_bytes); //debug
        #endif

        
        for(uint32_t j = 0; j < write_pages; j++) {
            // write page to the EEPROM
            #if !EEPROM_DEBUG
                // BUG BUG BUG: advance block pointer by page bytes
                block_ptr[0] = (j * eeprom->device->page_bytes);
                if (eeprom_i2c_write(i2caddr_7bit<<1, block_ptr, eeprom->device->address_bytes, &buf[j*eeprom->device->page_bytes], eeprom->device->page_bytes)) {
                    printf("Error writing EEPROM at %d\r\n", i*256 + j);
                    if(!write_from_buf) file_close(&eeprom->file_handle); // close the file if there was an error
                    return true; // error
                }
                //poll for write complete
                uint32_t timeout = 0xfffffu; // default timeout for I2C operations
                while(timeout) { // wait for the write to complete
                    hwi2c_status_t i2c_result = pio_i2c_write_array_timeout(i2caddr_7bit<<1, NULL, 0, 0xfffffu);
                    if(i2c_result==HWI2C_OK){
                        break; // if the transaction was successful, we can continue
                    }
                    timeout--; // decrement the timeout
                    if(!timeout) {
                        printf("\r\nError: EEPROM write timeout at %d\r\n", i*256 + j);
                        if(!write_from_buf) file_close(&eeprom->file_handle); // close the file if there was an error
                        return true; // error
                    }
                }

            #else
                printf("%d ", j);
            #endif
        }
        #if EEPROM_DEBUG
            printf("\r\n");
        #endif   
    }

    #if !EEPROM_DEBUG
        print_progress(address_blocks_total, address_blocks_total);
    #endif
    if(!write_from_buf){
        if(file_close(&eeprom->file_handle)) return true;
    }   
    return false; // success
}

bool eeprom_read(struct eeprom_info *eeprom, char *buf, uint32_t buf_size, char *verify_buf, uint32_t verify_buf_size, bool verify, bool verify_against_verify_buf){   
      
    //Each EEPROM 8 bit address covers 256 bytes
    uint32_t address_blocks_total=eeprom_get_address_blocks_total(eeprom);
    
    //reset pointers
    uint8_t block_ptr[2] = {0x00, 0x00}; // reset the block pointer

    if(!verify && !verify_against_verify_buf) { //read contents TO file
        if(file_open(&eeprom->file_handle, eeprom->file_name, FA_CREATE_ALWAYS | FA_WRITE)) return true; // create the file, overwrite if it exists
    }else if(verify && !verify_against_verify_buf){ //verify contents AGAINST file
        if(file_open(&eeprom->file_handle, eeprom->file_name, FA_READ)) return true; // open the file for reading
    }else if(verify_against_verify_buf){ //verify contents AGAINST 0xff
        if(buf_size < EEPROM_ADDRESS_PAGE_SIZE) {
            printf("Buffer size must be at least %d bytes for EEPROM read operation\r\n", EEPROM_ADDRESS_PAGE_SIZE);
            return true; // error
        }
    }else{
        printf("Invalid parameters for EEPROM read operation\r\n");
        return true; // error
    }

    for(uint32_t i = 0; i < address_blocks_total; i++) {
        // 256 bytes at a time, less for smaller devices (128 bytes)
        uint32_t read_size = eeprom_get_address_block_size(eeprom); 
        // calculate the address for the current 256 byte address range  
        block_ptr[0] = eeprom_get_address_block_start(eeprom, i);
        uint8_t i2caddr_7bit = eeprom_get_address_block_i2c_address(eeprom, i);
        #if !EEPROM_DEBUG
            print_progress(i, address_blocks_total);
        #else
            printf("Block %d, I2C address 0x%02X, address 0x%02X00, %d bytes\r\n", i, i2caddr_7bit, block_ptr[0], read_size); //debug
        #endif
    
        // read the page from the EEPROM
        #if !EEPROM_DEBUG
            if (i2c_transaction(eeprom->device_address<<1, block_ptr, eeprom->device->address_bytes, buf, EEPROM_ADDRESS_PAGE_SIZE)) {
                printf("Error reading EEPROM at %d\r\n", i*256);
                return true; // error
            }

            if(!verify) {
                // write the read data to the file
                if(file_write(&eeprom->file_handle, buf, read_size)) return true; // write the read data to the file
            } else if(verify){
                if(!verify_against_verify_buf) { //read more data if we are not verifying erase
                    if(file_read(&eeprom->file_handle, verify_buf, read_size)) return true; 
                }
                // compare the read data with the file data (or 0xff if verifying erase)
                for(uint32_t j = 0; j < read_size; j++) {
                    // verify against file data
                    if(buf[j] != verify_buf[j]) {
                        printf("\r\nError at 0x%08X: expected 0x%02X, read 0x%02X\r\n", i*256 + j, verify_buf[j], buf[j]);
                        if(!verify_against_verify_buf) if(!file_close(&eeprom->file_handle)) return true; // close the file if there was an error
                        return true; // error
                    }
                }
            }
        #endif

    }
    #if !EEPROM_DEBUG
        print_progress(address_blocks_total, address_blocks_total);
    #endif
    if(!verify_against_verify_buf) if(file_close(&eeprom->file_handle)) return true; // close the file after writing
    return false; // success
}

[ian]

Help added, which includes the device list and info.

New two color easier to read usage help block. eeprom command reference documentation updated.

[robjwells]

@ian

two extremely nasty functions

With the enormous caveat that I’m not a C programmer (just enough to be dangerous), they don’t look especially ugly to me, just a result of having to loop over each page in each block and do something. In the write maybe you could extract the timeout-handling, but you’d have to pull in a lot of state from the existing function. In the read really it’s just handling the verify option combinations that make it look cluttered.

[henrygab]

@ian

If any of the uber programmers out there suggest a better pattern

First, the code is complex, but overall has good commenting, the called functions seems to be at a reasonable level of abstraction, and the naming of functions is reasonable.

One way to reduce the complexity of a function is to reduce what people have to track in their head when reading through it. Many folks learn about dividing a problem into smaller parts, and doing each part in a separate function.

1. Perhaps overlooked in importance is minimizing function exit points.
   • e.g., Having return from nested for loops makes it much more difficult to mentally “collapse” those loops.
   • Strongly consider redesigning to exit for loops using break or continue when something fails.
     • Which requires you define a status variable indicating that everything is still OK.
     • Add a check of that status variable in the loop condition.
   • This will move the function return points outside the for loops. This allows folk to mentally collapse those loops.
   • The next number items includes that as one small part…
2. Similar to the above, a generally useful template is:
   • Zeroth, validate arguments. Early-exit is OK here, as there’s no cleanup.
     • Wrap the parameter validation in something that collapses (e.g., if (true) { } block, or #pragma region) to further improve readability.
     • Basically, if continue past that scope, the parameters are valid.
   • First, declare variables that store resources, pre-initialized to indicate they are not allocated.
     • e.g., NULL, INVALID_HANDLE, etc.
     • Declare a bool somethingFailed = false;
   • Second, pre-allocate resources (memory buffers, open files, etc.).
     • Track failures in that somethingFailed variable.
     • Do NOT exit or cleanup.
     • Do NOT stop trying to allocate things on failure
   • Third, wrap the “real” work in an if (!somethingFailed) {} scope.
     • This scope should have ZERO function return points
       • Update somethingFailed instead
       • For loops should check that variable also
     • If you cannot structure the real work to avoid calling return, convert it into a static function that returns a bool.
   • Fourth, regardless of somethingFailed, cleanup any allocated resources.
   • NOTE: If you do these steps, you will avoid a number of bugs that the current code has…
3. much more minor, the #if !EEPROM_DEBUG and else statements increase mental load also, because it requires mentally validating no relevant side-effects and that the code paths are substantially the same (or tracking their differences).
   • Consider using the debug print macros
     • The macros use both LEVEL and FLAG, and supports each flag having its own LEVEL for output filtering (or use the global LEVEL). In other words, EEPROM_DEBUG can just be another debugger flag.
   • If you don’t want to use RTT with the debug macros, add a compile-time choice that redirects the output to use the UART instead of RTT.
     • One gotcha: Global bool needed to track if the UART is setup or not, with the debug print macro doing nothing for UART if called before the UART setup is done.
     • Second gotcha: UART output from Core1 has the potential for deadlock. Not easily fixable without thread support (note: can still be cooperative multitasking, just need to yield to allow the UART to drain).

These three small changes in how the code is structured will not only highlight (and inherently fix) some existing bugs, but should feel easier to read through when done.

The template of ParamCheck / Allocate / Work / Free is a very powerful structure for high-quality, maintainable code. The functions you list have bugs that would be caught by using that … try it and let me know if you discovered the bugs, or want any hints.

[mbrugman]

At once place I worked, we’d have a limit on Cyclomatic complexity.

We started that after trying to maintain a monster function that grew over time, and it made things easier to deal with over time.

[ian]

//poll for busy status, return false if write is complete, true if timeout
static bool spi_eeprom_poll_busy(uint8_t block_select_bits){
    uint8_t reg;
    for(uint32_t i=0; i<0xfffff; i++) {
        hwspi_write_read_cs((uint8_t[]){SPI_EEPROM_RDSR_CMD}, 1, &reg, 1); // send the read status command
        if((reg & 0x01) == 0) { // check if WIP bit is clear
            return false; // write is complete
        }
    }
    //printf("Error: EEPROM write timeout\r\n");
    return true;
}

static bool spi_eeprom_read_16(uint8_t block_select_bits, uint8_t address_bytes, uint8_t *address, char *buf) {
    //TODO: get the block select bits from the device
    hwspi_write_read_cs((uint8_t[]){SPI_EEPROM_READ_CMD|block_select_bits, address[0], address[1], address[2]}, 1+address_bytes, buf, 16); // read 16 bytes from the EEPROM
    return false;
}

static bool spi_eeprom_read_256(uint8_t block_select_bits, uint8_t address_bytes, uint8_t *address, char *buf){
    //TODO: get block select bits from the device
    hwspi_write_read_cs((uint8_t[]){SPI_EEPROM_READ_CMD|block_select_bits, address[0], address[1], address[2]}, 1+address_bytes, buf, 256); // read
    return false;
}

static bool spi_eeprom_write_page(uint8_t block_select_bits, uint8_t address_bytes, uint8_t *address, uint32_t page_size, char *buf){
    //TODO: get block select bits from the device
    hwspi_write_read_cs((uint8_t[]){SPI_EEPROM_WREN_CMD}, 1, NULL, 0); // enable write
    //TODO: this will need to be copied into a buffer with command, address, page...
    hwspi_write_read_cs((uint8_t[]){SPI_EEPROM_WRITE_CMD|block_select_bits, address[0], address[1], address[2]}, 1 + address_bytes + page_size, NULL, 0); // write 0x00 to the status register
}

struct eeprom_hal_t {
    bool (*read_16)(uint8_t block_select_bits, uint8_t address_bytes, uint8_t *address, char *buf);
    bool (*read_256)(uint8_t block_select_bits, uint8_t address_bytes, uint8_t *address, char *buf);
    bool (*write_page)(uint8_t block_select_bits, uint8_t address_bytes, uint8_t *address, uint32_t page_size, char *buf);
    bool (*write_protection_blocks)(uint8_t block_select_bits, uint8_t address_bytes, uint8_t *address, uint8_t reg);
    bool (*poll_busy)(uint8_t block_select_bits);
};

static struct eeprom_hal_t eeprom_hal[] = {
    {.read_16 = spi_eeprom_read_16,
    .read_256 = spi_eeprom_read_256,
    .write_page = spi_eeprom_write_page,
    .write_protection_blocks = NULL, // not implemented
    .poll_busy = spi_eeprom_poll_busy},
    {.read_16 = NULL, // not implemented
    .read_256 = NULL, // not implemented
    .write_page = NULL, // not implemented
    .write_protection_blocks = NULL, // not implemented
    .poll_busy = NULL} // not implemented
};

After messing with the SPI EEPROM stuff, and equipped with recent I2C EEPROM experience, this is the direction I’m headed.

• Several basic function to read 16 and 256 row/page aligned bytes. We need to read max 256 bytes because some EEPROMs have block select bits in the opcode (SPI) or I2C address, so they don’t naturally roll over to the next page without explicitly setting those block bits.
• A page write function, can’t write more than a page anyways.
• Poll for busy function
• Function to manipulate the lock bits (if present)

SPI and I2C functions are then referenced in a hardware abstraction layer struct, to be reused by the same base code for both types of devices.

[ian]

Had some time to bring it all together. Here is what I ended up with:

static struct eeprom_hal_t spi_eeprom_hal = {
    .get_address = spi_eeprom_get_address,
    .read = spi_eeprom_read,
    .write_page = spi_eeprom_write_page,
    .write_protection_blocks = NULL, // not implemented
};

A hardware abstraction layer in a struct for I2C and SPI EEPROMs. Can be expanded to FRAM, FLASH, 1WIRE, whatever.

Nasty functions slightly more readable

static bool eeprom_write(struct eeprom_info *eeprom, uint8_t *buf, uint32_t buf_size, bool write_from_buf) {

    if(!write_from_buf){
        // open the file to write, close with message if error
        if(file_open(&eeprom->file_handle, eeprom->file_name, FA_READ)) return true; 
    }

    //Each EEPROM 8 bit address covers 256 bytes
    uint32_t address_blocks_total=eeprom_get_address_blocks_total(eeprom);
    // 256 bytes at a time, less for smaller devices (128 bytes)
    uint32_t write_size = eeprom_get_address_block_size(eeprom); 
    uint32_t write_pages = write_size / eeprom->device->page_bytes; 

    for(uint32_t i = 0; i < address_blocks_total; i++) {

        if(!write_from_buf){
            //read the next file chunk into the buffer, close with message if error
            if(file_read(&eeprom->file_handle, buf, write_size))return true;
        }

        #if !EEPROM_DEBUG
            print_progress(i, address_blocks_total);
        #else
            printf("Block %d, I2C address 0x%02X, address 0x%02X00, %d write pages, %d bytes\r\n", i, i2caddr_7bit, block_ptr[0], write_pages, eeprom->device->page_bytes); //debug
        #endif
        
        for(uint32_t j = 0; j < write_pages; j++) {
            // write page to the EEPROM
            #if !EEPROM_DEBUG
                if(eeprom->hal->write_page(eeprom, (i*256)+(j*eeprom->device->page_bytes), &buf[j*eeprom->device->page_bytes])) {
                    printf("Error writing EEPROM at %d\r\n", (i*256) + j);
                    if(!write_from_buf) file_close(&eeprom->file_handle); // close the file if there was an error
                    return true; // error
                }
            #else
                printf("%d ", j);
            #endif
        }
        #if EEPROM_DEBUG
            printf("\r\n");
        #endif   
    }

    #if !EEPROM_DEBUG
        print_progress(address_blocks_total, address_blocks_total);
    #endif
    if(!write_from_buf){
        if(file_close(&eeprom->file_handle)) return true;
    }   
    return false; // success
}

enum eeprom_read_action{
    EEPROM_READ_TO_FILE = 0,
    EEPROM_VERIFY_FILE,
    EEPROM_VERIFY_BUFFER
};

static bool eeprom_read(struct eeprom_info *eeprom, char *buf, uint32_t buf_size, char *verify_buf, uint32_t verify_buf_size, enum eeprom_read_action action) {   

    // figure out what we are doing
    switch(action){
        case EEPROM_READ_TO_FILE: // read contents TO file
            if(file_open(&eeprom->file_handle, eeprom->file_name, FA_CREATE_ALWAYS | FA_WRITE)) return true; // create the file, overwrite if it exists
            break;
        case EEPROM_VERIFY_FILE: // verify contents AGAINST file
            if(file_open(&eeprom->file_handle, eeprom->file_name, FA_READ)) return true; // open the file for reading
            break;
        case EEPROM_VERIFY_BUFFER: // verify contents AGAINST buffer
            if(buf_size < EEPROM_ADDRESS_PAGE_SIZE) {
                printf("Buffer size must be at least %d bytes for EEPROM read operation\r\n", EEPROM_ADDRESS_PAGE_SIZE);
                return true; // error
            }
            break;
        default:
            printf("Invalid action for EEPROM read operation\r\n");
            return true; // error
    }
    
    //Each EEPROM 8 bit address covers 256 bytes
    uint32_t address_blocks_total=eeprom_get_address_blocks_total(eeprom);
    // 256 bytes at a time, less for smaller devices (128 bytes)
    uint32_t read_size = eeprom_get_address_block_size(eeprom);     

    for(uint32_t i = 0; i < address_blocks_total; i++) {
        #if !EEPROM_DEBUG
            print_progress(i, address_blocks_total);
        #else
            printf("Block %d, I2C address 0x%02X, address 0x%02X00, %d bytes\r\n", i, i2caddr_7bit, block_ptr[0], read_size); //debug
        #endif
    
        #if !EEPROM_DEBUG
            // read the page from the EEPROM
            if(eeprom->hal->read(eeprom, i*256, read_size, buf)) {
                printf("Error reading EEPROM at %d\r\n", i*256);
                if(action != EEPROM_VERIFY_BUFFER) {
                    file_close(&eeprom->file_handle); // close the file if there was an error
                }
                return true; // error
            }

            if(action==EEPROM_READ_TO_FILE) {
                // write the read data to the file
                if(file_write(&eeprom->file_handle, buf, read_size)) return true; // write the read data to the file
            } else {
                // read more data to verify against
                if(action==EEPROM_VERIFY_FILE) { 
                    if(file_read(&eeprom->file_handle, verify_buf, read_size)) return true; 
                }
                // compare the read data with the file data (or buf if verifying by buffer)
                for(uint32_t j = 0; j < read_size; j++) {
                    // verify against file data
                    if(buf[j] != verify_buf[j]) {
                        printf("\r\nError at 0x%08X: expected 0x%02X, read 0x%02X\r\n", i*256 + j, verify_buf[j], buf[j]);
                        if(action==EEPROM_VERIFY_FILE) file_close(&eeprom->file_handle); // close the file if there was an error
                        return true; // error
                    }
                }
            }
        #endif

    }
    #if !EEPROM_DEBUG
        print_progress(address_blocks_total, address_blocks_total);
    #endif
    if(action != EEPROM_VERIFY_BUFFER) if(file_close(&eeprom->file_handle)) return true; // close the file after writing
    return false; // success
}

• Address and block select bits handled in the HAL
• Write complete polling handled in the HAL

[henrygab]

(post deleted by author)

[ian]

Yes! But in common code so it can be cleaned up in a single place. I’m going to do 1wire eeprom tomorrow so I have a full scope of the project, then I’ll go back and polish it up.