Add package tiny-vcdiff #17797

jue89 · 2022-03-11T12:13:01Z

Contribution description

This is a WIP PR that brings tiny-vcdiff into RIOT.
I mentioned this library during my talk on the last RIOT Summit

Currently this PR supports mtd as backend. But a vfs will be added, too. I guess those are the obvious interfaces for such a package.

Testing procedure

I will add a test later, that will make use of all implemented backends.

Issues/PRs references

pkg/tinyvcdiff/doc.txt

pkg/tinyvcdiff/include/vcdiff_mtd.h

aabadie · 2022-03-13T12:23:20Z

I'm realizing that there's no test application for this new package. So it won't be built by the CI. I would be nice to add a simple one (you can use make generate-test)

jue89 · 2022-03-13T20:39:29Z

I'm realizing that there's no test application for this new package. So it won't be built by the CI. I would be nice to add a simple one (you can use make generate-test)

Yep, I'm currently working on that. The reason why this is a draft PR.

jue89 · 2022-03-25T18:02:25Z

Writing the test revealed the bugs described in #17865 and #17866. Waiting for them to be on master.

benpicco · 2022-03-26T15:39:08Z

You can probably squash when you rebase

jue89 · 2022-03-26T18:39:01Z

I think everything should be in place, now. I'm ready to be ~~roasted~~ reviewed 🙈

pkg/tinyvcdiff/contrib/tinyvcdiff_mtd/mtd.c

pkg/tinyvcdiff/contrib/tinyvcdiff_vfs/vfs.c

tests/pkg_tinyvcdiff/main.c

benpicco

Nice, works like a charm - some more nits:

pkg/tinyvcdiff/contrib/tinyvcdiff_mtd/mtd.c

pkg/tinyvcdiff/contrib/tinyvcdiff_vfs/vfs.c

tests/pkg_tinyvcdiff/main.c

tests/pkg_tinyvcdiff/fakemtd.h

tests/pkg_tinyvcdiff/app.config.test

fjmolinas

Seems CI is mostly happy, but this will need some blacklisting

tests/pkg_tinyvcdiff/fakemtd.h

jue89 · 2022-03-28T11:43:31Z

Thank you for your reviews, @benpicco @fjmolinas!

Is it okay if I squash all fixups before addressing your suggestions?

jue89 · 2022-03-30T12:43:46Z

Rebased on latest master.

fjmolinas · 2022-03-30T14:39:41Z

pkg/tinyvcdiff/doc.txt

+ *     vcdiff_vfs_t target = VCDIFF_VFS_INIT(fd_target);
+ *     vcdiff_set_target_driver(&vcdiff, &vcdiff_vfs_driver, &target);
+ *
+ *     rc = vcdiff_apply_delta(&vcdiff, data, data_len);


Is there a requirement on how this needs to be fragmented?

Nope, It should work byte-wise. Like shown here.

If it doesn't, it's a bug ;-)

jue89 · 2022-03-30T15:57:53Z

Has anyone an idea how to handle this CI error?
cc1: error: unrecognized command line option '-Wno-implicit-fallthrough' [-Werror]

benpicco · 2022-03-30T16:03:13Z

Can you make the fall-throughs explicit with /* fall-through */?

jue89 · 2022-03-30T16:19:57Z

Well, I could. But that produces really noisy code :-/

fjmolinas · 2022-03-30T16:23:15Z

pkg/tinyvcdiff/contrib/tinyvcdiff_mtd/mtd.c

+    /* mtd_read_page will take care of calculation the right page
+     * it's safe to call mtd_read_page_raw: the erase driver already
+     * prepared the sector for writing */
+    int rc = mtd_write_page_raw(mtd->dev, src, 0, offset, len);


Are you sure this is right? The input offset is offset in bytes, while the offset parameter to mtd_write_page_raw is the offset in the page being written to, shouldn't it be something like:

Suggested change

int rc = mtd_write_page_raw(mtd->dev, src, 0, offset, len);

uint32_t page = offset / mtd->dev->page_size;

uint32_t page_offset = (offset % mtd->dev->page_size);

int rc = mtd_write_page_raw(mtd->dev, src, page, page_offset, len);

(untested)

Same question goes for the read function actually.

It should work to start at page 0 and let the MTD implemenation figure out the right page.

But that is only if there is write_page, not the case for mtd_flashpage for example.

Ah sorry, my bad, I misunderstood the API I think.

Well, but another problem could occur: un-aligned writes (i.e. addr % FLASHPAGE_WRITE_BLOCK_ALIGNMENT and size % FLASHPAGE_WRITE_BLOCK_SIZE are possible troublemaker ...). Or am I wrong?

yea but you don't get those through MTD - #17683 tries to fix that

I added write alignment for the MTD backend: b705803

jue89 · 2022-03-31T13:30:41Z

I think it would be nice to have working MTD ;-) Thus, I'm waiting for #17683.

jue89 · 2022-04-05T11:34:54Z

I added write alignment for this package. Once #17683 is merged, I'll add a check if the configured write alignment and MTD write alignment are compatible.

jue89 · 2022-04-05T11:36:29Z

Can you make the fall-throughs explicit with /* fall-through */?

I sneaked that in with some pre-processor magic. The package can now be compiled without -Wno-implicit-fallthrough.

jue89 · 2022-04-05T16:35:50Z

Finally a happy Murdock :)

jue89 · 2022-05-17T11:54:02Z

Rebased on latest master. Still looking fine.

@fjmolinas You faced problems with alignment on MTD flashpage, right? Could you test this PR again? I've introduced aligned writes for MTD backends.

benpicco

With a rebase you can now get rid of CONFIG_TINYVCDIFF_MTD_WRITE_SIZE

benpicco · 2022-05-23T10:28:13Z

pkg/tinyvcdiff/contrib/tinyvcdiff_mtd/mtd.c

+    size_t alignment_offset;
+    size_t copy_len;
+
+    alignment_offset = mtd->offset % CONFIG_TINYVCDIFF_MTD_WRITE_SIZE;


Suggested change

alignment_offset = mtd->offset % CONFIG_TINYVCDIFF_MTD_WRITE_SIZE;

alignment_offset = mtd->offset % mtd->write_size;

should now be possible.

vcdiff_mtd_t requires write_size to be known at compile time to adjust buffer sizes. But at least an assert() would be nice to have.

Could we use mtd->work_area instead?

This would add the dependency mtd_write_page. Currently, I'm not using any of the write page features. Adding this module would add unnecessary code just to have the (mtd_write_page-internal) buffer at hand. It feels a little bit hacky to me.

I'd say having assert()s in place that make sure the buffer is aligned and large enough would be a good thing to have in place.

The default of 4 bytes seems to be a size that fits all mtd devices available in RIOT (IIRC) and isn't wasteful in my opinion.

For mtd_flashpage it can be larger than 4, same for SD cards (but those are usually used with a filesystem)

benpicco · 2022-08-26T17:16:56Z

Well, I'm happy to merge this as is and the do a follow-up PR.

MrKevinWeiss · 2022-08-30T09:27:02Z

It seems like native tests are failing now hitting an assert

jue89 · 2022-08-30T10:26:25Z

Oh thank you for your review!

jue89 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports labels Mar 11, 2022

jue89 requested a review from fjmolinas March 11, 2022 12:13

github-actions bot added Area: build system Area: Build system Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration labels Mar 11, 2022

aabadie reviewed Mar 11, 2022

View reviewed changes

pkg/tinyvcdiff/doc.txt Outdated Show resolved Hide resolved

aabadie reviewed Mar 11, 2022

View reviewed changes

pkg/tinyvcdiff/include/vcdiff_mtd.h Outdated Show resolved Hide resolved

jue89 force-pushed the feature/pkg-tiny-vcdiff branch from 3ca9440 to b7997a9 Compare March 26, 2022 18:05

github-actions bot added the Area: tests Area: tests and testing framework label Mar 26, 2022

jue89 force-pushed the feature/pkg-tiny-vcdiff branch from b7997a9 to 9ba4d2f Compare March 26, 2022 18:36

jue89 marked this pull request as ready for review March 26, 2022 18:37

jue89 requested review from leandrolanzieri, jia200x, cgundogan, miri64, smlng and MichelRottleuthner as code owners March 26, 2022 18:37

jue89 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 26, 2022

jue89 force-pushed the feature/pkg-tiny-vcdiff branch from 9ba4d2f to 28e9e12 Compare March 26, 2022 18:44

benpicco reviewed Mar 26, 2022

View reviewed changes

benpicco reviewed Mar 27, 2022

View reviewed changes

fjmolinas reviewed Mar 28, 2022

View reviewed changes

tests/pkg_tinyvcdiff/app.config.test Outdated Show resolved Hide resolved

fjmolinas reviewed Mar 28, 2022

View reviewed changes

tests/pkg_tinyvcdiff/fakemtd.h Outdated Show resolved Hide resolved

fjmolinas reviewed Mar 30, 2022

View reviewed changes

jue89 added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 31, 2022

jue89 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 5, 2022

jue89 added 4 commits May 17, 2022 13:40

pkg/tinyvcdiff: add package

5571d5c

pkg/tinyvcdiff: add MTD driver

e2e4dbe

pkg/tinyvcdiff: add VFS driver

368e770

pkg/tinyvcdiff: add test

0f1a1dc

jue89 force-pushed the feature/pkg-tiny-vcdiff branch from 1a612fb to 0f1a1dc Compare May 17, 2022 11:42

jue89 requested a review from MrKevinWeiss as a code owner May 17, 2022 11:42

benpicco reviewed May 23, 2022

View reviewed changes

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 26, 2022

benpicco approved these changes Aug 26, 2022

View reviewed changes

benpicco merged commit cb27a26 into RIOT-OS:master Aug 26, 2022

MrKevinWeiss mentioned this pull request Aug 30, 2022

tests/pkg_tinyvcdiff: Initialize mtd->write_size #18529

Merged

jue89 deleted the feature/pkg-tiny-vcdiff branch August 30, 2022 10:25

maribu added this to the Release 2022.10 milestone Oct 14, 2022

-    int rc = mtd_write_page_raw(mtd->dev, src, 0, offset, len);
+    uint32_t page = offset / mtd->dev->page_size;
+    uint32_t page_offset = (offset % mtd->dev->page_size);
+    int rc = mtd_write_page_raw(mtd->dev, src, page, page_offset, len);

	alignment_offset = mtd->offset % CONFIG_TINYVCDIFF_MTD_WRITE_SIZE;
	alignment_offset = mtd->offset % mtd->write_size;

Add package tiny-vcdiff #17797

Add package tiny-vcdiff #17797

Conversation

jue89 commented Mar 11, 2022

Contribution description

Testing procedure

Issues/PRs references

aabadie commented Mar 13, 2022

jue89 commented Mar 13, 2022

jue89 commented Mar 25, 2022

benpicco commented Mar 26, 2022

jue89 commented Mar 26, 2022 • edited Loading

benpicco left a comment • edited Loading

Choose a reason for hiding this comment

fjmolinas left a comment

Choose a reason for hiding this comment

jue89 commented Mar 28, 2022

jue89 commented Mar 30, 2022

Choose a reason for hiding this comment

jue89 Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

jue89 commented Mar 30, 2022

benpicco commented Mar 30, 2022

jue89 commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas Mar 30, 2022 • edited Loading

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

jue89 commented Mar 31, 2022

jue89 commented Apr 5, 2022

jue89 commented Apr 5, 2022

jue89 commented Apr 5, 2022

jue89 commented May 17, 2022

benpicco left a 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

jue89 Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco commented Aug 26, 2022

MrKevinWeiss commented Aug 30, 2022

jue89 commented Aug 30, 2022

jue89 commented Mar 26, 2022 •

edited

Loading

benpicco left a comment •

edited

Loading

jue89 Mar 30, 2022 •

edited

Loading

fjmolinas Mar 30, 2022 •

edited

Loading

jue89 Aug 30, 2022 •

edited

Loading