[NXP] Add --wifipaf commission in chip-tool and apps of Linux platform #33977
+1,063
−12
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
Contributor
crlonxp
commented
Jun 18, 2024
•
crlonxp
commented
github-actions
bot
added
tools
examples
transport
setup payload
platform
linux
lib
controller
app
shell
labels
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
fessehaeve,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs and
jmartinez-silabs
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com>
* Change the name of option to [pair_mode]-[network] * Remove redundant compile flags * Move to start the Wi-Fi Manager in initialization stage * Unconditional the defintion * Add the cancel-publish / cancel-subscribe dbus interface * Fix bugs: - Redundant callback function registration - Remove the incorrect StackLock Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
- Unconditional functions in SetUpCodePairer
- Change the description of the comment
Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
* Unconditional the SetupCode verification Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
example:
- Linux DUT: sudo ./chip-all-clusters-app --wifi --wifipaf
- Controller: sudo ./chip-tool pairing code-wifi 1 ap_ssid ap_pwd MT:-24J0SGJ10KA0648G00
Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Add to end of the queue in OnDiscoveredDeviceOverWifiPAF() * Factor out the long expression into local Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
…allback function Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
|
PR #33977: Size comparison from 05e4c10 to c4e090e Increases above 0.2%:
Full report (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
|
PR #33977: Size comparison from 05e4c10 to ea75eeb Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…a_supplicant Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
bzbarsky-apple
requested changes
Jul 18, 2024
| @@ -90,6 +93,10 @@ using namespace chip::Encoding; | |||
| #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY | |||
| using namespace chip::Protocols::UserDirectedCommissioning; | |||
| #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY | |||
| #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF | |||
| static std::shared_ptr<DeviceCommissioner> DevCommPtr; | |||
There was a problem hiding this comment.
I don't understand how this is supposed to work. Per C++ spec:
If the object pointed to by ptr is already owned, the function generally results in undefined behavior.
and that's the scenario here, no? There are non-shared-ptr bits that will happily delete that object out from under you.
Maybe the intent was that you then reset the static shared_ptr when you get deleted? That would at least kind of work in that a properly created weak_ptr from that shared_ptr (which is also not happening here) would get notified when the shared_ptr gets reset, I think. But:
- That's not happening here.
- You can't use a static because multiple DeviceCommissioner instances might be trying to commission things at once, no?
| } | ||
| DevCommPtr.reset(this, custom_del); | ||
| mRendezvousParametersForDeviceDiscoveredOverWiFiPAF = params; | ||
| DeviceLayer::ConnectivityMgr().WiFiPAFConnect(params.GetSetupDiscriminator().value(), (void *) (&(DevCommPtr)), |
There was a problem hiding this comment.
OK, so this passes a pointer to the shared_ptr as the void*, right?
| #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF | ||
| void DeviceCommissioner::OnWiFiPAFSubscribeComplete(void * appState) | ||
| { | ||
| std::weak_ptr<DeviceCommissioner> * caller = (std::weak_ptr<DeviceCommissioner> *) (appState); |
There was a problem hiding this comment.
But this is casting the pointer we passed (to shared_ptr) to a weak_ptr?
I don't understand how this works.
| @@ -243,6 +253,39 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverSoftAP() | |||
| return CHIP_NO_ERROR; | |||
| } | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF | |||
| static std::shared_ptr<SetUpCodePairer> SetUpCodeParserPtr; | |||
There was a problem hiding this comment.
Again, I don't think this works. SetUpCodePairer lifetime is not managed via the shared ptr....
There was a problem hiding this comment.
Just to repeat for the Nth time: this needs tests. Not manual things that got run. Just fix the file in the tests directory under here to test these bits.
There was a problem hiding this comment.
I think you are saying to add the tests in setup_payload/tests/TestQRCode.cpp, right?
I add more tests in the last commit. Please review. Thank you
There was a problem hiding this comment.
Yes, that is much better, thank you, except for the missing checks.
* Add more tests in QRCode test Signed-off-by: Lo,Chin-Ran <chin-ran.lo@nxp.com>
Comment on lines
+53
to
+54
| bool mWiFiPAF = false; | ||
| const char * mWiFiPafExtCmds = nullptr; |
There was a problem hiding this comment.
Why the inconsistent capitalization?
|
|
||
| inPayload.rendezvousInformation.SetValue(RendezvousInformationFlags( | ||
| RendezvousInformationFlag::kWiFiPAF, RendezvousInformationFlag::kSoftAP, RendezvousInformationFlag::kOnNetwork)); | ||
|
|
There was a problem hiding this comment.
Shouldn't there be a EXPECT_TRUE(CheckWriteRead(inPayload)) here? Otherwise, the next line will just reset things and this is not tested...
|
|
||
| inPayload.rendezvousInformation.SetValue(RendezvousInformationFlags( | ||
| RendezvousInformationFlag::kWiFiPAF, RendezvousInformationFlag::kBLE, RendezvousInformationFlag::kOnNetwork)); | ||
|
|
There was a problem hiding this comment.
Again, should have EXPECT_TRUE(CheckWriteRead(inPayload)).
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.