Add support for FreeRTOS TCP/IP stack #9

urutva · 2023-10-04T13:10:12Z

Add support for FreeRTOS TCP/IP stack

Description

This PR adds support for FreeRTOS TCP/IP stack and makes it the default network stack for the FRI.

Test Steps

Tested AWS IoT core end to end connectivity and OTA job.

Checklist:

I have tested my changes. No regression in existing tests.
I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

Depends on FreeRTOS/FreeRTOS-Plus-TCP#1009

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aggarg · 2023-10-11T05:25:35Z

Config/freertos-config/FreeRTOSConfig.h

@@ -62,7 +62,7 @@ extern uint32_t SystemCoreClock;
 #define configTICK_RATE_HZ    ( ( uint32_t ) 100 ) /* Scheduler polling rate of 1000 Hz */

 #define pdMS_TO_TICKS( xTimeInMs )    ( ( TickType_t ) xTimeInMs )


Why are you defining these? These are already defined in projdefs.h.

Added description about FVP behaviour and the justification for setting configTICK_RATE_HZ to 100 and the custom definition of pdMS_TO_TICKS.

aggarg · 2023-10-11T05:26:06Z

Config/freertos-config/FreeRTOSConfig.h

@@ -62,7 +62,7 @@ extern uint32_t SystemCoreClock;
 #define configTICK_RATE_HZ    ( ( uint32_t ) 100 ) /* Scheduler polling rate of 1000 Hz */


Comment does not match the actual value.

Maybe let's just specify the unit in the comment so we never need to update this again should the value change.
OR just delete the comment since the macro already has the unit.

Added description about FVP behaviour and the justification for setting configTICK_RATE_HZ to 100 and the custom definition of pdMS_TO_TICKS.

aggarg · 2023-10-11T05:26:43Z

Config/freertos-config/FreeRTOSIPConfig.h

+#if ( ipconfigHAS_DEBUG_PRINTF == 1 )
+    #include <stdio.h>
+    #define FreeRTOS_debug_printf( X )   \
+    printf( "%p->%s %d: ",               \


Why both prrintf and vLoggingPrintf?

This was copied from one of the examples. Removed additional printf.

aggarg · 2023-10-11T05:27:30Z

Config/freertos-config/FreeRTOSIPConfig.h

+ * messages. */
+#define ipconfigHAS_PRINTF    1
+#if ( ipconfigHAS_PRINTF == 1 )
+    #include <stdio.h>


Why is this include needed here? Should this not be in the implementation file of vLoggingPrintf?

Good catch. This is a typo. #include <stdio.h> removed.

aggarg · 2023-10-11T05:28:15Z

Config/freertos-config/FreeRTOSIPConfig.h

+ */
+#define ipconfigIPv4_BACKWARD_COMPATIBLE    0
+
+#define portINLINE                          __inline


This should be part of portmacro.h, if not already there.

This is not needed. Removed.

aggarg · 2023-10-11T05:34:06Z

Middleware/FreeRTOS/FreeRTOS-Plus-TCP-lib/src/transport_interface_api.h

+ *
+ * @param[out] pNetworkContext The output parameter to return the created network context.
+ * @param[in] pServerInfo Server connection info.
+ * @param[in] pTLSParams socket configs for the connection.


Params sendTimeoutMs and recvTimeoutMs not documented.

Function header updated.

aggarg · 2023-10-11T05:34:41Z

Middleware/FreeRTOS/FreeRTOS-Plus-TCP-lib/src/transport_interface_api.h

+ * over the network.
+ *
+ * @param[in] pNetworkContext The network context created using Secure Sockets API.
+ * @param[in] pBuffer Buffer containing the bytes to send over the network stack.


Suggested change

* @param[in] pBuffer Buffer containing the bytes to send over the network stack.

* @param[in] pMessage Buffer containing the bytes to send over the network stack.

Function header updated.

aggarg · 2023-10-11T05:36:32Z

Middleware/FreeRTOS/FreeRTOS-Plus-TCP-lib/src/transport_mbedtls.c

+                /* Set socket receive timeout. */
+                /* transportTimeout = pdMS_TO_TICKS( recvTimeoutMs ); */
+                /* / * Setting the receive block time cannot fail. * / */
+                /* ( void ) FreeRTOS_setsockopt( pNetworkContext->socket, */
+                /*                               0, */
+                /*                               FREERTOS_SO_RCVTIMEO, */
+                /*                               &transportTimeout, */
+                /*                               sizeof( TickType_t ) ); */
+
+                /* / * Set socket send timeout. * / */
+                /* transportTimeout = pdMS_TO_TICKS( sendTimeoutMs ); */
+                /* / * Setting the send block time cannot fail. * / */
+                /* ( void ) FreeRTOS_setsockopt( pNetworkContext->socket, */
+                /*                               0, */
+                /*                               FREERTOS_SO_SNDTIMEO, */
+                /*                               &transportTimeout, */
+                /*                               sizeof( TickType_t ) ); */


Should we remove this commented code?

Good catch. We need this to set the send/receive timeout on the socket. Code retained.

aggarg · 2023-10-11T05:39:57Z

Projects/aws-iot-example/main.c

+                                             uint32_t ulDestinationAddress,
+                                             uint16_t usDestinationPort )
+{
+    psa_status_t xPsaStatus = PSA_ERROR_PROGRAMMER_ERROR;


Consider adding the following to avoid unused variable warning -

( void ) ulSourceAddress; ( void ) usSourcePort; ( void ) ulDestinationAddress; ( void ) usDestinationPort;

hugueskamba · 2023-10-11T09:27:00Z

manifest.yml

+    version: "3d5ee0e821cab38cb6e6265fcf1ce7552a54519d"
+    repository:
+      type: "git"
+      url: "https://github.com/urutva/FreeRTOS-Plus-TCP.git"


This should be updated to https://github.com/FreeRTOS/FreeRTOS-Plus-TCP.git

I am not sure if I am missing something but I cannot see it changed.
It is still pointing to the fork.

Here is my current URL: https://github.com/FreeRTOS/iot-reference-arm-corstone3xx/pull/9/files

Please feel free to resolve this. I do not have a resolve button.

hugueskamba · 2023-10-11T12:15:06Z

Projects/blinky/CMakeLists.txt

@@ -47,6 +47,8 @@ else()
    add_compile_options(-O1)
 endif()

+set(NETWORK_COMPONENT "IOT-VSOCKET")


Blinky does not need networking.

Good catch. Removed.

Please feel free to resolve this. I do not have a resolve button.

hugueskamba · 2023-10-11T12:53:55Z

Projects/aws-iot-example/CMakeLists.txt

+# Select the components required by the application
+set(NETWORK_COMPONENT "IOT-VSOCKET")


Suggested change

# Select the components required by the application

set(NETWORK_COMPONENT "IOT-VSOCKET")

set(CONNECTIVITY_STACK "IOT_VSOCKET" CACHE STRING "Choose the connectivity stack to use")

set_property(CACHE CONNECTIVITY_STACK PROPERTY STRINGS "IOT_VSOCKET")

NETWORK_COMPONENT changed to CONNECTIVITY_STACK .
May be I'm missing something, what is the need to set a property on the variable which isn't used by the build system. If the intention was to describe the possible values, I've added them to the description string in the variable definition.

I think it is so if a UI is used for CMake, the possible values will be selectable.

At the moment the connectivity stack is only selectable by changing the CMakeLists.txt. Perhaps also add an optional argument for the build shell script?

Currently there is no plan to support CMake UI in FRI.

Providing an option to select a network in the build script is useful, however, I cannot think of an use case where developers would like to switch network stack during each build. Hence, hard-coding it in CMake code is a better approach.

Please feel free to resolve this. I do not have a resolve button.

hugueskamba · 2023-10-11T12:55:05Z

Middleware/ARM/IoT_VSocket-lib/CMakeLists.txt

@@ -15,6 +15,8 @@ add_library(iot-vsocket STATIC
    transport_tls_iot_socket.c
 )

+add_library(network-component ALIAS iot-vsocket)


Suggested change

add_library(network-component ALIAS iot-vsocket)

add_library(connectivity-stack ALIAS iot-vsocket)

Please feel free to resolve this. I do not have a resolve button.

hugueskamba · 2023-10-11T12:57:05Z

Projects/aws-iot-example/CMakeLists.txt

 # Select the components required by the application
-set(NETWORK_COMPONENT "IOT-VSOCKET")
+set(NETWORK_COMPONENT "FREERTOS-TCP-IP")


Suggested change

# Select the components required by the application

set(NETWORK_COMPONENT "IOT-VSOCKET")

set(NETWORK_COMPONENT "FREERTOS-TCP-IP")

set(CONNECTIVITY_STACK "FREERTOS_PLUS_TCP" CACHE STRING "Choose the connectivity stack to use")

set_property(CACHE CONNECTIVITY_STACK PROPERTY STRINGS "FREERTOS_PLUS_TCP" "IOT_VSOCKET")

NETWORK_COMPONENT changed to CONNECTIVITY_STACK.
May be I'm missing something, what is the need to set a property on the variable which isn't used by the build system. If the intention was to describe the possible values, I've added them to the description string in the variable definition.

I think it is so if a UI is used for CMake, the possible values will be selectable.

At the moment the connectivity stack is only selectable by changing the CMakeLists.txt. Perhaps also add an optional argument for the build shell script?

Same as #9 (comment)

Please feel free to resolve this. I do not have a resolve button.

hugueskamba · 2023-10-11T12:58:37Z

Middleware/FreeRTOS/CMakeLists.txt

@@ -3,3 +3,6 @@
 # SPDX-License-Identifier: MIT

 add_subdirectory(kernel)
+if (${NETWORK_COMPONENT} STREQUAL "FREERTOS-TCP-IP")


Suggested change

if (${NETWORK_COMPONENT} STREQUAL "FREERTOS-TCP-IP")

if(CONNECTIVITY_STACK STREQUAL "FREERTOS_PLUS_TCP")

Please feel free to resolve this. I do not have a resolve button.

hugueskamba · 2023-10-11T13:09:19Z

Projects/aws-iot-example/mqtt-agent-wrapper/CMakeLists.txt

        app-config
        app-logging
        event-helper
+    PUBLIC


What is the reason for having this PUBLIC?
Is it worth explaining with a comment or in the PR?

It turns out that the dependency on connectivity-stack need not be public. Changed the dependency to PRIVATE.

Please feel free to resolve this. I do not have a resolve button.

Instead of directly using malloc, use FreeRTOS provided malloc API. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

FreeRTOS TCP/IP stack recommends `heap_4` and `BufferAllocation_2`. https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/tree/V4.0.0-appsec#note Since FreeRTOS manages the heap, set Mbed TLS memory APIs to use FreeRTOS heap APIs (mbedtls_platform_set_calloc_free). In addition, reduce allocated heap size from `0x000b0000` to `0x00000400`, since FreeRTOS manages heap when `heap_4` configuration is selected. Ideally, heap size should be 0, however, GNU expects linker symbol `end` to be available. Hence, set it to `0x400`. Also, set the FreeRTOS config `configAPPLICATION_ALLOCATED_HEAP` to 0, indicating FreeRTOS allocates the heap based on `configTOTAL_HEAP_SIZE` (720896 bytes). Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

The socket creation is part of `Middleware/ARM/IoT_VSocket-lib/AVH/interface/vsocket/iot_socket.c` and hence `socket_startup.c` is redundant. In addition, `socket_startup()` always returns 0. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

The system event helper library provides system event management, so that the tasks can wait on the required system event to happen. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Add a CMake variable `CONNECTIVITY_STACK`, so that the application can select a network stack from the supported network stacks. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Since we are integrating latest version of FreeRTOS TCP/IP stack which relies on FreeRTOS kernel, ensure that we are using the latest released version of FreeRTOS kernel which is `V10.6.1`. In addtion, fix a typo and remove unused macro from `FreeRTOSConfig.h`. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Added support for FreeRTOS TCP/IP stack and made it the default choice of network stack. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Added support for choosing either ecosystem FVP or Arm virtual hardware FVP running in the AMI to the `run.sh` script. The ecosystem FVP is used by default. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

With the addition of support for FreeRTOS TCP/IP stack, in addition to Arm Virtual Hardware using Amazon Machine Images, Arm FRI can now run ecosystem FVPs. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

Provide justification for setting `configTICK_RATE_HZ` to `100` to simulate scheduler polling rate of `1000 Hz` or 1 tick per second. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

hugueskamba · 2023-10-12T17:12:13Z

Bsp/an552_ns.ld

@@ -9,7 +9,7 @@
 */

 __STACK_SIZE = 0x00002800;
-__HEAP_SIZE  = 0x000b0000;
+__HEAP_SIZE  = 0x00000400;


As the linker scripts and FreeRTOSConfig.h are used by the two applications, is it also necessary to modify Blinky to use Heap 4 instead of Heap 3?

Can we move the heap selection to https://github.com/FreeRTOS/iot-reference-arm-corstone3xx/blob/main/Middleware/FreeRTOS/CMakeLists.txt so it applies to all applications using the FRI?

Good suggestion. Let's handle that as part of another PR to improve blinky example.

urutva requested a review from a team as a code owner October 4, 2023 13:10

urutva force-pushed the add-freertos-tcp-ip-stack-support branch from 4b2f7d0 to c17bd9b Compare October 4, 2023 13:23

urutva mentioned this pull request Oct 5, 2023

Add Ethernet driver for Corstone-300 FVP (MPS3_AN552) FreeRTOS/FreeRTOS-Plus-TCP#1009

Merged

2 tasks

urutva force-pushed the add-freertos-tcp-ip-stack-support branch from c17bd9b to 767e6de Compare October 9, 2023 08:38

sirnish self-requested a review October 10, 2023 20:11

aggarg reviewed Oct 11, 2023

View reviewed changes

hugueskamba reviewed Oct 11, 2023

View reviewed changes

urutva added 6 commits October 11, 2023 13:18

freertos-integration-tests: Use FreeRTOS malloc API

9071e8e

Instead of directly using malloc, use FreeRTOS provided malloc API. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

event-helper: Create a new system event helper library

08004e0

The system event helper library provides system event management, so that the tasks can wait on the required system event to happen. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

fri: Add network stack selection feature

55bd162

Add a CMake variable `CONNECTIVITY_STACK`, so that the application can select a network stack from the supported network stacks. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

urutva force-pushed the add-freertos-tcp-ip-stack-support branch from 767e6de to b9b82d7 Compare October 11, 2023 16:01

aggarg previously approved these changes Oct 12, 2023

View reviewed changes

urutva added 7 commits October 12, 2023 11:36

fri: Add FreeRTOS TCP/IP as git submodule

686027c

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

fri: Add support for FreeRTOS TCP/IP stack

8dffe6b

Added support for FreeRTOS TCP/IP stack and made it the default choice of network stack. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

scripts: Add support for FVP choice

7c9c2c2

Added support for choosing either ecosystem FVP or Arm virtual hardware FVP running in the AMI to the `run.sh` script. The ecosystem FVP is used by default. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

docs: Add instrucitons to install Arm ecosystem FVPs

893be54

With the addition of support for FreeRTOS TCP/IP stack, in addition to Arm Virtual Hardware using Amazon Machine Images, Arm FRI can now run ecosystem FVPs. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

ci: Update spell-check dictionary

d62e8ca

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

iot-vsocket: Fix/update function headers

f81d8ed

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

config: Add description about FVP behaviour

6463898

Provide justification for setting `configTICK_RATE_HZ` to `100` to simulate scheduler polling rate of `1000 Hz` or 1 tick per second. Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>

urutva dismissed aggarg’s stale review via 6463898 October 12, 2023 11:36

urutva force-pushed the add-freertos-tcp-ip-stack-support branch from b9b82d7 to 6463898 Compare October 12, 2023 11:36

hugueskamba approved these changes Oct 12, 2023

View reviewed changes

hugueskamba reviewed Oct 12, 2023

View reviewed changes

aggarg approved these changes Oct 12, 2023

View reviewed changes

amazonKamath self-requested a review October 13, 2023 09:30

amazonKamath approved these changes Oct 13, 2023

View reviewed changes

urutva merged commit 52b13ab into FreeRTOS:main Oct 13, 2023
4 checks passed

		@@ -62,7 +62,7 @@ extern uint32_t SystemCoreClock;
		#define configTICK_RATE_HZ ( ( uint32_t ) 100 ) /* Scheduler polling rate of 1000 Hz */

		#define pdMS_TO_TICKS( xTimeInMs ) ( ( TickType_t ) xTimeInMs )

	* @param[in] pBuffer Buffer containing the bytes to send over the network stack.
	* @param[in] pMessage Buffer containing the bytes to send over the network stack.

		# Select the components required by the application
		set(NETWORK_COMPONENT "IOT-VSOCKET")

	add_library(network-component ALIAS iot-vsocket)
	add_library(connectivity-stack ALIAS iot-vsocket)

	if (${NETWORK_COMPONENT} STREQUAL "FREERTOS-TCP-IP")
	if(CONNECTIVITY_STACK STREQUAL "FREERTOS_PLUS_TCP")

Add support for FreeRTOS TCP/IP stack #9

Add support for FreeRTOS TCP/IP stack #9

Conversation

urutva commented Oct 4, 2023

Description

Test Steps

Checklist:

Related Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urutva Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urutva Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugueskamba Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugueskamba Oct 12, 2023 • edited by urutva Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urutva Oct 11, 2023 •

edited

Loading

urutva Oct 11, 2023 •

edited

Loading

hugueskamba Oct 11, 2023 •

edited

Loading

hugueskamba Oct 12, 2023 •

edited by urutva

Loading