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

Refactor NFC directory structure #13414

Merged
merged 8 commits into from
Aug 13, 2020
Merged

Conversation

LDong-Arm
Copy link
Contributor

@LDong-Arm LDong-Arm commented Aug 11, 2020

Summary of changes

New NFC directory structure:

connectivity
├── nfc
│   ├── mbed_lib.json
│   ├── include
│   │   └── nfc             // public NFC APIs
│   │       ├── ndef/       // public NDEF APIs
│   │       ├── NFC.h
│   │       └── [...].h
│   ├── source
│   │   ├── ndef/
│   │   ├── NFCController.cpp
│   │   └── [...].cpp
│   ├── libraries
│   │   ├── acore   
│   │   └── stack   
│   └── tests
│       └── TESTS           // Greentea tests
│
└──drivers
    ├── [...]
    └── nfc
       └── PN512            // PN512 driver (incl. transceiver), separated from the main NFC source in this PR

Note: Unlike previously discussed, I have put acore and stack in libraries because they are both self-contained "sub" libraries, used by the main NFC code and target drivers. Keeping their existing internal structures makes more sense than trying to split them into include & source which is rather intrusive.

NOTE: This PR does NOT cover NFC in TEST_APPS - it hasn't been updated or used for a while, maybe the Connectivity team should take it over and decide what to do?

Impact of changes

None

Migration actions required

None

Documentation

None


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Manual testing

  • mbed test -t ARM -m NUCLEO_F401RE --compile -n connectivity-nfc-tests-tests-nfc-eeprom
  • mbed-os-example-nfc compiled for NUCLEO_F401RE (PN512 NFC chip) and DISCO_L475VG_IOT01A (M24SR NFC chip).

Reviewers

@donatieng @paul-szczepanek-arm @ARMmbed/mbed-os-connectivity @ARMmbed/mbed-os-core


@LDong-Arm
Copy link
Contributor Author

LDong-Arm commented Aug 11, 2020

@donatieng I didn't break down acore and stack because they are self-contained and used by both Mbed OS NFC and target drivers - refactoring into them might mess thing up in my opinion. Is it okay?

@ARMmbed/mbed-os-connectivity Any idea about NFC test app? Has anyone run it recently? I haven't been able to run it yet.

Copy link
Contributor

@rajkan01 rajkan01 left a comment

Choose a reason for hiding this comment

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

Current changes LGTM. Still, the connectivity team has to approve as some changes on the proposed directory structure in a confluence page

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 11, 2020
@ciarmcom
Copy link
Member

@LDong-Arm, thank you for your changes.
@paul-szczepanek-arm @donatieng @ARMmbed/mbed-os-maintainers please review.

@LDong-Arm
Copy link
Contributor Author

@donatieng has had a look at the revised directory structure (in the PR description) and agrees with the idea.

Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

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

I'll help @paul-szczepanek-arm get time for a coffee. This LGTM. Thanks @LDong-Arm!

@mergify mergify bot added needs: CI and removed needs: review labels Aug 13, 2020
@LDong-Arm
Copy link
Contributor Author

@rajkan01 @donatieng Thanks for the reviews!
@0xc0170 This is ready for CI, and the risk of causing rebase-related conflicts is low because we didn't touch non-NFC stuff (e.g. astyle, CMake, etc.)

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 13, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Aug 13, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 19, 2020
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.

7 participants