Skip to content

Conversation

@jordanhalase
Copy link
Contributor

@jordanhalase jordanhalase commented Jun 14, 2023

  • The code compiles without errors or warnings. Pre-existing warnings from unrelated code
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI (awaiting approval)

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

ESP ROM includes hard-coded, referentially transparent CRC functions via simple table lookups. Kilobytes of program space can be saved using them. A compile-time #[cfg] attribute is internally used to automatically link against the ROM function or to a fallback implementation if not available.

Currently, all chips implement all functions except ESP32-S2 which omits the *_be() variants. On this chip those functions will be compiled instead.

I tested the example code on a real ESP32-C3, and simulated the ESP32-S2 on Wokwi to test the fallback implementation. All examples compile.

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this LGTM! I have tested on hardware for each supported chip, all examples are working as expected.

Seems there's a conflict with CHANGELOG.md though, if you don't mind resolving that please I'm happy to merge!

@jordanhalase
Copy link
Contributor Author

Fixed merge conflict in CHANGELOG.md

@jessebraham jessebraham merged commit f22cd73 into esp-rs:main Jun 14, 2023
SergioGasquez pushed a commit to SergioGasquez/esp-hal that referenced this pull request Jun 15, 2023
* Add ESP ROM CRC and fallbacks to HAL

* Cargo fmt

* Add CRC examples

* Cargo fmt

* Cargo fmt and clippy (all)

* Update CHANGELOG.md
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.

2 participants