Skip to content

Conversation

@crasbe
Copy link
Contributor

@crasbe crasbe commented Jun 6, 2025

Contribution description

Currently only an MSB-first CRC8 function is implemented with a note telling the caller to reflect the data themselves. This PR adds an LSB-first CRC8 function, so the data doesn't have to be reflected manually. With more data, this can take a significant amount of time.

Testing procedure

Run the unittests and CI.

Issues/PRs references

Required for #19848.

@crasbe crasbe requested a review from Enoch247 June 6, 2025 20:03
@crasbe crasbe added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Jun 6, 2025
@crasbe crasbe requested a review from miri64 as a code owner June 6, 2025 20:03
@crasbe crasbe added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 6, 2025
@crasbe crasbe requested a review from jia200x as a code owner June 6, 2025 20:03
@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: sys Area: System labels Jun 6, 2025
@riot-ci
Copy link

riot-ci commented Jun 6, 2025

Murdock results

✔️ PASSED

a8ccd54 tests/unittests: Add unittest for CRC8-LSB

Success Failures Total Runtime
10379 0 10379 12m:47s

Artifacts

@crasbe
Copy link
Contributor Author

crasbe commented Jun 6, 2025

The latest fixup adds a dedicated test with the Onewire parameters, because why not.

@maribu
Copy link
Member

maribu commented Jun 7, 2025

We should probably consider renaming the _lsb() to _reflected(), but I like not mixing that concern with adding the implementation and keeping the naming convention.

But also, the reflected version does operate least significant bit first, so the _lsb() naming does have some merit.

@crasbe
Copy link
Contributor Author

crasbe commented Jun 7, 2025

We should probably consider renaming the _lsb() to _reflected(), but I like not mixing that concern with adding the implementation and keeping the naming convention.

But also, the reflected version does operate least significant bit first, so the _lsb() naming does have some merit.

There is some value in having that discussion before merging, because afterwards, once a Release has happened, we would have to deprecate the old function name if we decided to change it.

However I'm a bit torn, because the new function does not reflect the data, it just works LSB first (which is equivalent to reflecting the data beforehand). But on the other side, Maxim doesn't really call that algorithm "reflected".
Internally everything just works LSB first and therefore also the CRC works LSB first.

@maribu
Copy link
Member

maribu commented Jun 7, 2025

There is some value in having that discussion before merging

Could it be that you are not yet aware of how much energy the RIOT community has for bike-shedding and that discussions about naming easily take many months? 🤣

If we were to change the name, we would have to add a backward compatible alias for the CRC16 variant anyway. I think it would not make much difference to do the same for CRC8.

I'm personally fine with either name, especially as the doc here states both terminologies, so that people will find the function they are looking for easily.

@crasbe
Copy link
Contributor Author

crasbe commented Jun 7, 2025

Well then let's get this merged so I can get back to poking @Enoch247 :)

@crasbe crasbe added this pull request to the merge queue Jun 7, 2025
@crasbe crasbe changed the title sys/checksum: Add reflection CRC8 Function sys/checksum: Add LSB-first (reflected) CRC8 Function Jun 7, 2025
Merged via the queue into RIOT-OS:master with commit 42a3824 Jun 7, 2025
25 checks passed
@crasbe crasbe deleted the pr/crc8_refl branch June 7, 2025 16:27
@Teufelchen1 Teufelchen1 added this to the Release 2025.07 milestone Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation 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

Development

Successfully merging this pull request may close these issues.

4 participants