drivers/at24cxxx: implement _mtd_at24cxxx_read_page #19270

HendrikVE · 2023-02-13T10:47:07Z

Contribution description

The function read_page was missing which lead to (from a user perspective) undefined behavior on the MTD layer.

Testing procedure

Any application using MTD in conjunction with a board with an at24cxxx.

riot-ci · 2023-02-13T11:04:02Z

Murdock results

✔️ PASSED

3837536 drivers/at24cxxx: implement read_page for mtd

Success	Failures	Total	Runtime
6865	0	6865	12m:58s

Artifacts

Documentation preview

fabian18 · 2023-02-13T11:48:58Z

The at24cxxx is actually capable of reading more than one page and even across page boundaries.
You can check quickly by comparing 7.2 Page Write and 8.3 sequential read in the datasheet.
Unless you have got one for which this is not true, but I would be surprised.
So technically a _read_page() is not necessary.
Do you nevertheless think there is a reason to offer the at24cxxx_read_page() API?

HendrikVE · 2023-02-13T17:35:50Z

So technically a _read_page() is not necessary.

I gave it a quick try. While it is true that we can read sequentially, it seems like it is not possible to read arbitrarily many bytes with that.

I tested with the following code. mtd_read_page will call at24cxxx_read_page which in turn converts the page to a position and simply calls at24cxxx_read. This will induce a sequential read.

uint8_t buf1[1024];
uint8_t buf2[1024];

for (size_t i = 0; i < sizeof(buf1); i++) {
    buf1[i] = i;
}

mtd_write_page(MTD_DEV, buf1, 0, 0, 1024);
mtd_read_page(MTD_DEV, buf2, 0, 0, 1024);

for (size_t i = 0; i < sizeof(buf2); i++) {
    printf("buf1[%d] (=%d) == buf2[%d] (=%d): %d\n", i, buf1[i], i, buf2[i], buf1[i] == buf2[i]);
}

It works fine for the first 256 bytes, after that I only get garbage:

buf1[0] (=0) == buf2[0] (=0): 1
buf1[1] (=1) == buf2[1] (=1): 1
buf1[2] (=2) == buf2[2] (=2): 1
buf1[3] (=3) == buf2[3] (=3): 1
buf1[4] (=4) == buf2[4] (=4): 1
buf1[5] (=5) == buf2[5] (=5): 1
buf1[6] (=6) == buf2[6] (=6): 1
[...]
buf1[254] (=254) == buf2[254] (=254): 1
buf1[255] (=255) == buf2[255] (=255): 1
buf1[256] (=0) == buf2[256] (=40): 0
buf1[257] (=1) == buf2[257] (=48): 0
buf1[258] (=2) == buf2[258] (=0): 0
buf1[259] (=3) == buf2[259] (=32): 0
buf1[260] (=4) == buf2[260] (=44): 0
buf1[261] (=5) == buf2[261] (=48): 0
[...]

Since 256 is a power of two I suspect some kind of technical limitation here. So this is unfortunately not feasible for the MTD layer.

The same code runs perfectly fine with the changes in this PR for reading single pages:

[...]
buf1[254] (=254) == buf2[254] (=254): 1
buf1[255] (=255) == buf2[255] (=255): 1
buf1[256] (=0) == buf2[256] (=0): 1
buf1[257] (=1) == buf2[257] (=1): 1
buf1[258] (=2) == buf2[258] (=2): 1
buf1[259] (=3) == buf2[259] (=3): 1
buf1[260] (=4) == buf2[260] (=4): 1
buf1[261] (=5) == buf2[261] (=5): 1
[...]

fabian18 · 2023-02-13T20:39:13Z

This is an interesting discovery.
I tested your function with a nucleo-f767zi and it triggered an assert() in cpu/stm32/periph/i2c_1.c:220, which is assert(dev < I2C_NUMOF && length < MAX_BYTES_PER_FRAME);, where MAX_BYTES_PER_FRAME happens to be 256.
But there is also a different I2C implementation for stm32, which is cpu/stm32/periph/i2c_2.c, which has no MAX_BYTES_PER_FRAME.

The I2C implementation is selected in cpu/stm32/periph/Makefile:

# Select the specific implementation for `periph_i2c`
ifneq (,$(filter periph_i2c,$(USEMODULE)))
  ifneq (,$(filter $(CPU_FAM),f0 f3 f7 g0 g4 l0 l4 l5 u5 wb wl))
    SRC += i2c_1.c
  else ifneq (,$(filter $(CPU_FAM),f1 f2 f4 l1))
    SRC += i2c_2.c
  else
    $(error STM32 series I2C implementation not found.)
  endif
endif

So I tested your test function on a nucleo-f446re and the test works.
Which BOARD are you using?
I think there is something unexpected happening in the I2C implementation, or you may have assert() disabled.
The garbage in buf2 could maybe not come from the EEPROM, but may be just uninitialized data because buf2 is not static. If it was static it would be initialized with 0.
With the implementation of _read_page, it could be that the broken I2C is not triggered because the page size of your at24cxxx is less than some MAX_BYTES_PER_FRAME.

benpicco

Yea that's odd, I also always assumed that read would work across page boundaries.

What's the name of that EEPROM chip?

edit oops, didn't see there are more comments already. Looks like the EEPROM chip is not the culprit then but the i2c driver? That would be the one from sam0_common in our case.

drivers/at24cxxx/mtd/mtd.c

drivers/at24cxxx/at24cxxx.c

drivers/at24cxxx/mtd/mtd.c

drivers/include/at24cxxx.h

HendrikVE · 2023-02-14T09:27:10Z

Which BOARD are you using?

It's a custom board with a SAML21 and an AT24CS08.

I think there is something unexpected happening in the I2C implementation, or you may have assert() disabled.

Assertions are enabled.

The garbage in buf2 could maybe not come from the EEPROM, but may be just uninitialized data because buf2 is not static. If it was static it would be initialized with 0.

True, I initialized them with 0 and now instead of garbage I have 0 in buf2. So it seems like it's not read at all.

Yea that's odd, I also always assumed that read would work across page boundaries.

I mean ... it does. A page has a size of 16 bytes, so 16 pages are actually read successfully before the reading process seems to stop.

HendrikVE · 2023-02-14T09:32:29Z

With ENABLE_DEBUG=1 in sam0_common:

2023-02-14 10:29:01,706 # Wait for device to be ready
2023-02-14 10:29:01,710 # Generate start condition by sending address
2023-02-14 10:29:01,712 # Wait for response.
2023-02-14 10:29:01,714 # Looping through bytes
2023-02-14 10:29:01,720 # Written byte #0 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,721 # Wait for response.
2023-02-14 10:29:01,723 # Looping through bytes
2023-02-14 10:29:01,729 # Written byte #0 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,731 # Wait for response.
2023-02-14 10:29:01,737 # Written byte #1 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,738 # Wait for response.
2023-02-14 10:29:01,744 # Written byte #2 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,746 # Wait for response.
2023-02-14 10:29:01,752 # Written byte #3 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,754 # Wait for response.
2023-02-14 10:29:01,759 # Written byte #4 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,761 # Wait for response.
2023-02-14 10:29:01,767 # Written byte #5 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,769 # Wait for response.
2023-02-14 10:29:01,774 # Written byte #6 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,776 # Wait for response.
2023-02-14 10:29:01,782 # Written byte #7 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,784 # Wait for response.
2023-02-14 10:29:01,790 # Written byte #8 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,791 # Wait for response.
2023-02-14 10:29:01,797 # Written byte #9 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,799 # Wait for response.
2023-02-14 10:29:01,805 # Written byte #10 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,806 # Wait for response.
2023-02-14 10:29:01,812 # Written byte #11 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,814 # Wait for response.
2023-02-14 10:29:01,820 # Written byte #12 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,822 # Wait for response.
2023-02-14 10:29:01,828 # Written byte #13 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,829 # Wait for response.
2023-02-14 10:29:01,835 # Written byte #14 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,837 # Wait for response.
2023-02-14 10:29:01,843 # Written byte #15 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,845 # Wait for response.
2023-02-14 10:29:01,846 # Stop sent
2023-02-14 10:29:01,848 # Wait for device to be ready
2023-02-14 10:29:01,852 # Generate start condition by sending address
2023-02-14 10:29:01,853 # Wait for response.
2023-02-14 10:29:01,855 # Looping through bytes
2023-02-14 10:29:01,861 # Written byte #0 to data reg, now waiting for DR to be empty again
2023-02-14 10:29:01,863 # Wait for response.
2023-02-14 10:29:01,865 # Wait for device to be ready
2023-02-14 10:29:01,869 # Generate start condition by sending address
2023-02-14 10:29:01,871 # Wait for response.
2023-02-14 10:29:01,873 # Wait for response.
2023-02-14 10:29:01,874 # Wait for response.
2023-02-14 10:29:01,876 # Wait for response.
2023-02-14 10:29:01,878 # Wait for response.
[...]
2023-02-14 10:29:03,588 # Wait for response.
2023-02-14 10:29:03,589 # Stop sent

benpicco · 2023-02-14T13:57:09Z

drivers/at24cxxx/at24cxxx.c

+    int check = 0;
+
+    while (len) {
+        size_t clen = MIN(len, DEV_PAGE_SIZE - MOD_POW2(pos, DEV_PAGE_SIZE));


So this would also work?

Suggested change

size_t clen = MIN(len, DEV_PAGE_SIZE - MOD_POW2(pos, DEV_PAGE_SIZE));

size_t clen = MIN(len, 255);

Yes, if it is written like this, with a limited read, it works:

int at24cxxx_read_page(const at24cxxx_t *dev, uint32_t page, uint32_t offset, void *data, size_t len) { size_t pos = page * DEV_PAGE_SIZE + offset; size_t clen = MIN(len, 256 - MOD_POW2(pos, 256)); return at24cxxx_read(dev, pos, data, clen) == AT24CXXX_OK ? (int)clen : -EIO; }

That is only a fix specific to this CPU's I2C implementation though, but it is an improvement nevertheless. We could only do it when e.g. MODULE_SAM0_COMMON_PERIPH is defined?

Why do you need to subtract MOD_POW2(pos, 256)?

Also as @fabian18 pointed out the STM32 driver also has this limitation, so it might be more common. Reading up to 256 bytes at once is certainly an improvement to just reading 16 bytes (or failing).

Why do you need to subtract MOD_POW2(pos, 256)?

Oh I don't, that was just some bad copy pasta.

Also as @fabian18 pointed out the STM32 driver also has this limitation, so it might be more common. Reading up to 256 bytes at once is certainly an improvement to just reading 16 bytes (or failing).

Other implementations may have even lower limits. Should we put a warning?

Other implementations may have even lower limits.

I doubt that.
I can see how some (HW) implementations would limit the transfer size to 1 byte length. I can't really see how less than 1 byte would make sense.

I doubt that.
I can see how some (HW) implementations would limit the transfer size to 1 byte length. I can't really see how less than 1 byte would make sense.

1 byte?

255 = 8 bit = 1 byte

But we are reading 256 bytes, not bits? I'm confused.

If your length variable is 1 byte it can hold a maximum of 8 bits which can represent a value of up to 255

Ah now I got it, you are talking about I2C implementations using e.g. uint8_t for the length, right?

fabian18

If we want to hotfix this, I would introduce an internal AT24CXXX_I2C_READ_MAX which may be defined by a board and a function:

int _read_max(..., len) {
#ifdef AT24CXXX_I2C_READ_MAX
    while(len) {
        int l = _read(..., AT24CXXX_I2C_READ_MAX);
        len -= len;
       ....
    }
#else
    _read(..., len)
#endif
}

drivers/at24cxxx/at24cxxx.c

benpicco · 2023-02-14T15:29:31Z

If we want to hotfix this, I would introduce an internal AT24CXXX_I2C_READ_MAX

It's not really specific to at24 though, it's a property of the I2C implementation.
So we should have a PERIPH_I2C_MAX_BYTES_PER_FRAME instead that gets set by periph_cpu.h

fabian18 · 2023-02-14T15:34:17Z

Ok so basically export something like MAX_BYTES_PER_FRAME of i2c_1.c and drivers must deal with it. That´s better, yes

cpu/stm32/include/periph/f3/periph_cpu.h

drivers/at24cxxx/at24cxxx.c

drivers/at24cxxx/mtd/mtd.c

benpicco · 2023-02-14T22:45:22Z

drivers/at24cxxx/at24cxxx.c

+            data_p += clen;
+        }
+        else {
+            break;


Not

Suggested change

break;

return -EIO;

?

I would have thought that we do not want the operation to be "all or nothing" and return at least bytes we successfully read. That is also the answer to your question why there is the extra function at24cxxx_read_max: So we can return the bytes that were read instead of just returning AT24CXXX_OK as read_page in MTD needs to return the number of bytes read?

But it's hiding the error if one happened.
Also, what are you going to do with half a transaction? In case of the MTD layer, it will just retry because it expects read_page to return less than what was requested in case of boundary condition.

With this, in case of an error, this would just return 0 and we will spin forever.

So it should be the alternative that I proposed?

static int _mtd_at24cxxx_read_page(mtd_dev_t *mtd, void *dest, uint32_t page, uint32_t offset, uint32_t size) { return at24cxxx_read(DEV(mtd), page * (mtd->page_size) + offset, dest, size) == AT24CXXX_OK ? (int)size : -EIO; }

Also, what are you going to do with half a transaction?

MTD could request the second half in case something similar happens like only being able to read 256 bytes.

BTW even when changing that, we can still input len=0 and get a return of 0 and not -EIO, because we "successfully" read 0 bytes. If this is an issue we should forbid it I guess?

mtd_read_page() does while (count) { so the len = 0 case should never make it to the driver.

drivers/include/at24cxxx.h

benpicco

looks good to me

drivers/at24cxxx/mtd/mtd.c

…tions

benpicco · 2023-02-15T12:38:31Z

bors merge

19270: drivers/at24cxxx: implement _mtd_at24cxxx_read_page r=benpicco a=HendrikVE ### Contribution description The function `read_page` was missing which lead to (from a user perspective) undefined behavior on the MTD layer. ### Testing procedure Any application using MTD in conjunction with a board with an at24cxxx. Co-authored-by: Hendrik van Essen <hendrik.vanessen@ml-pa.com>

benpicco · 2023-02-15T12:39:42Z

bors cancel

bors · 2023-02-15T12:39:44Z

Canceled.

benpicco · 2023-02-15T12:40:26Z

bors merge

19270: drivers/at24cxxx: implement _mtd_at24cxxx_read_page r=benpicco a=HendrikVE ### Contribution description The function `read_page` was missing which lead to (from a user perspective) undefined behavior on the MTD layer. ### Testing procedure Any application using MTD in conjunction with a board with an at24cxxx. 19280: sys/shell/cmds: fix typo 'shell_cmd_grnc_udp' r=benpicco a=benpicco Co-authored-by: Hendrik van Essen <hendrik.vanessen@ml-pa.com> Co-authored-by: Benjamin Valentin <benpicco@beuth-hochschule.de>

bors · 2023-02-15T12:43:35Z

Build failed (retrying...):

Murdock

19270: drivers/at24cxxx: implement _mtd_at24cxxx_read_page r=benpicco a=HendrikVE ### Contribution description The function `read_page` was missing which lead to (from a user perspective) undefined behavior on the MTD layer. ### Testing procedure Any application using MTD in conjunction with a board with an at24cxxx. Co-authored-by: Hendrik van Essen <hendrik.vanessen@ml-pa.com>

bors · 2023-02-15T12:46:22Z

Build failed:

Murdock

benpicco · 2023-02-15T17:02:11Z

bors merge

19270: drivers/at24cxxx: implement _mtd_at24cxxx_read_page r=benpicco a=HendrikVE ### Contribution description The function `read_page` was missing which lead to (from a user perspective) undefined behavior on the MTD layer. ### Testing procedure Any application using MTD in conjunction with a board with an at24cxxx. Co-authored-by: Hendrik van Essen <hendrik.vanessen@ml-pa.com>

benpicco · 2023-02-15T17:03:09Z

bors cancel
bors merge

bors · 2023-02-15T17:03:11Z

Canceled.

bors · 2023-02-15T20:16:25Z

Build succeeded:

github-actions bot added the Area: drivers Area: Device drivers label Feb 13, 2023

HendrikVE added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 13, 2023

benpicco reviewed Feb 13, 2023

View reviewed changes

drivers/at24cxxx/mtd/mtd.c Outdated Show resolved Hide resolved

drivers/at24cxxx/at24cxxx.c Outdated Show resolved Hide resolved

drivers/at24cxxx/mtd/mtd.c Outdated Show resolved Hide resolved

drivers/include/at24cxxx.h Outdated Show resolved Hide resolved

HendrikVE force-pushed the pr/drivers/at24cxxx-implement_read_page branch from bd3a74a to 773202c Compare February 14, 2023 13:55

benpicco reviewed Feb 14, 2023

View reviewed changes

fabian18 reviewed Feb 14, 2023

View reviewed changes

drivers/at24cxxx/at24cxxx.c Outdated Show resolved Hide resolved

drivers/at24cxxx/at24cxxx.c Outdated Show resolved Hide resolved

fabian18 mentioned this pull request Feb 14, 2023

cpu/stm32/periph/i2c: export PERIPH_I2C_MAX_BYTES_PER_FRAME #19279

Merged

HendrikVE force-pushed the pr/drivers/at24cxxx-implement_read_page branch from 773202c to 61027ab Compare February 14, 2023 18:00

HendrikVE requested review from aabadie, fjmolinas, DipSwitch, vincent-d, dylad and keestux as code owners February 14, 2023 18:00

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Feb 14, 2023

HendrikVE changed the title ~~drivers/at24cxxx: implement read_page~~ drivers/at24cxxx: implement _mtd_at24cxxx_read_page Feb 14, 2023

benpicco reviewed Feb 14, 2023

View reviewed changes

cpu/stm32/include/periph/f3/periph_cpu.h Outdated Show resolved Hide resolved

benpicco reviewed Feb 14, 2023

View reviewed changes

HendrikVE force-pushed the pr/drivers/at24cxxx-implement_read_page branch from 61027ab to d3f0cf5 Compare February 15, 2023 10:37

benpicco approved these changes Feb 15, 2023

View reviewed changes

drivers/at24cxxx/mtd/mtd.c Outdated Show resolved Hide resolved

cpu/sam0_common: add PERIPH_I2C_MAX_BYTES_PER_FRAME due to I2C limita…

9b0cf96

…tions

drivers/at24cxxx: implement read_page for mtd

3837536

HendrikVE force-pushed the pr/drivers/at24cxxx-implement_read_page branch from d3f0cf5 to 3837536 Compare February 15, 2023 10:56

bors bot merged commit 5667814 into RIOT-OS:master Feb 15, 2023

HendrikVE deleted the pr/drivers/at24cxxx-implement_read_page branch March 1, 2023 10:58

MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023

	size_t clen = MIN(len, DEV_PAGE_SIZE - MOD_POW2(pos, DEV_PAGE_SIZE));
	size_t clen = MIN(len, 255);

drivers/at24cxxx: implement _mtd_at24cxxx_read_page #19270

drivers/at24cxxx: implement _mtd_at24cxxx_read_page #19270

Conversation

HendrikVE commented Feb 13, 2023

Contribution description

Testing procedure

riot-ci commented Feb 13, 2023 • edited

Murdock results

Artifacts

fabian18 commented Feb 13, 2023

HendrikVE commented Feb 13, 2023 • edited

fabian18 commented Feb 13, 2023 • edited

benpicco left a comment • edited

Choose a reason for hiding this comment

HendrikVE commented Feb 14, 2023

HendrikVE commented Feb 14, 2023

benpicco Feb 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian18 left a comment

Choose a reason for hiding this comment

benpicco commented Feb 14, 2023

fabian18 commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HendrikVE Feb 15, 2023 • edited

Choose a reason for hiding this comment

benpicco Feb 15, 2023 • edited

Choose a reason for hiding this comment

benpicco left a comment

Choose a reason for hiding this comment

benpicco commented Feb 15, 2023

benpicco commented Feb 15, 2023

bors bot commented Feb 15, 2023

benpicco commented Feb 15, 2023

bors bot commented Feb 15, 2023

bors bot commented Feb 15, 2023

benpicco commented Feb 15, 2023

benpicco commented Feb 15, 2023

bors bot commented Feb 15, 2023

bors bot commented Feb 15, 2023

riot-ci commented Feb 13, 2023 •

edited

HendrikVE commented Feb 13, 2023 •

edited

fabian18 commented Feb 13, 2023 •

edited

benpicco left a comment •

edited

benpicco Feb 14, 2023 •

edited

HendrikVE Feb 15, 2023 •

edited

benpicco Feb 15, 2023 •

edited