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

Add on_tag_removed trigger to pn532 #1436

Merged
merged 6 commits into from May 17, 2021
Merged

Conversation

romerod
Copy link
Contributor

@romerod romerod commented Dec 28, 2020

Description:

Add on_release on_tag_removed trigger to pn532 to get a notification when tag is removed.

Related issue (if applicable): closes esphome/feature-requests#172

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#915

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@probot-esphome
Copy link

Hey there @OttoWinter, @jesserockz, mind taking a look at this pull request as its been labeled with an integration (pn532) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

I like it all apart from the naming of on_released maybe on_tag_removed or something...
I was considering adding this at some point. Thanks

@probot-esphome probot-esphome bot added the core label Jan 4, 2021
@romerod romerod changed the title Add on_release trigger to pn532 Add on_tag_removed trigger to pn532 Jan 4, 2021
@romerod
Copy link
Contributor Author

romerod commented Jan 4, 2021

I like it all apart from the naming of on_released maybe on_tag_removed or something...
I was considering adding this at some point. Thanks

Ok, the reasoning was to keep it similar to the binary_sensor, but I agree it's easier to understand like that

@jesserockz
Copy link
Member

Thanks, Just needs a docs PR then can be merged in.

@ssieb
Copy link
Member

ssieb commented Jan 15, 2021

You need to edit the description to include the docs PR.
It also looks you might need to rebase the changes to fix conflicts.

@romerod romerod force-pushed the pn532_onreleasetag branch 2 times, most recently from dc014fc to a8645fc Compare January 15, 2021 12:50
@romerod
Copy link
Contributor Author

romerod commented Jan 15, 2021

Changed the description, rebased and tested.

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

This will not exactly work as it cannot read_tag_ when there is no tag present at the time.
While it wont crash, there will be errors in the log when it tries to authenticate with the tag.
It will return a blank tag with the uid: new nfc::NfcTag(uid, nfc::MIFARE_CLASSIC)

@probot-esphome probot-esphome bot removed the small-pr label May 17, 2021
@jesserockz jesserockz merged commit d3e291b into esphome:dev May 17, 2021
@romerod
Copy link
Contributor Author

romerod commented May 18, 2021

Thanks @jesserockz for finishing the work.

This was referenced Jun 9, 2021
This was referenced Jun 16, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pn532 on_no_tag
4 participants