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 NFCPy support #2190

Merged

Conversation

powertomato
Copy link

This Pull Request adds support for using NFCPy supported NFC readers

@s-martin s-martin added enhancement future3 Relates to future3 development labels Jan 1, 2024
Copy link
Collaborator

@s-martin s-martin 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 your addition! 🙏🎉
Nice work! 👍

I made a few comments to ensure that this new rfid reader is integrated properly.

@Groovylein
Copy link
Collaborator

Groovylein commented Jan 1, 2024

Except for the points that @s-martin found, I like the changes you did there!

Which device did you test on?

@powertomato
Copy link
Author

Except for the points that @s-martin found, I like the changes you did there!

Which device did you test on?

I tested on a Rock64 with dietpi. As for the NFC reader it's an ACR122U

@s-martin
Copy link
Collaborator

s-martin commented Jan 7, 2024

Thanks for the updates!

@pabera
Copy link
Collaborator

pabera commented Jan 16, 2024

I am unable to check this since I don't have a matching device available. Have you tested your set up properly? Was the installation process smooth. Since you followed the guidance, I'd suggest to merge this. Maybe you could resolve the above comments and send this for code review again. I have added this to the next release #2211.

@pabera pabera mentioned this pull request Jan 16, 2024
13 tasks
@pabera pabera added this to the v3.5 milestone Jan 16, 2024
@powertomato
Copy link
Author

I am unable to check this since I don't have a matching device available. Have you tested your set up properly? Was the installation process smooth. Since you followed the guidance, I'd suggest to merge this. Maybe you could resolve the above comments and send this for code review again. I have added this to the next release #2211.

This is tested, but I wanted to resolve the comments first and do a complete clean install again to confirm. I'm sick since a week, and will probably not come around that before the end of the week.

@pabera pabera modified the milestones: v3.5, v3.6 Jan 18, 2024
@powertomato
Copy link
Author

powertomato commented Jan 29, 2024

I have improved the installation script and customization by scanning for compatible devices and letting the user choose which one they want to use. It is very similar to the generic_usb RFID module.

The customization code will also generate an explicit device path, that will also allow to use multiple NFCpy compatible devices at the same time.

Edit: I just came accross git update-index --chmod=+x, now it works, the text below is outdated.

The code is tested on a fresh installation and works beside one small quirk, which I seemingly cannot fix.

The file src/jukebox/components/rfid/hardware/generic_nfcpy/setup.inc.sh is missing execution rights when checked out. The file is executable on my machine, but I suspect it has something to do, that I committed it on a windows machine...

When I add the execution rights and rerun run_register_rfid_reader.py everything works flawlessly.

@coveralls
Copy link

coveralls commented Jan 29, 2024

Pull Request Test Coverage Report for Build 7768616291

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 38.384%

Totals Coverage Status
Change from base Build 7646903644: 0.0%
Covered Lines: 418
Relevant Lines: 1089

💛 - Coveralls

@s-martin s-martin modified the milestones: v3.6, v3.5 Jan 30, 2024
@s-martin
Copy link
Collaborator

@powertomato it looks like you are finished and as you said you tested it we approved it.

If you have no more changes planned, we would merge it.

@s-martin
Copy link
Collaborator

s-martin commented Feb 2, 2024

Just stumbled over this code line:

included_readers = [NO_RFID_READER, 'generic_usb', 'rdm6300_serial', 'rc522_spi', 'pn532_i2c_py532', 'fake_reader_gui']

@pabera, @AlvinSchiller do we need to add nfcpy there?

@AlvinSchiller
Copy link
Collaborator

Just stumbled over this code line:

included_readers = [NO_RFID_READER, 'generic_usb', 'rdm6300_serial', 'rc522_spi', 'pn532_i2c_py532', 'fake_reader_gui']

@pabera, @AlvinSchiller do we need to add nfcpy there?

No, its not necessary. The nfcpy will be picked up as additional reader and is appended.
We could add it to get it in a different order.

Copy link
Collaborator

@AlvinSchiller AlvinSchiller left a comment

Choose a reason for hiding this comment

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

Nice work. Thanks again!

@s-martin s-martin merged commit e1b46b6 into MiczFlor:future3/develop Feb 4, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement future3 Relates to future3 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants