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

Feature/pn7160 token prov ext lib #951

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cburandt
Copy link

@cburandt cburandt commented Nov 7, 2024

Add module for NXP PN7160 NFC chip as token provider.

Adds EVerest/linux_libnfc-nci (forked from NXPNFCLinux and patched to work well with EVerest).

# linux_libnfc-nci for RFID
libnfc-nci:
git: https://github.com/EVerest/linux_libnfc-nci.git
git_tag: 202411.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor observation, for consistency with the other repos that use a date based version this could be 2024.11.0 (since it's date based it's not a semantic version anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can also have a
cmake_condition: "EVEREST_DEPENDENCY_ENABLED_LIBNFC_NCI"

Copy link
Contributor

Choose a reason for hiding this comment

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

A find_package somewhere around here needs to be added as well:

Copy link
Author

Choose a reason for hiding this comment

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

Since I don't know in which situation it would be used: Is a BUILD.libnfc_nci.bazel required?

Copy link
Contributor

Choose a reason for hiding this comment

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

With this entry:

ev_define_dependency(
it will only be built when you build the new module and excluded if you exclude the module

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure here, but these files are copied from somewhere, and hence we should check and indicate their licensing.

Copy link
Author

Choose a reason for hiding this comment

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

They are Apache-2.0-licensed. I will add according headers to them.

this->callback = std::move(callback_);

registerTagCallback(&nfc_callbacks);
doEnableDiscovery(DEFAULT_NFA_TECH_MASK, 0x01, 0x0, 0x1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line would benefit from some more explicit documentation about the flags

@cburandt cburandt force-pushed the feature/PN7160TokenProv-extLib branch from 10f8250 to ab1bb79 Compare November 13, 2024 22:18
@cburandt cburandt force-pushed the feature/PN7160TokenProv-extLib branch from 530394c to c5d8fbf Compare November 14, 2024 17:37
Signed-off-by: Christoph Burandt <[email protected]>
- add missing find_package in top-level CMakeLists.txt
- fix Version no. in dependencies.yaml
- fix clang-format violations
- add license headers to libnfc-nci config files
- add explaining comment to nfc discovery ininitialization
- use cmake property to extract config install path from libnfc_nci

Signed-off-by: Christoph Burandt <[email protected]>
@cburandt cburandt force-pushed the feature/PN7160TokenProv-extLib branch from c5d8fbf to b143c62 Compare November 14, 2024 17:47
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.

3 participants