Add Eight-Bit-Addressing mode to I2CEEBlockDevice.

[boomer41]

Summary of changes

When dealing with EEPROMs without a 16 bit adressing, the current
implementation does not work, as it writes a 16 bit address to the chip.
This may cause undefined behaviour.
This change adds a new constructor argument to enable this new eight-bit
mode. It defaults to false to not break existing code.
This constructor argument should actually never be necessary to manually
set, except when dealing with cheap devices.

An example for such a device is the Hua Hong NEC K24C02. Datasheet.

Impact of changes

Added a new optional argument at the end of all existing constructors.

Migration actions required

None, as the new constructor argument is optional and at the end.
The default value does not change functionality in existing code.

Documentation

Online documentation for I2CEEBlockDevice should feature the new constructors.

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

[ciarmcom]

@boomer41, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

[VeijoPesonen]

Looks good to me

[SeppoTakalo]

Is this implementation now only working for 2kb version of the chip?
But with the 8-bit address, there is 4kb and bigger versions of the chip, where the high bits of the address is muxed with the device address.

Example: How A0,A1&A2 are used as a page addresses on bigger devices

Can you fix your implementation so that with the same API, we are able to support 2K, 4K, 8K and 16K versions of this chip?

[boomer41]

Can you fix your implementation so that with the same API, we are able to support 2K, 4K, 8K and 16K versions of this chip?

@SeppoTakalo I initially thought about this, but I don't know if this adressing scheme is only used on this particular chip, or if it is more of a "standard".

It looks kind of odd when we add a "Hua Hong NEC K24C02"-Mode just for this IC.
I thought an 8-Bit adressing-mode is kind of useful for others, but I don't think adding a special mode for this particular IC makes any sense.
When one does need the larger versions of this IC, we could easily create multiple instances of I2CEEBlockDevice with different addresses in 8bit mode and handle the paging this way.

[SeppoTakalo]

@boomer41 I'm not too familiar with these I2C EEPROM's, so I did a bit googling..

To me, looks like this 8-bit mode is actually same as what Microchip/Atmel AT24C uses. See http://ww1.microchip.com/downloads/en/DeviceDoc/doc3256.pdf

Then the 16-bit addressing mode seems to be used with Microchip 24AA256/24LC256/24FC256 chips. See: http://ww1.microchip.com/downloads/en/devicedoc/21203m.pdf

So, in fact, the driver might have never worked with AT24C series chips.. The documentation just claims, but maybe it has been since refactored, and now broken, because probably has not been used for a while.

So, I would recommend that while you are at it, just take a peek of those 3 datasheets and fix the driver for all of them. Seem like the "8-bit addressing mode" should be described in the header as Microchip/Atmel AT24-series compatible. Then the 16-bit mode is Microchip 24-serial compatible.
Seems like the difference is just if the device name starts with "AT", it comes from the previously Atmel series, and uses different mode.

[SeppoTakalo]

Also, ST M24C series seems to be using the same 8-bit mode:
https://www.st.com/resource/en/datasheet/m24c08-f.pdf

[boomer41]

@SeppoTakalo Well then that's a whole different story.
I'll probably do this this evening. Shouln't take that long.

[boomer41]

@SeppoTakalo I have now added the said functionality.
Please take a good look upon pagination code, as I currently only have the 2k ICs :(

Pull request has been modified.

[boomer41]

I have also noticed that the write functions were incorrectly used. Checking for non-zero return code is wrong because '2' means timeout.

[boomer41]

Can someone please explain the astyle rules? Sometimes it wants the curly brackets on the same line as the if, sometimes it wants it in the next line. WTH?

[VeijoPesonen]

I don't know the reasoning behind the rules but you can run following command to fix all possible astyle issues related to your changes:

git diff --name-only $(git merge-base origin/master HEAD) | ( grep '.\(c\|cpp\|h\|hpp\)$' || true ) | ( grep -v -f .astyleignore || true ) | while read file; do astyle -n --options=.astylerc "${file}"; done

[SeppoTakalo]

Can someone please explain the astyle rules? Sometimes it wants the curly brackets on the same line as the if, sometimes it wants it in the next line. WTH?

Coding style is explained here: https://os.mbed.com/docs/mbed-os/v5.15/contributing/style.html
as well as how to run it for one specific file.

Try:

astyle -n --options=.astylerc components/storage/blockdevice/COMPONENT_I2CEE/I2CEEBlockDevice.*

[0xc0170]

CI started

[boomer41]

I have now removed the overcomplicated code.
Good to have someone actually looking at it.
I honestly didn't see that i could just shift the address to get the page. Should've just jumped at me.

The new code now does breaks against block sizes.

Sorry for all the mess caused with that overcomplicatedness.

[SeppoTakalo]

Code seems much simpler now.

I tried with following example:

int main()
{
    printf("i2cee test\n");
    I2CEEBlockDevice i2cee(D14, D15, 0xa0, 256, 1, 400000, true);
    i2cee.init();
    // Write "Hello World!" to the first block
    char buffer[256] = {0};
    strcat(buffer, __FILE__ " : " "Hello World. Build in " __DATE__ " " __TIME__);
    i2cee.program(buffer, 0, strlen(buffer)+1);
    memset(buffer, 0, 100);
    // Read back what was stored, read the whole device to buffer
    i2cee.read(buffer, 0, 256);
    printf("Content: %s\n", buffer);
    // Deinitialize the device
    i2cee.deinit();
}

The code seems to work OK when writing, but did not properly read. I'm not sure why. I suggest that you simplify the read operation to non-paged version, unless there is a clear requirement by some device.

[SeppoTakalo]

Are you sure that the read operation have to be paged for some devices?

If tried with AT24MAC and read operation as simple as:

int I2CEEBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)
{
    // Check the address and size fit onto the chip.
    MBED_ASSERT(is_valid_read(addr, size));

    auto *pBuffer = reinterpret_cast<char *>(buffer);

    _i2c->start();

    if (1 != _i2c->write(get_paged_device_address(addr))) {
        return BD_ERROR_DEVICE_ERROR;
    }

    if (!_address_is_eight_bit && 1 != _i2c->write((char)(addr >> 8u))) {
        return BD_ERROR_DEVICE_ERROR;
    }

    if (1 != _i2c->write((char)(addr & 0xffu))) {
        return BD_ERROR_DEVICE_ERROR;
    }

    _i2c->stop();

    if (0 != _i2c->read(_i2c_addr, pBuffer, size)) {
        return BD_ERROR_DEVICE_ERROR;
    }

    return BD_ERROR_OK;
}

And it works.. For me, it seems that only the write operation have to be single byte or full page, but no more. Read operation can be as large as the whole device.

This seems to be true for at least AT24C and Microchip 24XX devices:
http://ww1.microchip.com/downloads/en/devicedoc/21203m.pdf

To provide sequential reads, the 24XX256 contains an internal address
pointer which is incremented by one at the completion
of each operation. This address pointer allows the
entire memory contents to be serially read during one
operation. The internal address pointer will
automatically roll over from address 7FFF to address
0000 if the master acknowledges the byte received
from the array address 7FFF

[kjbracey]

In my experience, I've never seen paging limitation on reads.

[boomer41]

Unfortunately i can't check because I only have the 2k variants.
When you're sure there's no limitation, I'll remove that one.

[SeppoTakalo]

I checked, your paged read seems to work OK once you remove the _sync() call. That will reset the device's internal address, runing all writes.

You should not need _sync() in the read operation, as that was already done in program().

[kjbracey]

Think you've gone overboard and now have an excess blank line :)

(Any Git diff should show both cases in red normally).

[boomer41]

Oops, I didn't know where you wanted the NL.
Where exactly did you want the NL?
Thanks.

[kjbracey]

It's possible I'm being misled by GitHub's view, but I think originally you had an unterminated final } line. I think you went to the bottom and pressed "Enter" twice rather than once, so you now have an excess blank line.

[boomer41]

You should not need _sync() in the read operation, as that was already done in program().

@SeppoTakalo Funny, because adding _sync() to the read operation was necessary to get reliable data from my cheapo devices.
I'll check this evening if it works now.

[SeppoTakalo]

You should not need _sync() in the read operation, as that was already done in program().

@SeppoTakalo Funny, because adding _sync() to the read operation was necessary to get reliable data from my cheapo devices.
I'll check this evening if it works now.

This is strange..
If you check the _sync() operation, that is what manuals describe as ACKNOWLEDGE POLLING

Ref: http://ww1.microchip.com/downloads/en/devicedoc/21203m.pdf

I would suspect that the write operation clear out the address that we previously wrote to the device, because it effectively starts a new operation to the device.
If you think it is needed, it should be before we initiate the actual address writing to the device.

So basically:

1. _sync()
2. _i2c->start();
3. Write device address
4. write byte address
5. _i2c->stop();

But I still wonder why we need it, because we call _sync() after each page program.

[kjbracey]

This doesn't directly relate to this PR, but this driver should now really have a constructor that takes const i2c_pinmap_t & and passes to I2C to offer the possibility to avoid pulling in pinmap tables.

It would be nice to add this, but I guess as a follow-up PR.

[boomer41]

Yes, I think this should be a separate PR as it actually has nothing to do with the 8bit-mode.

[boomer41]

But I still wonder why we need it, because we call _sync() after each page program.

Well i added it between writing the address-to-read to the device and actually reading from it.
I guess my cheapo devices were just too slow. I'll try this this evening, with slowing down the bus frequency.
I only use this as a configuration rom, so this would be ok too.

[boomer41]

@SeppoTakalo Well it seems to work now too with the now pushed code.
Guess I did something weird in the beginning and fixed it midway.

[SeppoTakalo]

Looks good.
Thank you for the contribution.

[0xc0170]

CI started

[0xc0170]

CI did not start, restarted

[mbed-ci]

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts