Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Eight-Bit-Addressing mode to I2CEEBlockDevice. #12446

Merged
merged 11 commits into from
Feb 21, 2020
Merged

Add Eight-Bit-Addressing mode to I2CEEBlockDevice. #12446

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cd34860
Add Eight-Bit-Adressing mode to I2CEEBlockDevice.
boomer41 Feb 16, 2020
8d1978e
I2CEEBlockDevice: Add paging to eight bit mode
boomer41 Feb 17, 2020
52aed22
Refactor paged_handler to directly give the paged device address.
boomer41 Feb 17, 2020
e850984
Correctly check return codes from bytewise write function of I2C.
boomer41 Feb 17, 2020
267d8cc
Preserve original size as it is modified inside the handler function.
boomer41 Feb 17, 2020
76a177f
Use pageSize-constant instead of hardcoded value
boomer41 Feb 17, 2020
7c589ae
Fix astyle
boomer41 Feb 17, 2020
532f378
Remove overcomplicated code from I2CEEBlockDevice
boomer41 Feb 18, 2020
3741440
Fix typo and clarify documentation
boomer41 Feb 19, 2020
4d0c346
Fix Code style and reorder class members to get besser packing
boomer41 Feb 19, 2020
440fa49
Remove paging and sync from I2CEEBlockDevice::read(), as it is not ne…
boomer41 Feb 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 67 additions & 24 deletions components/storage/blockdevice/COMPONENT_I2CEE/I2CEEBlockDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,21 @@ using namespace mbed;

I2CEEBlockDevice::I2CEEBlockDevice(
PinName sda, PinName scl, uint8_t addr,
bd_size_t size, bd_size_t block, int freq)
: _i2c_addr(addr), _size(size), _block(block)
bd_size_t size, bd_size_t block, int freq,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bool address_is_eight_bit)
: _i2c_addr(addr), _size(size), _block(block),
_address_is_eight_bit(address_is_eight_bit)
{
_i2c = new (_i2c_buffer) I2C(sda, scl);
_i2c->frequency(freq);
}

I2CEEBlockDevice::I2CEEBlockDevice(
I2C *i2c_obj, uint8_t addr,
bd_size_t size, bd_size_t block)
: _i2c_addr(addr), _size(size), _block(block)
bd_size_t size, bd_size_t block,
bool address_is_eight_bit)
: _i2c_addr(addr), _size(size), _block(block),
_address_is_eight_bit(address_is_eight_bit)
{
_i2c = i2c_obj;
}
Expand All @@ -58,48 +62,76 @@ 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));

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

if (!_i2c->write(_i2c_addr | 0) ||
!_i2c->write((char)(addr >> 8)) ||
!_i2c->write((char)(addr & 0xff))) {
return BD_ERROR_DEVICE_ERROR;
}
while (size > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uint32_t off = addr % _block;
uint32_t chunk = (off + size < _block) ? size : (_block - off);

_i2c->start();

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

auto err = _sync();
if (err) {
return err;
}
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();

auto err = _sync();

if (err) {
return err;
}

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

if (0 != _i2c->read(_i2c_addr, static_cast<char *>(buffer), size)) {
return BD_ERROR_DEVICE_ERROR;
addr += chunk;
size -= chunk;
pBuffer += chunk;
}

return 0;
return BD_ERROR_OK;
}

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

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

// While we have some more data to write.
while (size > 0) {
uint32_t off = addr % _block;
uint32_t chunk = (off + size < _block) ? size : (_block - off);

_i2c->start();

if (!_i2c->write(_i2c_addr | 0) ||
!_i2c->write((char)(addr >> 8)) ||
!_i2c->write((char)(addr & 0xff))) {
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;
}

for (unsigned i = 0; i < chunk; i++) {
_i2c->write(static_cast<const char *>(buffer)[i]);
if (1 != _i2c->write(pBuffer[i])) {
return BD_ERROR_DEVICE_ERROR;
}
}

_i2c->stop();
Expand All @@ -112,10 +144,10 @@ int I2CEEBlockDevice::program(const void *buffer, bd_addr_t addr, bd_size_t size

addr += chunk;
size -= chunk;
buffer = static_cast<const char *>(buffer) + chunk;
pBuffer += chunk;
}

return 0;
return BD_ERROR_OK;
}

int I2CEEBlockDevice::erase(bd_addr_t addr, bd_size_t size)
Expand Down Expand Up @@ -164,3 +196,14 @@ const char *I2CEEBlockDevice::get_type() const
{
return "I2CEE";
}

uint8_t I2CEEBlockDevice::get_paged_device_address(const bd_addr_t &address)
{
if (!this->_address_is_eight_bit) {
return this->_i2c_addr;
} else {
// Use the three least significant bits of the 2nd byte as the page
// The page will be bits 2-4 of the user defined addresses.
return this->_i2c_addr | ((address & 0x0700u) >> 7u);
}
}
50 changes: 35 additions & 15 deletions components/storage/blockdevice/COMPONENT_I2CEE/I2CEEBlockDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,29 +59,37 @@ class I2CEEBlockDevice : public BlockDevice {
public:
/** Constructor to create an I2CEEBlockDevice on I2C pins
*
* @param sda The pin name for the sda line of the I2C bus.
* @param scl The pin name for the scl line of the I2C bus.
* @param addr The 8bit I2C address of the chip, common range 0xa0 - 0xae.
* @param size The size of the device in bytes
* @param block The page size of the device in bytes, defaults to 32bytes
* @param freq The frequency of the I2C bus, defaults to 400K.
* @param sda The pin name for the sda line of the I2C bus.
* @param scl The pin name for the scl line of the I2C bus.
* @param addr The 8bit I2C address of the chip, common range 0xa0 - 0xae.
* @param size The size of the device in bytes
* @param block The page size of the device in bytes, defaults to 32bytes
* @param freq The frequency of the I2C bus, defaults to 400K.
* @param address_is_eight_bit Specifies whether the EEPROM device is using eight bit
* addresses instead of 16 bit addresses. This is used for example
* in AT24C series chips.
*/
I2CEEBlockDevice(
PinName sda, PinName scl, uint8_t address,
bd_size_t size, bd_size_t block = 32,
int bus_speed = 400000);
int bus_speed = 400000,
bool address_is_eight_bit = false);

/** Constructor to create an I2CEEBlockDevice on I2C pins
*
* @param i2c The I2C instance pointer
* @param addr The 8bit I2C address of the chip, common range 0xa0 - 0xae.
* @param size The size of the device in bytes
* @param block The page size of the device in bytes, defaults to 32bytes
* @param freq The frequency of the I2C bus, defaults to 400K.
*/
*
* @param i2c The I2C instance pointer
* @param addr The 8bit I2C address of the chip, common range 0xa0 - 0xae.
* @param size The size of the device in bytes
* @param block The page size of the device in bytes, defaults to 32bytes
* @param freq The frequency of the I2C bus, defaults to 400K.
* @param address_is_eight_bit Specifies whether the EEPROM device is using eight bit
* addresses instead of 16 bit addresses. This is used for example
* in AT24C series chips.
*/
I2CEEBlockDevice(
mbed::I2C *i2c_obj, uint8_t address,
bd_size_t size, bd_size_t block = 32);
bd_size_t size, bd_size_t block = 32,
bool address_is_eight_bit = false);

/** Destructor of I2CEEBlockDevice
*/
Expand Down Expand Up @@ -169,7 +177,19 @@ class I2CEEBlockDevice : public BlockDevice {
uint32_t _size;
uint32_t _block;

bool _address_is_eight_bit;

int _sync();

/**
* Gets the device's I2C address with respect to the requested page.
* When eight bit mode is disabled, this function is a noop.
* When eight bit mode is enabled, it sets the bits required
* in the devices address. Other bits remain unchanged.
* @param address An address in the requested page.
* @return The device's I2C address for that page
*/
uint8_t get_paged_device_address(const bd_addr_t &address);
};


Expand Down