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.

Sign up for GitHub

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

Refactor CRC constructor method to remove warning #11478

Merged
merged 1 commit into from
Sep 25, 2019
Merged

Refactor CRC constructor method to remove warning #11478

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions drivers/MbedCRC.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,22 +514,19 @@ class MbedCRC {
MBED_STATIC_ASSERT(width <= 32, "Max 32-bit CRC supported");

#if DEVICE_CRC
if (POLY_32BIT_REV_ANSI == polynomial) {
Copy link

Choose a reason for hiding this comment

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

The code removed appears unnecessary given that the switch case below also
handles the case where the polynomial is POLY_32BIT_REV_ANSI

Code appears to be same but is used for different cases, HW vs Software CRC. See #if DEVICE_CRC and 4d7fdfc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not see how that the code removed changes anything as the execution path when the switch is reached for the case where polynomial is POLY_32BIT_REV_ANSI results in _crc_table = (uint32_t *)Table_CRC_32bit_Rev_ANSI; _mode = TABLE; just like in the code removed.

Why is the code removed necessary if the outcome is the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current code stops the HAL version being called for that case by returning early.

Not 100% clear on why this polynomial needs to be special-cased here, rather than dealt with in the HALs' hal_crc_is_supported, but it would make more sense just to make the entire #if DEVICE_CRC block have an extra if (polynomial != POLY_32BIT_REV_ANSI) test around it. This isn't a good use of early return.

Copy link
Collaborator Author

@hugueskamba hugueskamba Sep 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I got that it returns early but there is no comments on why it is necessary for POLY_32BIT_REV_ANSI or even if it is necessary.

If it is not necessary and the polynomial is dealt with by hal_crc_is_supported then we do not need the piece of code removed.

A quick look at hal_crc_is_supported suggests it is handled and therefore we do not need the code removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point is that the API has been hacked so that polynomial 0xEDB88320 is treated specially elsewhere in the software code. It doesn't actually run the same generic algorithm, so passing polynomial 0xEDB88320 to the HW API won't achieve the same effect unless they're also all modified to treat 0xEDB88320 as a magic number, which presumably they haven't been.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at it, I think the issue is that default constructor uses magic values of the polynomial template parameter to infer other parameters like reflection.

I think the previous PR tried to use a new magic polynomial value to generate the reversal, but broke what the polynomial parameter meant.

I think this reverse CRC would have been accurately described by

MbedCRC<POLY_32BIT_ANSI, 32> normal_crc(); // equivalent to (~0, ~0, true, true)
MbedCRC<POLY_32BIT_ANSI, 32> reverse_crc(~0, ~0, false, false);

without the previous PR.

If we want MbedCRC<POLY_32BIT_ANSI_REV, 32> to be able to invoke hardware, we'd need a few bits that did polynomial == POLY_32_BIT_ANSI_REV ? POLY_32_BIT : polynomial every time we went to pass it to the hardware.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just triple-checked what it's doing. POLY_32BIT_REV_ANSI actually computes exactly the same CRC as POLY_32BIT_ANSI... It's not even reversing the output order or anything. (The constructor shows reversal, but the special-case code ignores those parameters).

All it seems to be is expressing the polynomial itself in opposite-order form. Which doesn't change the result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@deepikabhavnani and @rajkan01 could you please comment on the above?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be looking at the whole thing further; there are general issues with the way the HW/SW selection is done, and image size. And I'll look at what to do with POLY_32BIT_REV_ANSI.

So this PR probably wants to just focus on eliminating the warning for now.

I think the safest change, not affecting functionality, is to wrap the entire hw check block up, leaving the switch statement to handle the REV case.

#if DEVICE_CRC
if (POLY_32BIT_REV_ANSI != polynomial) {
    crc_mbed_config_t conf;
    ...
    if (hal_crc_is_supported(&config)) {
        _mode = HARDWARE;
        return;
    }
}
#endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#11478 (comment)

_crc_table = (uint32_t *)Table_CRC_32bit_Rev_ANSI;
_mode = TABLE;
return;
}
crc_mbed_config_t config;
config.polynomial = polynomial;
config.width = width;
config.initial_xor = _initial_value;
config.final_xor = _final_xor;
config.reflect_in = _reflect_data;
config.reflect_out = _reflect_remainder;

if (hal_crc_is_supported(&config)) {
_mode = HARDWARE;
return;
if (POLY_32BIT_REV_ANSI != polynomial) {
crc_mbed_config_t config;
config.polynomial = polynomial;
config.width = width;
config.initial_xor = _initial_value;
config.final_xor = _final_xor;
config.reflect_in = _reflect_data;
config.reflect_out = _reflect_remainder;

if (hal_crc_is_supported(&config)) {
_mode = HARDWARE;
return;
}
}
#endif

Expand Down