Use LWIP_TCPIP_CORE_LOCKING for lwip API #99
+100
−20
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,7 @@ extern "C" { | |
|
|
||
| // https://github.com/espressif/arduino-esp32/issues/10526 | ||
| namespace { | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| struct tcp_core_guard { | ||
| bool do_lock; | ||
| inline tcp_core_guard() : do_lock(!sys_thread_tcpip(LWIP_CORE_LOCK_QUERY_HOLDER)) { | ||
|
|
@@ -390,8 +390,7 @@ static bool _start_async_task() { | |
|
|
||
| /* | ||
| * LwIP Callbacks | ||
| */ | ||
| static void _bind_tcp_callbacks(tcp_pcb *pcb, AsyncClient *client) { | ||
| tcp_arg(pcb, client); | ||
| tcp_recv(pcb, &AsyncTCP_detail::tcp_recv); | ||
|
|
@@ -537,8 +536,7 @@ static void _tcp_dns_found(const char *name, ip_addr_t *ipaddr, void *arg) { | |
|
|
||
| /* | ||
| * TCP/IP API Calls | ||
| */ | ||
| #include "lwip/priv/tcpip_priv.h" | ||
|
|
||
| typedef struct { | ||
|
|
@@ -566,7 +564,8 @@ typedef struct { | |
| }; | ||
| } tcp_api_call_t; | ||
|
|
||
| // context switching callback | ||
| [[maybe_unused]] static err_t _tcp_output_api(struct tcpip_api_call_data *api_call_msg) { | ||
| tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg; | ||
| msg->err = ERR_CONN; | ||
| if (*msg->pcb) { | ||
|
|
@@ -575,17 +574,23 @@ static err_t _tcp_output_api(struct tcpip_api_call_data *api_call_msg) { | |
| return msg->err; | ||
| } | ||
|
|
||
| inline esp_err_t _tcp_output(tcp_pcb **pcb) { | ||
| if (!pcb || !*pcb) { | ||
| return ERR_CONN; | ||
| } | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_core_guard tcg; | ||
| return tcp_output(*pcb); | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = pcb; | ||
| tcpip_api_call(_tcp_output_api, (struct tcpip_api_call_data *)&msg); | ||
| return msg.err; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
|
Comment on lines
+581
to
+589
Member
There was a problem hiding this comment. @vortigont @willmmiles : do you think we could go even further with this cleanup and only use LWIP_TCPIP_CORE_LOCKING and force it if not defined, which means we even remove the macros and always use the guard ? and remove the usage of tcpip_api_call ? According to lwip doc, my understanding is that we have the choice... What I do not know if what is happening if we force locking and do not go through the context switching API, and another lib which is using lwip also does not use global locking but instead go through tcpip_api_call. Locking would probably still work on Arduino 2 I suppose (I could test) ?
Author
There was a problem hiding this comment. for Arduino lwip is pre-build, so I do not thinks we could enforce it for older core, but need to check, yes.
Member
There was a problem hiding this comment.
yes that was my point (pre-build)... LWIP_TCPIP_CORE_LOCKING was not used before so wondering what is happening if we force it whatever the platform, but we have arduino 2 internals (like sntp for ntp time or asyncudp lib) using tcpip_api_call. AsyncTCP is also used with Libretiny also. There was a problem hiding this comment. Defining LWIP_TCPIP_CORE_LOCKING affects compiling LwIP's own sources. We have to use whatever setting it was compiled with -- it's not a decision made on the outside.
Member
There was a problem hiding this comment. Thanks for clarifying that up for me :-) |
||
| } | ||
|
|
||
| // context switching callback | ||
| [[maybe_unused]] static err_t _tcp_write_api(struct tcpip_api_call_data *api_call_msg) { | ||
| tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg; | ||
| msg->err = ERR_CONN; | ||
| if (*msg->pcb) { | ||
|
|
@@ -594,20 +599,27 @@ static err_t _tcp_write_api(struct tcpip_api_call_data *api_call_msg) { | |
| return msg->err; | ||
| } | ||
|
|
||
|
|
||
| inline esp_err_t _tcp_write(tcp_pcb **pcb, const char *data, size_t size, uint8_t apiflags) { | ||
| if (!pcb || !*pcb) { | ||
| return ERR_CONN; | ||
| } | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_core_guard tcg; | ||
| return tcp_write(*pcb, data, size, apiflags); | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = pcb; | ||
| msg.write.data = data; | ||
| msg.write.size = size; | ||
| msg.write.apiflags = apiflags; | ||
| tcpip_api_call(_tcp_write_api, (struct tcpip_api_call_data *)&msg); | ||
| return msg.err; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| } | ||
|
|
||
| // context switching callback | ||
| [[maybe_unused]] static err_t _tcp_recved_api(struct tcpip_api_call_data *api_call_msg) { | ||
| tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg; | ||
| msg->err = ERR_CONN; | ||
| if (*msg->pcb) { | ||
|
|
@@ -617,15 +629,22 @@ static err_t _tcp_recved_api(struct tcpip_api_call_data *api_call_msg) { | |
| return msg->err; | ||
| } | ||
|
|
||
|
|
||
| inline esp_err_t _tcp_recved(tcp_pcb **pcb, size_t len) { | ||
| if (!pcb || !*pcb) { | ||
| return ERR_CONN; | ||
| } | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_core_guard tcg; | ||
| tcp_recved(*pcb, len); | ||
| return ERR_OK; | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = pcb; | ||
| msg.received = len; | ||
| tcpip_api_call(_tcp_recved_api, (struct tcpip_api_call_data *)&msg); | ||
| return msg.err; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| } | ||
|
|
||
| static err_t _tcp_close_api(struct tcpip_api_call_data *api_call_msg) { | ||
|
|
@@ -645,6 +664,7 @@ static err_t _tcp_close_api(struct tcpip_api_call_data *api_call_msg) { | |
| if (tcp_close(pcb) != ERR_OK) { | ||
| // We do not permit failure here: abandon the pcb anyways. | ||
| tcp_abort(pcb); | ||
| msg->err = ERR_ABRT; // if we called tcp_abort here, then we must return ERR_ABRT | ||
| } | ||
| msg->err = ERR_OK; | ||
| *msg->pcb = nullptr; // PCB is now the property of LwIP | ||
|
|
@@ -657,18 +677,38 @@ static err_t _tcp_close_api(struct tcpip_api_call_data *api_call_msg) { | |
| return msg->err; | ||
| } | ||
|
|
||
| inline esp_err_t _tcp_close(tcp_pcb **pcb, AsyncClient *client) { | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| if (pcb && *pcb) { | ||
| esp_err_t err{ERR_OK}; | ||
| tcp_core_guard tcg; | ||
| _reset_tcp_callbacks(*pcb, client); | ||
| if (tcp_close(*pcb) != ERR_OK) { | ||
| // We do not permit failure here: abandon the pcb anyways. | ||
| tcp_abort(*pcb); | ||
| err = ERR_ABRT; // if we called tcp_abort here, then we must return ERR_ABRT | ||
| } | ||
| *pcb = nullptr; // PCB is now the property of LwIP | ||
| return err; | ||
| } else { | ||
| // Ensure there is not an error event queued for this client | ||
| return _remove_events_for_client(client) ? ERR_OK : ERR_CONN; | ||
| } | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = pcb; | ||
| msg.close = client; | ||
| tcpip_api_call(_tcp_close_api, (struct tcpip_api_call_data *)&msg); | ||
| return msg.err; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| } | ||
|
|
||
| // context switching callback | ||
| [[maybe_unused]] static err_t _tcp_abort_api(struct tcpip_api_call_data *api_call_msg) { | ||
| tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg; | ||
| if (*msg->pcb) { | ||
| // Like close(), we must ensure that the queue is cleared | ||
| _reset_tcp_callbacks(*(msg->pcb), msg->close); | ||
| tcp_abort(*msg->pcb); | ||
| *msg->pcb = nullptr; // PCB is now the property of LwIP | ||
| msg->err = ERR_ABRT; | ||
|
|
@@ -678,78 +718,118 @@ static err_t _tcp_abort_api(struct tcpip_api_call_data *api_call_msg) { | |
| return msg->err; | ||
| } | ||
|
|
||
|
|
||
| inline esp_err_t _tcp_abort(tcp_pcb **pcb, AsyncClient *client) { | ||
| if (!pcb || !*pcb) { | ||
| return ERR_CONN; | ||
| } | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_core_guard tcg; | ||
| // Like close(), we must ensure that the queue is cleared | ||
| _reset_tcp_callbacks(*pcb, client); | ||
| tcp_abort(*pcb); | ||
| *pcb = nullptr; // PCB is now the property of LwIP | ||
vortigont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return ERR_ABRT; | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = pcb; | ||
| msg.close = client; | ||
| tcpip_api_call(_tcp_abort_api, (struct tcpip_api_call_data *)&msg); | ||
| return msg.err; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| } | ||
|
|
||
| // context switching callback | ||
| [[maybe_unused]] static err_t _tcp_connect_api(struct tcpip_api_call_data *api_call_msg) { | ||
| tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg; | ||
| msg->err = tcp_connect(*msg->pcb, msg->connect.addr, msg->connect.port, msg->connect.cb); | ||
| return msg->err; | ||
| } | ||
|
|
||
|
|
||
| inline esp_err_t _tcp_connect(tcp_pcb *pcb, ip_addr_t *addr, uint16_t port, tcp_connected_fn cb) { | ||
| if (!pcb) { | ||
| return ESP_FAIL; | ||
| } | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_core_guard tcg; | ||
| return tcp_connect(pcb, addr, port, cb); | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = &pcb; // cannot be invalidated by LwIP at this point | ||
| msg.connect.addr = addr; | ||
| msg.connect.port = port; | ||
| msg.connect.cb = cb; | ||
| tcpip_api_call(_tcp_connect_api, (struct tcpip_api_call_data *)&msg); | ||
| return msg.err; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| } | ||
|
|
||
| // context switching callback | ||
| [[maybe_unused]] static err_t _tcp_bind_api(struct tcpip_api_call_data *api_call_msg) { | ||
| tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg; | ||
| tcp_pcb *pcb = *msg->pcb; | ||
| msg->err = tcp_bind(pcb, msg->bind.addr, msg->bind.port); | ||
| if (msg->err != ERR_OK) { | ||
| // Close the pcb on behalf of the server without an extra round-trip through the LwIP lock | ||
| if (tcp_close(pcb) != ERR_OK) { | ||
| tcp_abort(pcb); | ||
| msg->err = ERR_ABRT; // if we called tcp_abort here, then we must return ERR_ABRT | ||
| } | ||
| *msg->pcb = nullptr; // PCB is now owned by LwIP | ||
| } | ||
| return msg->err; | ||
| } | ||
|
|
||
| inline esp_err_t _tcp_bind(tcp_pcb **pcb, ip_addr_t *addr, uint16_t port) { | ||
| if (!pcb || !*pcb) { | ||
| return ESP_FAIL; | ||
| } | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_core_guard tcg; | ||
| auto err = tcp_bind(*pcb, addr, port); | ||
| if (err != ERR_OK) { | ||
| // Close the pcb on behalf of the server without an extra round-trip through the LwIP lock | ||
| if (tcp_close(*pcb) != ERR_OK) { | ||
| tcp_abort(*pcb); | ||
| err = ERR_ABRT; // if we called tcp_abort here, then we must return ERR_ABRT | ||
| } | ||
| *pcb = nullptr; // PCB is now owned by LwIP | ||
| } | ||
| return err; | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = pcb; | ||
| msg.bind.addr = addr; | ||
| msg.bind.port = port; | ||
| tcpip_api_call(_tcp_bind_api, (struct tcpip_api_call_data *)&msg); | ||
| return msg.err; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| } | ||
|
|
||
| // context switching callback | ||
| [[maybe_unused]] static err_t _tcp_listen_api(struct tcpip_api_call_data *api_call_msg) { | ||
| tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg; | ||
| msg->err = 0; | ||
| *msg->pcb = tcp_listen_with_backlog(*msg->pcb, msg->backlog); | ||
| return msg->err; | ||
| } | ||
|
|
||
|
|
||
| static tcp_pcb *_tcp_listen_with_backlog(tcp_pcb *pcb, uint8_t backlog) { | ||
| if (!pcb) { | ||
| return NULL; | ||
| } | ||
| #if defined CONFIG_LWIP_TCPIP_CORE_LOCKING && CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_core_guard tcg; | ||
| return tcp_listen_with_backlog(pcb, backlog ? backlog : 0xFF); | ||
| #else // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| tcp_api_call_t msg; | ||
| msg.pcb = &pcb; | ||
| msg.backlog = backlog ? backlog : 0xFF; | ||
| tcpip_api_call(_tcp_listen_api, (struct tcpip_api_call_data *)&msg); | ||
| return pcb; | ||
| #endif // CONFIG_LWIP_TCPIP_CORE_LOCKING | ||
| } | ||
|
|
||
| /* | ||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it to avoid using that if -DCONFIG_LWIP_TCPIP_CORE_LOCKING=0 is set ?
If so, should it be instead more clear and be:
#if CONFIG_LWIP_TCPIP_CORE_LOCKING == 1?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found any guidance if such a case could be used when
CONFIG_LWIP_TCPIP_CORE_LOCKING=0,in IDF examples I found only when it's defined as
1, but checks are exist to ensure it'strue, quite confusing.So, yes, it's to check it that it's not just defined, but defined as
true. You think== 1would be more appropriate here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I would stick with what Arduino Core and ESP-IDF do. They would fail way before than us. And in their code they only use:
#ifdef CONFIG_LWIP_TCPIP_CORE_LOCKINGand in esp-idf:#if CONFIG_LWIP_TCPIP_CORE_LOCKINGSo maybe just
#if CONFIG_LWIP_TCPIP_CORE_LOCKING?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
#if CONFIG_LWIP_TCPIP_CORE_LOCKINGwill fail on non esp32 platforms, thisCONFIG_part belongs to IDF.Maybe check
LWIP_TCPIP_CORE_LOCKINGitself? I do not have Libretiny's at hand, but will try to check what's there in those SDK's. Any clue?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neither I...
So maybe it is better to leave it like you did to handle all the combinations...
Or we do like arduino:
ifdef CONFIG_LWIP_TCPIP_CORE_LOCKING, but this opens the door to unexpected behavior like you mentioned withCONFIG_LWIP_TCPIP_CORE_LOCKING=0Probably better to stick with what you did...