Renaming prior to the delivery of the work about "NFC Commissioning". #36801
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In current code, "NFC Commissioning" was used for Onboarding Payload data put into an NFC Tag. This naming will create confusion with the new "NFC Commissioning" feature where NFC is used as the commissioning channel. We propose to use the folllowing names: - "NFC onboarding payload" when an NFC Tag is used to store the onboarding payload data (as an alternate solution to QRCode) - "NFC commissioning" when an NFC Tag is used to perform the first commissioning phase (as an alternate solution to BLE) Defines, variables and files have been renamed to reflect this. The following renaming have been done: - CHIP_DEVICE_CONFIG_ENABLE_NFC -> CHIP_DEVICE_CONFIG_ENABLE_NFC_ONBOARDING_PAYLOAD - CONFIG_CHIP_NFC_COMMISSIONING -> CONFIG_CHIP_NFC_ONBOARDING_PAYLOAD - chip_enable_nfc -> chip_enable_nfc_onboarding_payload - NFCManager -> NFCOnboardingPayloadManager - NFCManagerImpl -> NFCOnboardingPayloadManagerImpl - NFCMgr -> NFCOnboardingMgr - NFCMgrImpl -> NFCOnboardingPayloadMgrImpl The following files have been renamed for the platforms Zephyr, nrfconnect, nxp/k32w0 and telink: NFCManagerImpl.cpp -> NFCOnboardingPayloadManagerImpl.cpp NFCManagerImpl.h -> NFCOnboardingPayloadManagerImpl.h NFCManager.h -> NFCOnboardingPayloadManager.h
|
Review changes with |
github-actions
bot
added
documentation
|
PR #36801: Size comparison from 425e5bd to 196bf00 Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
What about |
Damian-Nordic
approved these changes
Dec 11, 2024
@Damian-Nordic Thank you! I have pushed a commit with the changes that you suggested. |
@bauerschwan I have missed this file. I don't have much a preference. Do you prefer that we keep it like that or rename it? As indicated on Slack, the code managing "Onboarding Payload" data is for Commissionee side whereas the code adding NFC Commissioning (and using NFC as a new Transport Type) is on Commissioner side. So the classes are completely distinct. In a next PR, you will see the new classes used for NFC Commissioning. |
|
PR #36801: Size comparison from 425e5bd to 93d2adf Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
@Damian-Nordic I see that the Zephyr files are used by Nordic:
Who should give a green light for the modification of those files? |
I think only Nordic uses these files (we should actually move them to nrfconnect dir) so it's enough for you to get +1 from me. |
|
@bauerschwan I see that in the following PR (adding NFC as a new Transport file), I do have some new NFC.cpp and NFC.h files coming in src/transport/raw. IMO this is not an issue. For BLE this is exactly the same. You've got:
|
bauerschwan
approved these changes
Dec 12, 2024
s07641069
approved these changes
Dec 13, 2024
Thank you very much Alex! I have pushed your changes. |
andy31415
previously requested changes
Dec 16, 2024
There was a problem hiding this comment.
@OlivierGre please add a Testing section to your PR to describe how this PR was tested or is to be tested. If manual please describe in detail how it was tested and also why automated testing is impossible.
Also make sure you clean it up. There is a Make sure you delete these instructions (to prove you have read them). in there still ....
Also requires restyle fixes.
@andy31415 Hi, For the tests, I'm not able to test it because this code is for the Commissionee side for platforms that I don't have (Zephyr, nrfconnect, nxp/k32w0 and telink). I have requested a review and an approval from those companies:
I wanted to do the change about the restyler but, when I look at the details provided, I can see that it ended with an error; Thanks |
|
@andy31415 Also, I don't understand why many CI checks indicate "cancelled after 1m" (ex: " |
So how do we know that this code is good? There has to be some way to test it? |
|
@OlivierGre please add a Then reviewers can make an informed decision about the code. Maybe details about how testing will follow eventualy, because code that will never be tested/executed sounds wrong. |
We have a fast-cancel in case some fast required checks fail, so we free up CI resources. In this case |
|
@OlivierGre for the restyle, the restyler task contains the command needed to restyle, generally ready for copy & paste (I hope it is generally visible)
|
|
PR #36801: Size comparison from 1b4c56c to 009b64a Full report (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
@Damian-Nordic For the moment I don't find the clue. Would you know where CHIP_NFC_COMMISSIONING could be defined? Thanks |
|
@OlivierGre you seem to have missed |
Oh, thank you Damian. I can see that I have missed this file because my editor did not include the files without extension. |
|
PR #36801: Size comparison from 1b4c56c to 5af2efa Full report (81 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36801: Size comparison from c6a68c6 to 4a41f22 Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In current code, "NFC Commissioning" was used for Onboarding Payload data put into an NFC Tag. This naming will create confusion with the new "NFC Commissioning" feature where NFC is used as the commissioning channel.
We propose to use the folllowing names:
Defines, variables and files have been renamed to reflect this. The following renaming have been done:
The following files have been renamed for the platforms Zephyr, nrfconnect, nxp/k32w0 and telink: NFCManagerImpl.cpp -> NFCOnboardingPayloadManagerImpl.cpp NFCManagerImpl.h -> NFCOnboardingPayloadManagerImpl.h NFCManager.h -> NFCOnboardingPayloadManager.h
Testing
The current renamings are for the platforms Zephyr, nrfconnect, nxp/k32w0 and telink.
I'm not able to test those platforms so I only relied on GitHub's automatic checks to test that it is building proprely on all those platforms.
In parallel, I have asked Nordic (Zephyr is also their code), NXP and Telink to review those changes and give a green light.
At that time, NXP feedback is missing.