Refactor CRC constructor method to remove warning

[hugueskamba]

Description

The warning removed is
[Warning] MbedCRC.h@524,0: [Pe111]: statement is unreachable
It can be observed when building mbed-os-example-blinky with the IAR toolchain.

The removed code is handled by the switch statement below where it was.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@deepikabhavnani

Release Notes

[ciarmcom]

@hugueskamba, thank you for your changes.
@deepikabhavnani @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[deepikabhavnani]

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

[hugueskamba]

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?

[kjbracey]

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.

[hugueskamba]

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.

[kjbracey]

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.

[kjbracey]

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.

[kjbracey]

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.

[hugueskamba]

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

[kjbracey]

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

[hugueskamba]

#11478 (comment)

[hugueskamba]

This force-push wraps up the entire hardware check block, leaving the switch statement to handle the POLY_32BIT_REV_ANSI case.

[0xc0170]

Please fix astyle failures

[hugueskamba]

Please fix astyle failures

This force-push fixes the code style error.

[0xc0170]

CI restarted

[mbed-ci]

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts