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

Added ESP8266 support and documentation. #562

Closed
wants to merge 2 commits into from

Conversation

Spoozy
Copy link

@Spoozy Spoozy commented May 28, 2021

Q A
Bug fix? no
New feature? yes
Doc update? yes
BC breaks? no
Deprecations? no
Fixed tickets no

@Rotzbua
Copy link
Collaborator

Rotzbua commented May 29, 2021

Hi, thanks for your PR.
A full list of pin configs is available at: https://github.com/miguelbalboa/rfid#pin-layout
Updating every example with new pin layouts for a lot of board is a bit overdocumentation. Maybe it would be useful to have a link to the readme in every example.

@Spoozy
Copy link
Author

Spoozy commented May 29, 2021

No worries, although I could change the documentation, the most important thing is to have the constexpr uint8_t RST_PIN = D3; as the normal #define RST_PIN 9 doesn't work using the ESP8266 from what I have tested. I will implement the link to the README :)

@hochwasser
Copy link

What is the problem when the define got replaced with const or sonstexpr? Does this break old code?
If not, I would suggest replacing the defines.

@Rotzbua
Copy link
Collaborator

Rotzbua commented Aug 4, 2021

@hochwasser The problem is that all tutorials and instructions with this library uses #defines. The usage of constexpr would not break anything but confuses Arduino beginners.

I created a fork #522 which targets new concepts.

Rotzbua added a commit that referenced this pull request Aug 5, 2021
@Rotzbua
Copy link
Collaborator

Rotzbua commented Aug 5, 2021

@Spoozy the pr conflicts so I added the hint the link to the readme to every example. I think that is the initial intention of the pr. Thanks for contribution 👍

@Rotzbua Rotzbua closed this Aug 5, 2021
@hochwasser
Copy link

@Rotzbua I understand that constexpr is a relatively new. const could also be used in this situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants