-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Conversation
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 |
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. |
That all sounds good. Have you seen a project which does this sort of heavy-reuse of CRC calculation? |
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.
030ccb9
to
78f81f3
Compare
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; |
There was a problem hiding this comment.
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
Going to come at this in a less invasive manner some time in the future. |
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.