[BL602] Fix sve2 problems #22582
Merged
[BL602] Fix sve2 problems #22582
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
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple and
lazarkov
pullapprove
bot
requested review from
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
|
PR #22582: Size comparison from 5caec5a to 41d39d5 Increases (3 builds for psoc6, qpg, telink)
Decreases (8 builds for bl602, esp32, k32w, nrfconnect, psoc6, telink)
Full report (37 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
@jczhang777 please add and link an issue for this. The template explicitly asks for a text of the form |
andy31415
reviewed
Sep 13, 2022
| { | ||
| if (!ip_addr_cmp(&netif->ip6_addr[i], IP6_ADDR_ANY)) | ||
| { | ||
| memcpy(&if_ip6[addr_count++], &netif->ip6_addr[i], sizeof(ip6_addr_t)); |
There was a problem hiding this comment.
how do you ensure that if_ip6 has sufficient space for addr_count ?
I think the size of input arrays should be passed in to methods. Currently nothing seems to guarantee that kMaxIPv6AddrCount >= LWIP_IPV6_NUM_ADDRESSES
There was a problem hiding this comment.
what guarantees that if_ip6 has at least min(kMaxIPv6AddrCount, LWIP_IPV6_NUM_ADDRESSES) elements?
The size of if_ip6 should be passed in to this method as an argument, not assumed.
andy31415
reviewed
Sep 13, 2022
andy31415
reviewed
Sep 13, 2022
| bl_efuse_read_mac(ifp->MacAddress); | ||
| ifp->hardwareAddress = ByteSpan(ifp->MacAddress, 6); | ||
|
|
||
| wifi_mgmr_sta_ip_get(&ip, &gw, &mask); |
There was a problem hiding this comment.
do these methods never fail? Like is there always an ip/gw/mask?
There was a problem hiding this comment.
What was the resolution here? This was marked resolved yet I see no comment of "this never fails" nor do I see code changes that would check for a return value from the wifi_mgmr_sta_ip_get
andy31415
reviewed
Sep 13, 2022
andy31415
reviewed
Sep 13, 2022
| memcpy(wifi_ssid, ssid, ssidLen); | ||
| memcpy(passwd, key, keyLen); | ||
| wifi_interface_t wifi_interface; | ||
| wifi_interface = wifi_mgmr_sta_enable(); | ||
| wifi_mgmr_sta_connect(wifi_interface, ssid, passwd, NULL, NULL, 0, 0); | ||
| wifi_mgmr_sta_connect(wifi_interface, wifi_ssid, passwd, NULL, NULL, 0, 0); |
There was a problem hiding this comment.
Do these methods return an error code we could check and log on failures?
There was a problem hiding this comment.
These were marked resolved ... what was the resolution? I see no deltas.
An answer like "this never fails" or implementation change would make sense.
andy31415
reviewed
Sep 13, 2022
|
accepted for 1.0: platform fixes |
andy31415
requested changes
Sep 15, 2022
andy31415
reviewed
Sep 15, 2022
| while (state != WIFI_STATE_IDLE) | ||
| { | ||
| wifi_mgmr_state_get(&state); | ||
| vTaskDelay(100); |
There was a problem hiding this comment.
what is the 100 here? why is it ok to use ticks which depend on tick rate instead of computing a standard time. At least a comment of why we do not care is appropriate.
andy31415
reviewed
Sep 15, 2022
| @@ -32,6 +32,8 @@ | |||
| #include <string> | |||
| #include <utils_log.h> | |||
|
|
|||
| #define WIFI_STA_DISCONNECT_DELAY (pdMS_TO_TICKS(200)) | |||
There was a problem hiding this comment.
in C++ use static constexpr ... for type safety instead of macros.
|
@jczhang777 if you mark comments resolved, please do a code change (which would make it obvious why resolved) or add a comment reply saying why no change is needed. I do not have API documentation for the SDK calls you use and you are the expert in the platform, so I am providing suggestions but if you say Want to make sure things were considered. |
|
I am force-merging this due to 1.0 branch: this PR makes things better, it just seems to be missing some error condition handling and some slight style issues and inconsistencies. These can be made in a followup by the platform owner. As this is not core SDK, I will not block PR on them. |
|
@jczhang777 - followup for more fixes would be appreciated/recommented. |
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.
Issue Being Resolved
fixes #22616
Change overview