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

Couple of small updates to rfid.esp and add more comments #621

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

mcnewton
Copy link

  • No need to test (uid.length() == 0) multiple times in genericRead, put all the alternative hardware in one 'if' block.
  • Quote the debug 'Username' to make it easier to read
  • Add more comments to help people like me understand what's happening :)

@matjack1
Copy link
Collaborator

@mcnewton looks like this is not changing anything right? It's fine if you want to add comments, just make it clear if you want to change code while you add comments so that it's easier to review.

If you want to get this merged, can you please resolve the conflict? Thanks!

@mcnewton
Copy link
Author

Hi @matjack1 - it's mainly comments, yes - but there's a small tidy lines 175-187 where it pulls the && uid.length() == 0 off three if statements and just wraps them all in if (uid.length() == 0). With optimisations it probably compiles down to the same thing, but makes it a bit easier to read. I did mention it in the PR comment.

I'll rebase and update the PR - I made the PRs so they could be pulled in independently, but figured that if you tried to pull them all in they might clash slightly :) Thanks for pulling the rest of them!

helps us mere mortals can understand what's happening :)

also quote username in debug output for clarity
@matjack1
Copy link
Collaborator

matjack1 commented Apr 9, 2024

Cool, thanks :)

@matjack1 matjack1 merged commit d7be1a4 into esprfid:dev Apr 9, 2024
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.

None yet

2 participants