Fix the dependency error and some changed requested by reviewer #34764
+739
−252
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
Aug 3, 2024
crlonxp
commented
- Change the default setting of WiFiPAF as enabled if running on Linux system
- Change to use C++ casting
- Move the paf cancel function from destructor to shutdown
|
Review changes with SemanticDiff. |
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harsha-rajendran,
hawk248,
hicklin and
jepenven-silabs
Signed-off-by: Lo,Chin-Ran <[email protected]>
Signed-off-by: Lo,Chin-Ran <[email protected]>
Signed-off-by: Lo,Chin-Ran <[email protected]>
Signed-off-by: Lo,Chin-Ran <[email protected]>
… the string Signed-off-by: Lo,Chin-Ran <[email protected]>
…a_supplicant Signed-off-by: Lo,Chin-Ran <[email protected]>
…ipaf is not selected Signed-off-by: Lo,Chin-Ran <[email protected]>
…o on ipv6 Signed-off-by: Lo,Chin-Ran <[email protected]>
|
PR #34764: Size comparison from 75d7e6b to a7a761a Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
* Fix the problems in NANCancelPublish by using the incorrect type and id Signed-off-by: Lo,Chin-Ran <[email protected]>
|
PR #34764: Size comparison from c18a6de to d617481 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Oct 30, 2024
| @@ -154,6 +154,10 @@ static_library("Linux") { | |||
| if (chip_enable_ble) { | |||
| public_deps += [ "dbus/bluez" ] | |||
| } | |||
|
|
|||
| if (chip_device_config_enable_wifipaf) { | |||
| public_deps += [ "${chip_root}/src/wifipaf:wifipaf" ] | |||
There was a problem hiding this comment.
Suggested change
| public_deps += [ "${chip_root}/src/wifipaf:wifipaf" ] | |
| public_deps += [ "${chip_root}/src/wifipaf" ] |
andy31415
reviewed
Oct 30, 2024
| # Include wifi-paf to commission the device or not | ||
| # This is a feature of Wi-Fi spec that it can be enabled if wifi is enabled | ||
| # and the supplicant can support. | ||
| chip_device_config_enable_wifipaf = chip_enable_wifi && false |
There was a problem hiding this comment.
&& false means false.
Maybe you can assert that if one is true the other one is as well instead.
andy31415
reviewed
Oct 30, 2024
| @@ -34,6 +34,10 @@ source_set("without-logging") { | |||
| "${chip_root}/src/transport", | |||
| ] | |||
|
|
|||
| if (chip_device_config_enable_wifipaf == true) { | |||
There was a problem hiding this comment.
Suggested change
| if (chip_device_config_enable_wifipaf == true) { | |
| if (chip_device_config_enable_wifipaf) { |
andy31415
reviewed
Oct 30, 2024
| @@ -289,12 +289,15 @@ struct ConnectivityManager::SEDIntervalsConfig | |||
| }; | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF | |||
| #define NAN_FREQ_LIST_ALL 0xff | |||
There was a problem hiding this comment.
use constexpr uint8_t kNaNFreqListAll (or whatever type or name is apropriate instead) to get type safety as well.
andy31415
reviewed
Oct 30, 2024
| * The max amount of time (in seconds) the chip controller will discovery Wi-Fi PAF | ||
| */ | ||
| #ifndef CHIP_DEVICE_CONFIG_WIFIPAF_DISCOVERY_TIMEOUT | ||
| #define CHIP_DEVICE_CONFIG_WIFIPAF_DISCOVERY_TIMEOUT (15 * 60) |
There was a problem hiding this comment.
please add unit of measurement suffix to all these constants. for seconds it should probably be _SEC
andy31415
reviewed
Oct 30, 2024
| @@ -369,6 +369,71 @@ class SampleTestEventTriggerHandler : public TestEventTriggerHandler | |||
| } | |||
| }; | |||
|
|
|||
| #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF | |||
| void int_array_add_unique(uint16_t ** res, uint16_t a) | |||
There was a problem hiding this comment.
Add a comment on what this method does and usage examples.
andy31415
reviewed
Oct 30, 2024
| void int_array_add_unique(uint16_t ** res, uint16_t a) | ||
| { | ||
| size_t reslen, max_size; | ||
| uint16_t * n; |
There was a problem hiding this comment.
move this closer to usage instead of at a top. Also give it a better name than n. What does it mean.
TLDR: I cannot tell at a glance what this method is doing and what its logic or variables mean. Please make this readable.
andy31415
reviewed
Oct 30, 2024
| *res = n; | ||
| } | ||
|
|
||
| static uint16_t WiFiPAFGet_FreqList(char * ArgStrn, uint16_t ** pfreq_list) |
There was a problem hiding this comment.
Please also improve readability on this method to:
- describe what it does and how (what do the arguments mean, how are they transformed, usage examples
- rename some variables (what is
lenfor example)? - is
freq_listneeded? we seem to use it just for a get and to later place it in pfreq_list. If needed this should be moved to decrease its scope. - Can we somehow make it so that ArgStrn can be const? generally this seems the intent and we use non-const so that strtok works.
andy31415
reviewed
Oct 30, 2024
| args.enable = LinuxDeviceOptions::GetInstance().mWiFiPAF; | ||
| args.ExtCmds = LinuxDeviceOptions::GetInstance().mWiFiPAFExtCmds; | ||
| args.enable = LinuxDeviceOptions::GetInstance().mWiFiPAF; | ||
| args.freq_list_len = WiFiPAFGet_FreqList((char *) LinuxDeviceOptions::GetInstance().mWiFiPAFExtCmds, &args.pfreq_list); |
There was a problem hiding this comment.
cast is suspicious. You should use C++-style casts, and then it is a const_cast and that is a code-smell. Fix up WiFiPAFGet_FreqList to accept const char instead.
andy31415
reviewed
Oct 30, 2024
| DeviceLayer::ConnectivityMgr().SetWiFiPAFAdvertisingEnabled(args); | ||
| if ((args.freq_list_len > 0) && (args.freq_list_len != NAN_FREQ_LIST_ALL)) |
There was a problem hiding this comment.
explain this code in a comment? why are we freeing all except the all constant?
Why can't this be automatic (like having the ARGS handle memory managment, not the caller)?
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.