Skip to content

Update wifi.cpp - added faster reflect functions #896

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnudles
Copy link

@gnudles gnudles commented Apr 1, 2025

I know this is so insignificant because it only hapens on initialization. But maybe future implementation might require it. Feel free to reject it.

I know this is so insignificant because it only hapens on initialization. But maybe future implementation might require it
@rogerman
Copy link
Collaborator

rogerman commented Apr 1, 2025

Your replacement functions, reflect8() and reflect32() rearrange the bits on a per-nibble basis, but the original reflect() function rearranges the bits on a per-bit basis. Therefore, these are NOT equivalent functions at all. Nice try trying to optimize, but we've gotta reject this PR because your new functions cannot replace the old function. Optimization is hard.

@rogerman rogerman closed this Apr 1, 2025
@rogerman
Copy link
Collaborator

rogerman commented Apr 1, 2025

Okay, might of analyzed this PR too quickly -- the functions actually are equivalent. My bad.

Still, at the end of the day, we're just filling a LUT of 256 32-bit values only once at initialization, so this isn't even a performance issue. Meh. Because the functions are, in fact, equivalent, I'll reopen this PR if anyone actually cares.

@rogerman rogerman reopened this Apr 1, 2025
@gnudles
Copy link
Author

gnudles commented Apr 2, 2025

Yes I know this is not significant at all. I just thought, maybe one day, someone would need to use it in a different place in the code, so he better have a faster implementation...

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