Am 16. Juni 2021 20:15:02 MESZ schrieb "Rémy DZIEMIASZKO" <remy.dziemias...@smile.fr>: >Le mer. 16 juin 2021 à 15:24, Bas Prins <bas.pri...@gmail.com> a écrit >: >> >> Hi Remy, >> >> Thanks for your answer. >> >> It feels a bit awkward this way though. Do you know any reason why >user code should be bothered with reloading the timeout ticks? Why >doesn't http_client deal with this when it gets notified about a new >packet on "httpc_tcp_recv" ? >> > >I agree with you that http_client timeout management shall be improved:
Definitely seems so. Would anyone please file a bug report for this? Or even better, a patch fixing this? The timeout should be reset when receiving a chunk but should probably be overridable at compile time or runtime, too. Regards, Simon >1) By reloading the timer after each chunk reception (what you have >implemented below) >2) By giving the possibility for the application to choose the timeout >value(s). From simple to more complex possible enhancement: >2.a) By turning HTTPC_POLL_TIMEOUT into a lwip option instead of a >fixed #define. >2.b) By passing an additional parameter to httpc_get_file to configure >for each connection a specific timeout value (because timeout value >may depend on the server behaviour or network performance). >2.c) By passing various parameters for various timeouts: timeout for >the initial connection to the server, timeout between chunk >receptions, timeout of the complete http request > >> And no, the opaque type is still as is in the latest and greatest >version of lwip. Clearly indicating "I shouldn't be using their >httpc_state_t, right? >> >> Would this make sense to you: >> >> /** http client tcp recv callback */ >> static err_t >> httpc_tcp_recv(void *arg, struct altcp_pcb *pcb, struct pbuf *p, >err_t r) >> { >> httpc_state_t* req = (httpc_state_t*)arg; >> LWIP_UNUSED_ARG(r); >> ... >> ... >> ... >> if ((p != NULL) && (req->parse_state == HTTPC_PARSE_RX_DATA)) { >> req->rx_content_len += p->tot_len; >> >> >> // ----- RESET TIMER TICKS HERE ------ >> req->timeout_ticks = HTTPC_POLL_TIMEOUT; >> >> >> if (req->recv_fn != NULL) { >> /* directly return here: the connection migth already be >aborted from the callback! */ >> return req->recv_fn(req->callback_arg, pcb, p, r); >> } else { >> altcp_recved(pcb, p->tot_len); >> pbuf_free(p); >> } >> } >> return ERR_OK; >> } >> >> This solves my problem, and this way user code is not bothered with >something I think it shouldn't have to bother with. And the >httpc_state_t can remain opaque as well. >> >> Best regards, bas >> >> >> >> Op wo 16 jun. 2021 om 14:15 schreef Rémy DZIEMIASZKO ><remy.dziemias...@smile.fr>: >>> >>> Hello, >>> >>> When you call 'httpc_get_file' the parameter 'connection' gives you >a >>> handle on the http_state used for that connection >>> then you can passes this handle to 'httpc_get_file' via the >parameter >>> 'void * callback_arg' >>> then your received function will get the handle via the parameter >'void * arg' >>> then in the received function you can do (http_state_t >>> *)arg->timeout_ticks = MY_RELOAD_VALUE; >>> >>> http_state_t * foo; >>> httpc_get_file(ip, port, uri, settings, my_recv_fn, foo, &foo) >>> >>> err_t my_recv_fn(void * arg, ...) >>> { >>> (http_state_t *)arg->timeout_ticks = MY_RELOAD_VALUE; >>> } >>> >>> You might have a compilation issue because 'http_state_t' is >normally >>> an opaque type for the application then the member 'timeout_ticks' >is >>> not visible from the application. >>> In a past project I solved that by exporting the definition of >>> 'http_state_t' but maybe this is already fixed in the last release >of >>> lwip. >>> >>> Regards >>> Rémy >>> >>> Le mer. 16 juin 2021 à 13:24, Bas Prins <bas.pri...@gmail.com> a >écrit : >>> > >>> > Dear , >>> > >>> > I don't think I am able to reset the timer. The 'arg' passed in >>> > >>> > err_t rec_fn(void *arg, struct altcp_pcb *conn, struct pbuf *p, >err_t err) >>> > >>> > is not of type httpc_state_t. The rec_fn callback is called here: >>> > >>> > static err_t >>> > httpc_tcp_recv(void *arg, struct altcp_pcb *pcb, struct pbuf *p, >err_t r) >>> > ... >>> > ... >>> > ... >>> > if (req->recv_fn != NULL) { >>> > /* directly return here: the connection migth already be >aborted from the callback! */ >>> > return req->recv_fn(req->callback_arg, pcb, p, r); >>> > } >>> > >>> > So what's the deal with this function >>> > >>> > /** http client tcp poll callback */ >>> > static err_t >>> > httpc_tcp_poll(void *arg, struct altcp_pcb *pcb) >>> > { >>> > /* implement timeout */ >>> > httpc_state_t* req = (httpc_state_t*)arg; >>> > LWIP_UNUSED_ARG(pcb); >>> > if (req != NULL) { >>> > if (req->timeout_ticks) { >>> > req->timeout_ticks--; >>> > } >>> > if (!req->timeout_ticks) { >>> > return httpc_close(req, HTTPC_RESULT_ERR_TIMEOUT, 0, >ERR_OK); >>> > } >>> > } >>> > return ERR_OK; >>> > } >>> > >>> > It only decreases the ticks counter. It never gets reset. So I am >not allowed to download files longer than 30 secs? If the download >doesn't succeed within that time, http_client closes the connection. >>> > >>> > I would expect that either the timeout counter is reset every >chunk of data which is received, or that user code (my code) would >receive a pointer to the httpc_state_t so that it could reset the >timer. >>> > >>> > Can somebody please explain where I am wrong, what am I missing, >or that indeed the current implementation of http_client is lacking ? >>> > >>> > Many thanks in advance >>> > >>> > best regards, bas >>> > >>> > >>> > >>> > Op wo 16 jun. 2021 om 09:29 schreef Bas Prins ><bas.pri...@gmail.com>: >>> >> >>> >> Hi all, >>> >> >>> >> I found the problem, which was rather easy to find once I >convinced myself I am man enough to actually read code ;-). >>> >> >>> >> But now I wonder how to solve this appropriately. The problem >seems to be, that this function decreases the ticks, until the timeout >period is consumed, and then simply closes the connection. >>> >> >>> >> /** http client tcp poll callback */ >>> >> static err_t >>> >> httpc_tcp_poll(void *arg, struct altcp_pcb *pcb) >>> >> { >>> >> /* implement timeout */ >>> >> httpc_state_t* req = (httpc_state_t*)arg; >>> >> LWIP_UNUSED_ARG(pcb); >>> >> if (req != NULL) { >>> >> if (req->timeout_ticks) { >>> >> req->timeout_ticks--; >>> >> } >>> >> if (!req->timeout_ticks) { >>> >> return httpc_close(req, HTTPC_RESULT_ERR_TIMEOUT, 0, >ERR_OK); >>> >> } >>> >> } >>> >> return ERR_OK; >>> >> } >>> >> >>> >> So I wonder, who should be responsible for resetting the timeout >counter? My code? The definition of the timeout_ticks seems to only >visible for to http_client.c. >>> >> >>> >> If I simply comment the line which decreases the counter the >download works. >>> >> >>> >> Am I still missing the obvious here? >>> >> >>> >> Many thanks. >>> >> >>> >> best regards, bas >>> >> >>> >> Op ma 14 jun. 2021 om 22:05 schreef Bas Prins ><bas.pri...@gmail.com>: >>> >>> >>> >>> Hi Simon, all, >>> >>> >>> >>> Fixed the problems I had which led to missing a byte >occasionally. >>> >>> >>> >>> Still unable to download a file using http_client.h. I did learn >a small part. I log all arguments of >>> >>> >>> >>> void result_fn(void *arg, httpc_result_t httpc_result, u32_t >rx_content_len, u32_t srv_res, err_t err) >>> >>> >>> >>> Which produces this log line: >>> >>> 2021-06-14 21:31:52,041 [DEBUG][thread: 5][UartReader] DOWNLOAD >finished: httpc_result=5, rx_content_len=5019, srv_res=0, error=0 >>> >>> >>> >>> /** Connection timed out (server didn't respond in time) */ >>> >>> HTTPC_RESULT_ERR_TIMEOUT = 5, >>> >>> >>> >>> I attached the pcap file of tcpdump. >>> >>> - It contains a couple of attempts to download the file >(513KB.zip). >>> >>> - It shows TCP window full warnings. I don't care too much about >that now. I'll deal with that once I have a stable download going. >>> >>> - It shows the result of the timeout where my STM board closes >the connection. >>> >>> >>> >>> I also attached my lwipopts.h and the loggings produced by my >app (including lwip ppp logging). >>> >>> >>> >>> Can you help me understand what is going wrong? >>> >>> Why is LWIP complaining about the server not responding in time, >while it seems that the server is doing just fine? >>> >>> >>> >>> PS: obviously I tested the download on my desktop, I am able to >download the file repeatedly. >>> >>> >>> >>> Many thanks in advance! >>> >>> >>> >>> Best regards, Bas >>> >>> >>> >>> >>> >>> Op vr 11 jun. 2021 om 07:15 schreef goldsi...@gmx.de ><goldsi...@gmx.de>: >>> >>>> >>> >>>> Am 10.06.2021 um 17:27 schrieb Bas Prins: >>> >>>> > Hi Simon, >>> >>>> > >>> >>>> > Thanks, that's a very likely explanation already. I just >added the >>> >>>> > pbuf_free(p) but run into an assert occasionally. >>> >>>> >>> >>>> You might have to check for 'p != NULL' just like in tcp recv >callback >>> >>>> functions. >>> >>>> >>> >>>> Regards, >>> >>>> Simon >>> >>>> >>> >>>> _______________________________________________ >>> >>>> lwip-users mailing list >>> >>>> lwip-users@nongnu.org >>> >>>> https://lists.nongnu.org/mailman/listinfo/lwip-users >>> > >>> > _______________________________________________ >>> > lwip-users mailing list >>> > lwip-users@nongnu.org >>> > https://lists.nongnu.org/mailman/listinfo/lwip-users >>> >>> _______________________________________________ >>> lwip-users mailing list >>> lwip-users@nongnu.org >>> https://lists.nongnu.org/mailman/listinfo/lwip-users >> >> _______________________________________________ >> lwip-users mailing list >> lwip-users@nongnu.org >> https://lists.nongnu.org/mailman/listinfo/lwip-users > >_______________________________________________ >lwip-users mailing list >lwip-users@nongnu.org >https://lists.nongnu.org/mailman/listinfo/lwip-users _______________________________________________ lwip-users mailing list lwip-users@nongnu.org https://lists.nongnu.org/mailman/listinfo/lwip-users