Skip to content
This repository has been archived by the owner on Nov 11, 2024. It is now read-only.

Commit

Permalink
A different approach to the PPP OOS problem:
Browse files Browse the repository at this point in the history 
- on the CMUX side, don't leave the old "direct" AT client locked; this means that CMUX can be taken down again by any task (otherwise FreeRTOS complains that the task unlocking the mutex is not the same as the task that had the mutex locked),
- on the PPP side, don't use the IP addresses passed in: ESP-IDF PPP picks these up during PPP LCP negotiation anyway, and not needing these allows the next bullet point,
- uPortPppReconnect() now stops the PPP link up into ESP-IDF (but doesn't destroy it), disconnects and reconnects PPP from a cellular module standpoint and then restarts the PPP link up into ESP-IDF afterwards; this does a proper/full reconnection, which might not _always_ be necessary but sometimes is: better safe than sorry.
  • Loading branch information
@RobMeades
RobMeades committed Sep 23, 2024
1 parent 5521e30 commit 62be5fc
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 167 deletions.
6 changes: 6 additions & 0 deletions cell/src/u_cell_mux.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1761,6 +1761,11 @@ int32_t uCellMuxPrivateEnable(uCellPrivateInstance_t *pInstance)
// of our instance to the new AT handle, leaving the
// old AT handle locked
pInstance->atHandle = atHandle;
// Unlock the now inactive AT client handle from this task,
// just in case we want to switch CMUX off from some
// other task (FreeRTOS doesn't like mutexes being unlocked
// by a task that didn't lock them).
uAtClientUnlock(pContext->savedAtHandle);
// The setting of echo-off and AT+CMEE is port-specific,
// so we need to set those here for the new port
#ifdef U_CFG_CELL_ENABLE_NUMERIC_ERROR
Expand Down Expand Up @@ -1892,6 +1897,7 @@ int32_t uCellMuxPrivateDisable(uCellPrivateInstance_t *pInstance)
if (pContext->savedAtHandle != NULL) {
// Copy the settings of the AT handler on channel 1
// back into the original one, in case they have changed
uAtClientLock(pContext->savedAtHandle);
errorCode = uCellMuxPrivateCopyAtClient(atHandle, pContext->savedAtHandle);
// While we set the error code above, there's not a whole lot
// we can do if this fails, so continue anyway; close the
Expand Down
23 changes: 0 additions & 23 deletions common/network/api/u_network.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,6 @@
* powered-up and may be reconfigured etc.: you
* must call uNetworkInterfaceUp() to talk with
* it again.
* IMPORTANT: under some circumstances, e.g. when
* using a GNSS device inside a cellular module
* or when using PPP with cellular,
* uNetworkInterfaceUp() may leave a mutex locked
* that will be released by uNetworkInterfaceDown(),
* hence it is important that uNetworkInterfaceDown()
* is called from the same task that called
* uNetworkInterfaceUp().
* uDeviceClose(): call this to power the device down and clear
* up any resources belonging to it; uDeviceOpen()
* must be called to re-instantiate the device.
Expand Down Expand Up @@ -236,21 +228,6 @@ int32_t uNetworkInterfaceUp(uDeviceHandle_t devHandle, uNetworkType_t netType,
* Note: for a Wi-Fi network, this function uses the
* uWifiSetConnectionStatusCallback() callback.
*
* Note: if you call uNetworkInterfaceDown() on a GNSS network that
* is inside a cellular device, or if you have been using PPP with
* a cellular module (i.e. U_CFG_PPP_ENABLE is defined), it is possible
* your RTOS will assert that a mutex is being released by a task that
* does not own it; for example FreeRTOS may do this. The reason for
* this is that, when uNetworkInterfaceUp() was called, the cellular
* module will have been told to create a CMUX channel (to carry AT
* and either PPP or GNSS traffic simultaneously) and the original AT
* client will have been left locked. If uNetworkInterfaceDown() is
* called from a _different_ _task_ to the one that called
* uNetworkInterfaceUp(), the assert will be triggered when the original
* AT client is unlocked. A fix for this is to call uNetworkInterfaceDown()
* from the same task that called uNetworkInterfaceUp(). We are
* investigating whether there is a way to remove this restriction.
*
* @param devHandle the handle of the device that is carrying the
* network.
* @param netType which of the module interfaces to take down.
Expand Down
198 changes: 54 additions & 144 deletions port/platform/esp-idf/src/u_port_ppp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#include "stddef.h" // NULL, size_t etc.
#include "stdint.h" // int32_t etc.
#include "stdbool.h"
#include "string.h" // memset()
#include "string.h" // memset(), strcpy()

#include "u_cfg_sw.h"
#include "u_error_common.h"
Expand Down Expand Up @@ -76,6 +76,20 @@
# define U_PORT_PPP_TX_LOOP_DELAY_MS 10
#endif

#ifndef U_PORT_PPP_USERNAME_MAX_LENGTH
/** How long a PPP user name is allowed to be, including room
* for a null terminator.
*/
# define U_PORT_PPP_USERNAME_MAX_LENGTH 64
#endif

#ifndef U_PORT_PPP_PASSWORD_MAX_LENGTH
/** How long a PPP password is allowed to be, including room
* for a null terminator.
*/
# define U_PORT_PPP_PASSWORD_MAX_LENGTH 64
#endif

/* ----------------------------------------------------------------
* TYPES
* -------------------------------------------------------------- */
Expand All @@ -91,10 +105,8 @@ struct uPortPppInterface_t;
typedef struct {
esp_netif_driver_base_t base;
struct uPortPppInterface_t *pPppInterface;
uSockIpAddress_t *pIpAddress;
uSockIpAddress_t *pDnsIpAddressPrimary;
const char *pUsername;
const char *pPassword;
char username[U_PORT_PPP_USERNAME_MAX_LENGTH];
char password[U_PORT_PPP_PASSWORD_MAX_LENGTH];
uPortPppAuthenticationMode_t authenticationMode;
} uPortPppNetifDriver_t;

Expand Down Expand Up @@ -160,98 +172,6 @@ static uPortPppInterface_t *pFindPppInterface(void *pDevHandle)
return pPppInterface;
}

/** Convert an IP address of ours to ESP-IDF format.
*/
static int32_t convertIpAddress(uSockIpAddress_t *pIn, esp_ip_addr_t *pOut)
{
int32_t espError = ESP_ERR_INVALID_ARG;

memset(pOut, 0, sizeof(*pOut));
switch (pIn->type) {
case U_SOCK_ADDRESS_TYPE_V4:
pOut->type = ESP_IPADDR_TYPE_V4;
pOut->u_addr.ip4.addr = ESP_IP4TOADDR(((pIn->address.ipv4 >> 24) && 0xFF),
((pIn->address.ipv4 >> 16) && 0xFF),
((pIn->address.ipv4 >> 8) && 0xFF),
(pIn->address.ipv4 && 0xFF));
espError = ESP_OK;
break;
case U_SOCK_ADDRESS_TYPE_V6:
pOut->type = ESP_IPADDR_TYPE_V6;
for (size_t x = 0; x < sizeof(pOut->u_addr.ip6.addr) / sizeof(pOut->u_addr.ip6.addr[0]); x++) {
pOut->u_addr.ip6.addr[x] = ESP_IP4TOADDR(((pIn->address.ipv6[x] >> 24) && 0xFF),
((pIn->address.ipv6[x] >> 16) && 0xFF),
((pIn->address.ipv6[x] >> 8) && 0xFF),
(pIn->address.ipv6[x] && 0xFF));
}
espError = ESP_OK;
break;
default:
break;
}

return espError;
}

#if 0
// Switch off DHCP and tell the IP stack what our IP address is.
static esp_err_t setIpAddress(esp_netif_t *pEspNetif, uSockIpAddress_t *pIpAddress)
{
esp_err_t espError = ESP_ERR_INVALID_ARG;
esp_ip_addr_t espIpAddress = {0};
esp_netif_ip_info_t ipInfo = {0};

switch (pIpAddress->type) {
case U_SOCK_ADDRESS_TYPE_V4:
ipInfo.netmask.addr = ESP_IP4TOADDR(0xFF, 0xFF, 0xFF, 0);
// TODO ipInfo.gw
espError = convertIpAddress(pIpAddress, &espIpAddress);
break;
case U_SOCK_ADDRESS_TYPE_V6:
espError = ESP_ERR_NOT_SUPPORTED;
break;
default:
break;
}
if (espError == ESP_OK) {
memcpy(&ipInfo.ip, &espIpAddress.u_addr.ip4, sizeof(ipInfo.ip));
espError = esp_netif_dhcpc_stop(pEspNetif);
if (espError == ESP_ERR_ESP_NETIF_DHCP_ALREADY_STOPPED) {
espError = ESP_OK;
}
if (espError == ESP_OK) {
espError = esp_netif_set_ip_info(pEspNetif, &ipInfo);
}
}

return espError;
}

// Set a DNS address.
static esp_err_t setDnsAddress(esp_netif_t *pEspNetif, esp_netif_dns_type_t type,
uSockIpAddress_t *pIpAddress)
{
esp_err_t espError = ESP_ERR_INVALID_ARG;
esp_netif_dns_info_t dnsInfo = {0};

switch (pIpAddress->type) {
case U_SOCK_ADDRESS_TYPE_V4:
espError = convertIpAddress(pIpAddress, &dnsInfo.ip);
break;
case U_SOCK_ADDRESS_TYPE_V6:
espError = ESP_ERR_NOT_SUPPORTED;
break;
default:
break;
}
if (espError == ESP_OK) {
espError = esp_netif_set_dns_info(pEspNetif, type, &dnsInfo);
}

return espError;
}
#endif

// This function is provided as a callback to the NETIF layer of
// ESP-IDF in the structure esp_netif_driver_ifconfig_t, see
// postAttachStart().
Expand Down Expand Up @@ -325,26 +245,6 @@ static esp_err_t postAttachStart(esp_netif_t *pEspNetif, void *pArgs)
}
}

#if 0
if ((espError == ESP_OK) && (pDriver->pIpAddress != NULL)) {
espError = setIpAddress(pEspNetif, pDriver->pIpAddress);
pDriver->pIpAddress = NULL; // NULL'ed so that we don't think it can be used again
}

if (espError == ESP_OK) {
if (pDriver->pDnsIpAddressPrimary != NULL) {
espError = setDnsAddress(pEspNetif, ESP_NETIF_DNS_MAIN, pDriver->pDnsIpAddressPrimary);
pDriver->pDnsIpAddressPrimary = NULL; // NULL'ed so that we don't think it can be used again
} else {
uSockAddress_t address;
if (uSockStringToAddress(U_PORT_PPP_DNS_PRIMARY_DEFAULT_STR, &address) > 0) {
espError = setDnsAddress(pEspNetif, ESP_NETIF_DNS_MAIN, &address.ipAddress);
}
}
}
// Note: secondary DNS address not supported by ESP-IDF for PPP
#endif

#if defined(CONFIG_LWIP_PPP_PAP_SUPPORT) || defined(CONFIG_LWIP_PPP_CHAP_SUPPORT)
if (espError == ESP_OK) {
// Choose at least PAP since otherwise LCP negotiation will fail
Expand All @@ -353,18 +253,8 @@ static esp_err_t postAttachStart(esp_netif_t *pEspNetif, void *pArgs)
if (authenticationType != NETIF_PPP_AUTHTYPE_CHAP) {
authenticationType = NETIF_PPP_AUTHTYPE_PAP;
}
// Set the username/password fields to at least be empty strings
// otherwise the authentication mode will not be accepted
if (pDriver->pUsername == NULL) {
pDriver->pUsername = "";
}
if (pDriver->pPassword == NULL) {
pDriver->pPassword = "";
}
espError = esp_netif_ppp_set_auth(pEspNetif, authenticationType,
pDriver->pUsername, pDriver->pPassword);
pDriver->pUsername = NULL; // NULL'ed so that we don't think
pDriver->pPassword = NULL; // they can be used again
pDriver->username, pDriver->password);
}
#endif

Expand Down Expand Up @@ -588,7 +478,10 @@ int32_t uPortPppConnect(void *pDevHandle,
esp_netif_t *pEspNetif = NULL;
size_t guardCount = 0;

// ESP-IDF can't use a secondary DNS address on a PPP connection
// ESP-IDF obtains these during LCP negotiation,
// we don't need to use the values passed in
(void) pIpAddress;
(void) pDnsIpAddressPrimary;
(void) pDnsIpAddressSecondary;

if (gMutex != NULL) {
Expand All @@ -599,7 +492,9 @@ int32_t uPortPppConnect(void *pDevHandle,
if ((pUsername == NULL) && (pPassword == NULL)) {
authenticationMode = U_PORT_PPP_AUTHENTICATION_MODE_NONE;
}
if (authenticationMode < U_PORT_PPP_AUTHENTICATION_MODE_MAX_NUM) {
if ((authenticationMode < U_PORT_PPP_AUTHENTICATION_MODE_MAX_NUM) &&
((pUsername == NULL) || (strlen(pUsername) < U_PORT_PPP_USERNAME_MAX_LENGTH)) &&
((pPassword == NULL) || (strlen(pPassword) < U_PORT_PPP_PASSWORD_MAX_LENGTH))) {
errorCode = (int32_t) U_ERROR_COMMON_NOT_FOUND;
pPppInterface = pFindPppInterface(pDevHandle);
if (pPppInterface != NULL) {
Expand All @@ -612,14 +507,16 @@ int32_t uPortPppConnect(void *pDevHandle,
// pPppInterface->netifDriver.base.netif
pPppInterface->netifDriver.base.post_attach = postAttachStart;
pPppInterface->netifDriver.pPppInterface = pPppInterface;
// Note that only the pointers are stored for these parameters,
// the contents are not copied: this is fine since they are
// used by postAttachStart(), which is called by
// esp_netif_action_start(), and that's it
pPppInterface->netifDriver.pIpAddress = pIpAddress;
pPppInterface->netifDriver.pDnsIpAddressPrimary = pDnsIpAddressPrimary;
pPppInterface->netifDriver.pUsername = pUsername;
pPppInterface->netifDriver.pPassword = pPassword;
pPppInterface->netifDriver.username[0] = 0;
if (pUsername != NULL) {
// Length is checked above so strcpy() is fine here
strcpy(pPppInterface->netifDriver.username, pUsername);
}
pPppInterface->netifDriver.password[0] = 0;
if (pPassword != NULL) {
// Length is checked above so strcpy() is fine here
strcpy(pPppInterface->netifDriver.password, pPassword);
}
pPppInterface->netifDriver.authenticationMode = authenticationMode;
errorCode = (int32_t) U_ERROR_COMMON_PLATFORM;
if ((esp_event_handler_register(NETIF_PPP_STATUS, ESP_EVENT_ANY_ID, eventPppChanged,
Expand Down Expand Up @@ -683,6 +580,10 @@ int32_t uPortPppReconnect(void *pDevHandle,
uPortPppInterface_t *pPppInterface;
esp_netif_t *pEspNetif = NULL;

// ESP-IDF obtains the IP address during LCP negotiation,
// we don't need to use the one passed in
(void) pIpAddress;

if (gMutex != NULL) {

U_PORT_MUTEX_LOCK(gMutex);
Expand All @@ -692,14 +593,20 @@ int32_t uPortPppReconnect(void *pDevHandle,
if (pPppInterface != NULL) {
errorCode = (int32_t) U_ERROR_COMMON_PLATFORM;
pEspNetif = pPppInterface->netifDriver.base.netif;
#if 0
if ((pEspNetif != NULL) && (setIpAddress(pEspNetif, pIpAddress) == ESP_OK)) {
#else
(void) pIpAddress;
if (pEspNetif != NULL) {
#endif
errorCode = (int32_t) U_ERROR_COMMON_SUCCESS;
// Perform a disconnect and then connect again
// Perform a disconnect and then connect again,
// including stopping and re-starting the PPP
// connection up into ESP-IDF
if (pPppInterface->ipConnected) {
esp_netif_action_disconnected(pPppInterface->netifDriver.base.netif, NULL, 0, NULL);
}
esp_netif_action_stop(pPppInterface->netifDriver.base.netif, NULL, 0, NULL);
// Wait for the IP stack to let us go
uPortLog("U_PORT_PPP: waiting to be released in order to reconnect.\n");
uPortSemaphoreTryTake(pPppInterface->semaphoreExit,
U_PORT_PPP_SHUTDOWN_TIMEOUT_SECONDS * 1000);
uPortLog("U_PORT_PPP: released.\n");
if (pPppInterface->pDisconnectCallback != NULL) {
pPppInterface->pDisconnectCallback(pPppInterface->pDevHandle,
pPppInterface->ipConnected);
Expand All @@ -711,6 +618,9 @@ int32_t uPortPppReconnect(void *pDevHandle,
U_PORT_PPP_RECEIVE_BUFFER_BYTES,
NULL);
}
if (errorCode == 0) {
esp_netif_action_start(pEspNetif, NULL, 0, NULL);
}
}
}

Expand Down

0 comments on commit 62be5fc

Please sign in to comment.