From f6a5e24873de3564935ffbb8c9ace8d9cdb610bf Mon Sep 17 00:00:00 2001 From: Andrey Tolstoy Date: Tue, 20 Jun 2017 15:09:06 +0700 Subject: [PATCH 1/5] Constantly holding a mutex breaks priority inheritance mechanisms in FreeRTOS. Do not block _start mutex in ActiveObjectBase. It's not even used anywhere --- system/src/active_object.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/system/src/active_object.cpp b/system/src/active_object.cpp index 5732f9d033..57445deecf 100644 --- a/system/src/active_object.cpp +++ b/system/src/active_object.cpp @@ -41,7 +41,9 @@ void ActiveObjectBase::start_thread() void ActiveObjectBase::run() { - std::lock_guard lck (_start); + /* XXX: We shouldn't constantly hold a mutex. This breaks priority inhertiance mechanisms in FreeRTOS. */ + /* It's not even used anywhere */ + // std::lock_guard lck (_start); started = true; uint32_t last_background_run = 0; From f62225036ffd4013bbb21cb326987784cf6f859b Mon Sep 17 00:00:00 2001 From: Andrey Tolstoy Date: Tue, 20 Jun 2017 15:10:39 +0700 Subject: [PATCH 2/5] [Photon/P1] socket_hal: Some cosmetic changes: wiced semaphores replaced by os_mutex_t, std::vector replaced with spark::Vector, SocketListLock implemented with std::lock_guard --- hal/src/photon/socket_hal.cpp | 56 +++++++++++------------------------ 1 file changed, 18 insertions(+), 38 deletions(-) diff --git a/hal/src/photon/socket_hal.cpp b/hal/src/photon/socket_hal.cpp index 04dcd3975e..5fd5b70107 100644 --- a/hal/src/photon/socket_hal.cpp +++ b/hal/src/photon/socket_hal.cpp @@ -32,6 +32,8 @@ #include #include "lwip/api.h" #include "network_interface.h" +#include "spark_wiring_thread.h" +#include "spark_wiring_vector.h" wiced_result_t wiced_last_error( wiced_tcp_socket_t* socket); @@ -208,13 +210,12 @@ class tcp_server_client_t { struct tcp_server_t : public wiced_tcp_server_t { tcp_server_t() { - wiced_rtos_init_semaphore(&accept_lock); - wiced_rtos_set_semaphore(&accept_lock); + os_mutex_create(&accept_lock); memset(clients, 0, sizeof(clients)); } ~tcp_server_t() { - wiced_rtos_deinit_semaphore(&accept_lock); + os_mutex_destroy(accept_lock); } /** @@ -240,14 +241,14 @@ struct tcp_server_t : public wiced_tcp_server_t wiced_result_t accept(wiced_tcp_socket_t* socket) { wiced_result_t result; if ((result=wiced_tcp_accept(socket))==WICED_SUCCESS) { - wiced_rtos_get_semaphore(&accept_lock, WICED_WAIT_FOREVER); + os_mutex_lock(accept_lock); int idx = index(socket); if (idx>=0) { clients[idx] = new tcp_server_client_t(this, socket); - to_accept.insert(to_accept.end(), idx); + to_accept.append(idx); } - wiced_rtos_set_semaphore(&accept_lock); + os_mutex_unlock(accept_lock); } return result; } @@ -257,13 +258,13 @@ struct tcp_server_t : public wiced_tcp_server_t * @return The next client, or NULL */ tcp_server_client_t* next_accept() { - wiced_rtos_get_semaphore(&accept_lock, WICED_WAIT_FOREVER); + os_mutex_lock(accept_lock); int index = -1; if (to_accept.size()) { index = *to_accept.begin(); - to_accept.erase(to_accept.begin()); + to_accept.removeAt(0); } - wiced_rtos_set_semaphore(&accept_lock); + os_mutex_unlock(accept_lock); return index>=0 ? clients[index] : NULL; } @@ -273,12 +274,12 @@ struct tcp_server_t : public wiced_tcp_server_t * @return */ wiced_result_t notify_disconnected(wiced_tcp_socket_t* socket) { - wiced_rtos_get_semaphore(&accept_lock, WICED_WAIT_FOREVER); + os_mutex_lock(accept_lock); int idx = index(socket); tcp_server_client_t* client = clients[idx]; if (client) client->notify_disconnected(); - wiced_rtos_set_semaphore(&accept_lock); + os_mutex_unlock(accept_lock); return WICED_SUCCESS; } @@ -316,8 +317,8 @@ struct tcp_server_t : public wiced_tcp_server_t // for each server instance, maintain an associated tcp_server_client_t instance tcp_server_client_t* clients[WICED_MAXIMUM_NUMBER_OF_SERVER_SOCKETS]; - wiced_semaphore_t accept_lock; - std::vector to_accept; + os_mutex_t accept_lock; + spark::Vector to_accept; }; void tcp_server_client_t::close() { @@ -443,24 +444,6 @@ class socket_t bool isOpen() { return !closed && is_inner_open(); } }; -/** - * Lock/unlock a semaphore via RAII - */ -class SemaphoreLock -{ - wiced_semaphore_t& sem; - -public: - - SemaphoreLock(wiced_semaphore_t& sem_) : sem(sem_) { - wiced_rtos_get_semaphore(&sem, NEVER_TIMEOUT); - } - - ~SemaphoreLock() { - wiced_rtos_set_semaphore(&sem); - } -}; - /** * Maintains a singly linked list of sockets. Access to the list is not * made thread-safe - callers should use SocketListLock to correctly serialize @@ -469,19 +452,16 @@ class SemaphoreLock struct SocketList { socket_t* items; - wiced_semaphore_t semaphore; + Mutex mutex; public: SocketList() : items(NULL) { - wiced_rtos_init_semaphore(&semaphore); - wiced_rtos_set_semaphore(&semaphore); } ~SocketList() { - wiced_rtos_deinit_semaphore(&semaphore); } /** @@ -550,11 +530,11 @@ struct SocketList friend class SocketListLock; }; -struct SocketListLock : SemaphoreLock +struct SocketListLock : std::lock_guard { SocketListLock(SocketList& list) : - SemaphoreLock(list.semaphore) - {} + std::lock_guard(list.mutex) + {}; }; class ServerSocketList : public SocketList From bfe701f6fab81c99b84b9fc85e83b0053ee6389b Mon Sep 17 00:00:00 2001 From: Andrey Tolstoy Date: Tue, 20 Jun 2017 15:15:45 +0700 Subject: [PATCH 3/5] TCPClient: It's much better to just copy from a previously created s_invalid_client static TCPClient(socket_handle_invalid()) when there are no new connections, instead of allocating new one each call --- wiring/src/spark_wiring_tcpserver.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/wiring/src/spark_wiring_tcpserver.cpp b/wiring/src/spark_wiring_tcpserver.cpp index e719e00536..0c7fbd9ace 100644 --- a/wiring/src/spark_wiring_tcpserver.cpp +++ b/wiring/src/spark_wiring_tcpserver.cpp @@ -26,9 +26,12 @@ #include "spark_wiring_tcpclient.h" #include "spark_wiring_tcpserver.h" #include "spark_wiring_network.h" +#include "spark_wiring_thread.h" using namespace spark; +static TCPClient* s_invalid_client = NULL; + class TCPServerClient : public TCPClient { @@ -48,7 +51,11 @@ class TCPServerClient : public TCPClient TCPServer::TCPServer(uint16_t port, network_interface_t nif) : _port(port), _nif(nif), _sock(socket_handle_invalid()), _client(socket_handle_invalid()) { - + SINGLE_THREADED_BLOCK() { + if (!s_invalid_client) { + s_invalid_client = new TCPClient(socket_handle_invalid()); + } + } } bool TCPServer::begin() @@ -90,7 +97,7 @@ TCPClient TCPServer::available() if((!Network.from(_nif).ready()) || (_sock == SOCKET_INVALID)) { _sock = SOCKET_INVALID; - _client = TCPClient(SOCKET_INVALID); + _client = *s_invalid_client; return _client; } @@ -98,7 +105,7 @@ TCPClient TCPServer::available() if (!socket_handle_valid(sock)) { - _client = TCPClient(SOCKET_INVALID); + _client = *s_invalid_client; } else { From 07787f41c55b72a4c6212758f6d92fbfa0c5787a Mon Sep 17 00:00:00 2001 From: Andrey Tolstoy Date: Tue, 20 Jun 2017 16:50:24 +0700 Subject: [PATCH 4/5] Updated Supplicant_BESL.a. See spark/photon-wiced#14 --- hal/src/photon/lib/Supplicant_BESL.a | Bin 778642 -> 778642 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/hal/src/photon/lib/Supplicant_BESL.a b/hal/src/photon/lib/Supplicant_BESL.a index 110cc5f15dabbf722c26b015ea29d1195a5de219..8db78b1966fefa4e2dfc2c0b10ad86122a8e6457 100644 GIT binary patch delta 285 zcmbPqSbx%C{RuLxmL`@K<{K3!O0t1iW(Ly_)G~=}J||i66e3^<7HCfTzdh+cV?-TP zupOvs`+-{KzP-q*+Ow=#wr5$hUP*_EF1W=dzCC&|tG5J91Z4d7L%Uf+M36+pw;Nwz zO=kx?#;kq*b=K|kud{uRhNv)_z95Z5eEVi&_FP6r=Ix56?3^H4+l)Py8Ax|pvR9Ze z8gFOHV^3!W(piP<@7RHKWF@=fTbPMy9NXuoakgtgOf;Fk;5w^#yYocO?amXq!aZTC NZgFj&e~bGw8vqDXT}c1{ delta 283 zcmbPqSbx%C{RuLx1_mZ3mKzl(N`hGy)A!dhiEln9S@Dz&Bx7MT{Xi{~SaZ_ Date: Tue, 20 Jun 2017 20:24:32 +0200 Subject: [PATCH 5/5] Add USE_THREADING option to app/tcp_server test --- user/tests/app/tcp_server/README.md | 2 +- user/tests/app/tcp_server/server.cpp | 7 ++++++- user/tests/app/tcp_server/test.mk | 7 +++++++ 3 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 user/tests/app/tcp_server/test.mk diff --git a/user/tests/app/tcp_server/README.md b/user/tests/app/tcp_server/README.md index cadf3b393e..37068da3c1 100644 --- a/user/tests/app/tcp_server/README.md +++ b/user/tests/app/tcp_server/README.md @@ -1,4 +1,4 @@ -Flash server application to the device: +Flash server application to the device (add USE_THREADING=y option to enable threading): $ cd ~/firmware/modules $ make -s all program-dfu PLATFORM=photon TEST=app/tcp_server diff --git a/user/tests/app/tcp_server/server.cpp b/user/tests/app/tcp_server/server.cpp index f96b5eeb29..a213f4554e 100644 --- a/user/tests/app/tcp_server/server.cpp +++ b/user/tests/app/tcp_server/server.cpp @@ -2,6 +2,10 @@ SYSTEM_MODE(MANUAL) +#if USE_THREADING == 1 +SYSTEM_THREAD(ENABLED) +#endif + // Assertion macro to use in Errorable classes #define CHECK(_expr) \ do { \ @@ -284,7 +288,8 @@ class NetworkManager: public Errorable { } addr_ = IPAddress(); WiFi.connect(); - if (!WiFi.ready()) { + if (!waitFor(WiFi.ready, 10000)) { + WiFi.disconnect(); setError("Unable to connect to network"); return false; } diff --git a/user/tests/app/tcp_server/test.mk b/user/tests/app/tcp_server/test.mk new file mode 100644 index 0000000000..e3f0383877 --- /dev/null +++ b/user/tests/app/tcp_server/test.mk @@ -0,0 +1,7 @@ +ifeq ("${USE_THREADING}","y") +USE_THREADING_VALUE=1 +else +USE_THREADING_VALUE=0 +endif + +CFLAGS += -DUSE_THREADING=${USE_THREADING_VALUE}