Skip to content

Fletcher32: Add incremental API#19981

Merged
bors[bot] merged 2 commits intoRIOT-OS:masterfrom
bergzand:pr/fletcher32/incremental
Oct 20, 2023
Merged

Fletcher32: Add incremental API#19981
bors[bot] merged 2 commits intoRIOT-OS:masterfrom
bergzand:pr/fletcher32/incremental

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Oct 17, 2023

Contribution description

This PR extends the current fletcher32 checksum with an incremental API mode. This way the bytes to be checksummed can be supplied via multiple successive calls and do not have to be provided in a single consecutive buffer.

I've also rephrased the warning with the original function a bit as that function uses an unaligned_get_u16 to access the data. The data thus does not require alignment, but the length does need to be supplied as number of 16 bit words.

Testing procedure

The test has been extended

Issues/PRs references

None

@bergzand bergzand added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Oct 17, 2023
@bergzand bergzand requested a review from miri64 as a code owner October 17, 2023 20:02
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: sys Area: System labels Oct 17, 2023
@bergzand bergzand force-pushed the pr/fletcher32/incremental branch from 77ec405 to 6c89129 Compare October 17, 2023 20:10
Teufelchen1
Teufelchen1 requested changes Oct 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

Only minor remarks by me and one static-test! 💪

Teufelchen1
Teufelchen1 approved these changes Oct 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

🦐

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 18, 2023
@riot-ci
Copy link
Copy Markdown

riot-ci commented Oct 18, 2023 •
edited
Loading

Murdock results

✔️ PASSED

a00702e Fletcher32: Rephrase note on 16 bit word operations

Success Failures Total Runtime
7937 0 7937 16m:00s

Artifacts

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 18, 2023

... waiting for users passing in byte array length as "words" ...

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2023

... waiting for users passing in byte array length as "words" ...

The whole api of this module has in my opinion always been a huge footgun. Any suggestions here?

@bergzand bergzand force-pushed the pr/fletcher32/incremental branch 2 times, most recently from ba5314a to 71da3f4 Compare October 18, 2023 10:57
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 18, 2023

The whole api of this module has in my opinion always been a huge footgun. Any suggestions here?

Agreed. Now that there's a struct with state, buffer single bytes there if necessary?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 18, 2023

... but of course I don't want to hold up the already ACK'ed PR.

benpicco
benpicco reviewed Oct 18, 2023
View reviewed changes
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2023

Agreed. Now that there's a struct with state, buffer single bytes there if necessary?

  1. Follow up
  2. How would you handle the case where the full length of the (incrementally passed) data is odd?

benpicco
benpicco reviewed Oct 18, 2023
View reviewed changes
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 18, 2023

Feel free to squash

@bergzand bergzand force-pushed the pr/fletcher32/incremental branch from 1257420 to a00702e Compare October 18, 2023 11:22
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2023

Squashed!

@bergzand bergzand mentioned this pull request Oct 19, 2023
Open
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 19, 2023

bors merge

@bors
Copy link
Copy Markdown
Contributor

bors bot commented Oct 20, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 03d3874 into RIOT-OS:master Oct 20, 2023
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco left review comments

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

Assignees

No one assigned

Labels

Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2024.01

Development

Successfully merging this pull request may close these issues.

6 participants

@bergzand @riot-ci @kaspar030 @benpicco @Teufelchen1 @MrKevinWeiss