Skip to content

Adding SiWx917 Network Stack Application APIs#89716

Closed
ragurram26 wants to merge 17 commits intozephyrproject-rtos:mainfrom
ragurram26:Embedded_TA_stack
Closed

Adding SiWx917 Network Stack Application APIs#89716
ragurram26 wants to merge 17 commits intozephyrproject-rtos:mainfrom
ragurram26:Embedded_TA_stack

Conversation

@ragurram26
Copy link
Contributor

@ragurram26 ragurram26 commented May 9, 2025

To support the SiWx917 Network Stack Application
protocol offloading, we need to add the Silicon Labs APIs,
so adding the corresponding source files and include files

@github-actions
Copy link

github-actions bot commented May 9, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_silabs zephyrproject-rtos/hal_silabs@a0095a7 (main) zephyrproject-rtos/hal_silabs#100 zephyrproject-rtos/hal_silabs#100/files

DNM label due to: 1 project with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@github-actions github-actions bot added manifest manifest-hal_silabs DNM (manifest) This PR should not be merged (controlled by action-manifest) labels May 9, 2025
@ragurram26 ragurram26 changed the title Adding SL Embedded TA APIs Adding Silabs Embedded TA APIs May 9, 2025
@jhedberg
Copy link
Member

jhedberg commented May 9, 2025

@ragurram26 please provide a more detailed PR & commit message description, explaining the background to this, why this is needed, etc. Also most people in a Zephyr context won't know what "TA" is, so you need to explain that as well. If you're adding new APIs (like the title implies) I'd also expect to see some sample code to demonstrate their usage and provide code coverage for CI.

@jhedberg
Copy link
Member

jhedberg commented May 9, 2025

One other thing: when you have to enumerate multiple independent changes in a single commit message (like you're doing now), it's a clear indication that you should split this up into multiple commits.

net_if_dormant_off(sidev->iface);
}

#ifndef CONFIG_WIFI_SILABS_SIWX91X_NET_STACK_OFFLOAD
Copy link
Member

Choose a reason for hiding this comment

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

Could this use if (!IS_ENABLED(CONFIG_WIFI_SILABS_SIWX91X_NET_STACK_OFFLOAD)) {?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@ragurram26 ragurram26 changed the title Adding Silabs Embedded TA APIs Adding Silabs Embedded SiWx917 APIs May 9, 2025
@ragurram26 ragurram26 force-pushed the Embedded_TA_stack branch from 8d5e691 to 8b9c083 Compare May 9, 2025 10:01
@ragurram26
Copy link
Contributor Author

ragurram26 commented May 9, 2025

@jhedberg I have updated the description .

I'd also expect to see some sample code to demonstrate their usage and provide code coverage for CI.
Should we include the application in this pull request, or is there an alternative method to ensure code coverage for continuous integration (CI)?

@ragurram26 ragurram26 force-pushed the Embedded_TA_stack branch from 8b9c083 to 6966645 Compare May 9, 2025 10:10
@rettichschnidi rettichschnidi removed their request for review May 12, 2025 07:40
@ragurram26 ragurram26 force-pushed the Embedded_TA_stack branch 9 times, most recently from b4e40e2 to 0c25983 Compare May 29, 2025 09:58
@ragurram26
Copy link
Contributor Author

I believe you need to provide examples and to prove the usefulness of these features before to go further.

I have added sample example in this path :samples/boards/siwx91x

Copy link
Contributor

@jerome-pouiller jerome-pouiller left a comment

Choose a reason for hiding this comment

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

This PR is big enough. I would suggest to focus on DNS and ping and see this can be accepted.

SiWx91x series.

config WIFI_SILABS_SIWX91X_SSL_MEMORY_CLOUD
bool "Offloaded implementation for the SSL/TLS cloud connections"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the issue is specific to cloud servers?

}
#ifdef CONFIG_WIFI_SILABS_SIWX91X_SOCKETS
boot_config->tcp_ip_feature_bit_map |=
SL_SI91X_TCP_IP_TOTAL_SOCKETS(CONFIG_WIFI_SILABS_SIWX91X_TOTAL_SOCKETS);
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

Overview
********

A sample application that demonstrates the DNS client and ICMP functionality.This program will get the IP address
Copy link
Contributor

Choose a reason for hiding this comment

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

80 columns.

#define PING_PACKET_SIZE 64
#define PING_PACKETS 30

typedef enum siwx91x_app_state_e {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of exposing this in the header file?

struct net_if *iface);
static void handle_wifi_connect_result(struct net_mgmt_event_callback *cb);
static sl_status_t network_event_handler(sl_net_event_t event, sl_status_t status, void *data,
uint32_t data_length);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of declaring static (private) functions in public header?

(k_thread_entry_t)application_start, NULL, NULL, NULL,
K_PRIO_PREEMPT(10), 0, K_NO_WAIT);

k_thread_start(application_tid);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a the point of creating a new thread?

return;
} break;
default: {
k_sleep(K_MSEC(500));
Copy link
Contributor

Choose a reason for hiding this comment

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

From a power consumption, latency and software design perspective, this loop event is not a great idea. Please remove it.

case SIWX91X_WIFI_CONNECT_STATE: {
struct wifi_connect_req_params *cnx_params = NULL;

cnx_params = malloc(sizeof(struct wifi_connect_req_params));
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you don't use the stack to allocate this struct?

};
} context;

#define WIFI_SHELL_MGMT_EVENTS (NET_EVENT_WIFI_CONNECT_RESULT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this symbol useful?

CONFIG_NET_DHCPV6=n
CONFIG_MAIN_STACK_SIZE=2048
CONFIG_WIFI_SILABS_SIWX91X_PING=y
CONFIG_WIFI_SILABS_SIWX91X_DNS_CLIENT=y No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some numbers showing the added value of CONFIG_WIFI_SILABS_SIWX91X_PING and CONFIG_WIFI_SILABS_SIWX91X_DNS_CLIENT compared to the Zephyr native implementation.

ragurram26 and others added 17 commits June 5, 2025 10:51
To support the SiWx917 SNTP  Network Stack Application protocol
offloading, we need to integrate the Silicon Labs APIs,
so adding the corresponding source files and include files
in the kconfig and CMakelist

Co-authored-by: Swami Das Nampalli <swami.das@silabs.com>
Signed-off-by: Swami Das Nampalli <swami.das@silabs.com>
Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 HTTP  Network Stack Application
protocol offloading, we need to integrate the Silicon
Labs APIs, so adding the corresponding source files
and include files in the kconfig and CMakelist

Co-authored-by: Swami Das Nampalli <swami.das@silabs.com>
Signed-off-by: Swami Das Nampalli <swami.das@silabs.com>
Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 MQTT Network Stack Application
protocol offloading, we need to integrate the Silicon
Labs APIs, so adding the corresponding source files
and include files in the kconfig and CMakelist

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 ICMP Network Stack Application
protocol offloading, we need to integrate the Silicon
Labs APIs, so adding the corresponding source files
and include files in the kconfig and CMakelist

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 Websockets Network Stack Application
protocol offloading, we need to integrate the Silicon Labs
APIs, so adding the corresponding source files and include
files in the kconfig and CMakelist

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 MDNS Network Stack Application
protocol offloading, we need to integrate the Silicon Labs
APIs, so adding the corresponding source files and include
files in the kconfig and CMakelist

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 DNS Network Stack Application
protocol offloading, we need to integrate the Silicon Labs
APIs, so adding the corresponding source files and include
files in the kconfig and CMakelist

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 TLS Sockets , Need to
enable the TLS bitmap in the intilization

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 Single TLS Sockets,
Need to enable the Single TLS bitmap in
the intilization

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 Three SSL Sockets,
Need to enable the Three SSL bitmap in
the intilization

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 SSL high streaming
throughput, Need to enable the high streaming
bitmap in the intilization

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
To support the SiWx917 Multiple TLS version,
Need to enable the SSL version bitmap
in the intilization

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
Added support for 16K SSL record sizes, which
improves performance for SSL connections
that use larger record sizes

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
Added support to allocate additional memory for
SSL/TLS connections, typically required for
connections to cloud servers

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
Added support to Configure number of selects in SiWx917

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
Added support to Configure number of sockets in SiWx917

Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
This application demonstrates support for SiWx91x
ICMP and DNS protocols

Co-authored-by: Swami Das Nampalli <swami.das@silabs.com>
Signed-off-by: Swami Das Nampalli <swami.das@silabs.com>
Signed-off-by: Rahul Gurram <rahul.gurram@silabs.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 5, 2025

@github-actions
Copy link

github-actions bot commented Aug 5, 2025

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Samples Samples area: Wi-Fi Wi-Fi DNM (manifest) This PR should not be merged (controlled by action-manifest) manifest manifest-hal_silabs platform: Silabs Silicon Labs platform: STM32 ST Micro STM32 Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants