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

Fix inconsistency between mbed crc and psoc6 crc implementations. #12324

Merged

Conversation

dustin-crossman
Copy link
Contributor

Summary of changes

There is an incongruity between what MBeds CRC spec expects and what PSoC6 hardware actually performs when it comes to the final XOR and remainder reversal: MBed expects the final remainder to be reversed then XORed while PSoC6s CRC, however, performs the final XOR then reverses the resulting value.
This fixes this by reversing the final XOR value before feeding it to the PSoC6 hardware. This works because

Rev(A) XOR B == Rev( A XOR Rev(B) )

Impact of changes

Migration actions required

Documentation

None


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

CY8CKIT_062_WIFI_BT-GCC_ARM_GT.txt
CY8CPROTO_062_4343W-GCC_ARM_GT.txt


Reviewers


@ciarmcom ciarmcom requested review from maclobdell and a team January 29, 2020 00:00
@ciarmcom
Copy link
Member

@dustin-crossman, thank you for your changes.
@maclobdell @ARMmbed/mbed-os-maintainers please review.

// significant than width are dropped.
static uint32_t reverse(uint8_t width, uint32_t in)
{
MBED_ASSERT(width <= 32);
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I wouldn't bother with the assert - this is internal, and you've already cleared the HAL_CRC_IS_SUPPORTED check, and there are a squillion uint32_ts around here. 32-bit or less is kind of assumed.

Actually though, having this after HAL_CRC_IS_SUPPORTED means the compiler should hopefully skip putting in the assert anyway. So maybe I shouldn't be worrying about code size.

Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

Has this newly shown up due to the extra Greentea tests? Good. I did recently add some to try to catch this possible error - it was clear the reversal of XOR and init values wasn't being exercised fully.

@mergify mergify bot added needs: CI and removed needs: review labels Jan 29, 2020
@dustin-crossman
Copy link
Contributor Author

@kjbracey-arm Yep! The crc test cases you added was exactly what was failing for us.

@kjbracey
Copy link
Contributor

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 30, 2020

Test run: SUCCESS

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

@kjbracey kjbracey merged commit c8d7778 into ARMmbed:master Jan 30, 2020
@mergify
Copy link

mergify bot commented Jan 30, 2020

This PR does not contain release version label after merging.

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 and removed Release review required labels Feb 4, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 4, 2020

I've fixed the version: Set to 6.0.0-alpha-2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devices: cypress release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants