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.

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

AP_Math: split CRCs from checksums #27355

Closed
wants to merge 14 commits into from

Conversation

tpwrules
Copy link
Contributor

All CRCs are checksums but not all checksums are CRCs.

Paves the way for deduplication and unification of CRC implementations, including interface and standard of implementation.

No functional change yet. Hopefully this cleanup is appreciated.

@tridge
Copy link
Contributor

tridge commented Jun 22, 2024

No functional change yet. Hopefully this cleanup is appreciated.

I'm a bit sceptical of the benefit of this change. Can you point at some examples of the ultimate benefit? I'm well aware of the distinction between crc and checksum, but for our purposes that is an implementation detail and really only of academic interest

@tpwrules
Copy link
Contributor Author

tpwrules commented Jun 22, 2024

The benefit is that by separating out the CRCs, we can switch to a common implementation for them. There is a known taxonomy (see section 15 here) and there are various code generators and template libraries available given those parameters.

This lets us identify and remove duplicates (e.g. crc_xmodem, crc16_ccitt) and intelligently switch between bit-wise or byte-wise computation with the space/speed tradeoff. We can also remove all the warts of the different functions (argument order, width of input size) and reduce flash usage.

@peterbarker
Copy link
Contributor

The benefit is that by separating out the CRCs, we can switch to a common implementation for them. There is a known taxonomy (see section 15 here) and there are various code generators and template libraries available given those parameters.

This lets us identify and remove duplicates (e.g. crc_xmodem, crc16_ccitt) and intelligently switch between bit-wise or byte-wise computation with the space/speed tradeoff. We can also remove all the warts of the different functions (argument order, width of input size) and reduce flash usage.

That all sounds good. Have you seen a project which does this sort of heavy-reuse of CRC calculation?

@tpwrules tpwrules force-pushed the pr/crc-disambiguate branch from 030ccb9 to 78f81f3 Compare July 5, 2024 19:56
@tpwrules
Copy link
Contributor Author

tpwrules commented Jul 5, 2024

That all sounds good. Have you seen a project which does this sort of heavy-reuse of CRC calculation?

No, I'm not really aware of anything else that needs to compute so many varied CRCs.

I looked into this in a bit more detail and while I still think this cleanup is valuable and worthwhile, the followup work will probably desire an extreme amount of testing and I can't commit to it right now. It's also not likely to save much flash in particular. It's more targeted towards reducing the code amount and simplifying it.

@@ -286,7 +286,7 @@ void AP_RCProtocol_FPort2::_process_byte(uint32_t timestamp_us, uint8_t b)
// check checksum byte
bool AP_RCProtocol_FPort2::check_checksum(void)
{
return crc_sum8_with_carry(&byte_input.buf[1], byte_input.control_len-1) == 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Decided this was a bad rename

@tpwrules
Copy link
Contributor Author

Going to come at this in a less invasive manner some time in the future.

@tpwrules tpwrules closed this Jul 13, 2024
@tpwrules tpwrules deleted the pr/crc-disambiguate branch July 13, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants