examples/lwip_ipv4: add example for LWIP IPv4 client/server#21519
examples/lwip_ipv4: add example for LWIP IPv4 client/server#21519maribu merged 5 commits intoRIOT-OS:masterfrom
Conversation
30edfe6 to
b6ba8d4
Compare
|
I try to generate @crasbe could you add label |
|
I'll run it on my work computer tomorrow, then you can address the suggestions without triggering a full rebuild on every fixup. |
|
This is the result for BOARD_INSUFFICIENT_MEMORY := \
bluepill-stm32f030c8 \
i-nucleo-lrwan1 \
nucleo-c031c6 \
nucleo-f030r8 \
nucleo-f031k6 \
nucleo-f042k6 \
nucleo-l011k4 \
nucleo-l031k6 \
nucleo-l053r8 \
samd10-xmini \
slstk3400a \
stk3200 \
stm32f030f4-demo \
stm32f0discovery \
stm32g0316-disco \
stm32l0538-disco \
weact-g030f6 \
# |
|
@crasbe Thanks for |
829cea9 to
867808b
Compare
| mutex_lock(&server_mutex); | ||
| server_running = 0; | ||
| mutex_unlock(&server_mutex); |
There was a problem hiding this comment.
What do you need that for?
There was a problem hiding this comment.
My idea is that if due to some errors this thread ends - we could start server once again.
I tested this situation with similar code after sock_udp_recv function (lines 98-105). When RIOT receives UDP datagram longer than 64 bytes (buffer size) it ends with error, but I could restart server and later everything works well.
There was a problem hiding this comment.
But why the need for a mutex?
There was a problem hiding this comment.
This variable is changed by other thread (CLI thread in server command). Also I leave this in the example as example of synchronization. Yes, I known that probability of hazards or race is minimal.
I could leave this, remove mutex (in my opinion all should works fine, only in very rare case user have to issue server command once again, after error message that thread is started) or rewrite code to use _Atomic and atomic_compare_exchange_strong.
ea3ad62 to
9a20829
Compare
|
All remarks from review fixed. |
|
Can me move this PR a little forward? |
| } | ||
|
|
||
| if (data_len != (size_t)sock_udp_send(&sock, argv[3], data_len, NULL)) { | ||
| printf("Error sock_udp_send!\n"); |
There was a problem hiding this comment.
You likely want to print the error.
There was a problem hiding this comment.
Yes I want to print error.
Your comment concerns wrong condition? You suggest add only condition for <0?
There was a problem hiding this comment.
sock_udp_send() gives you an error code that can tell the user why the send failed.
There was a problem hiding this comment.
Ok. Thanks for hint!
There was a problem hiding this comment.
I add printing error code to error message. Is that OK?
| mutex_lock(&server_mutex); | ||
| server_running = 0; | ||
| mutex_unlock(&server_mutex); |
There was a problem hiding this comment.
But why the need for a mutex?
85936e5 to
eeb5403
Compare
|
Can me move this PR little forward? |
|
@benpicco I fix most of your remarks and have questions to one or too. Could you review the code once again and could we move this PR forward? |
|
Beginning of new week ... good time to finish this PR :) @benpicco - can be move it little bit forward? |
376fdf3 to
5c2ae13
Compare
|
Huh how did you get there? |
krzysztof-cabaj
left a comment
There was a problem hiding this comment.
This should show all my pending comments.
| mutex_lock(&server_mutex); | ||
| server_running = 0; | ||
| mutex_unlock(&server_mutex); |
There was a problem hiding this comment.
This variable is changed by other thread (CLI thread in server command). Also I leave this in the example as example of synchronization. Yes, I known that probability of hazards or race is minimal.
I could leave this, remove mutex (in my opinion all should works fine, only in very rare case user have to issue server command once again, after error message that thread is started) or rewrite code to use _Atomic and atomic_compare_exchange_strong.
| } | ||
|
|
||
| if (data_len != (size_t)sock_udp_send(&sock, argv[3], data_len, NULL)) { | ||
| printf("Error sock_udp_send!\n"); |
There was a problem hiding this comment.
Ok. Thanks for hint!
| } | ||
|
|
||
| if (data_len != (size_t)sock_udp_send(&sock, argv[3], data_len, NULL)) { | ||
| printf("Error sock_udp_send!\n"); |
There was a problem hiding this comment.
I add printing error code to error message. Is that OK?
|
@benpicco thanks for spotting the problem and showing solution. I don't now why some of my comments are not send - I'm almost certain that I post all comments in this same manner. I see that some of the comments are pending from June :( I hope this allows us finalizing this PR without problems - and answers the question why you do not see my comments. |
f836f51 to
ab23020
Compare
|
@benpicco I try to fix all comments. If I missed something please let me known. I leave two issues, which are not changed:
|
ab23020 to
cfb07ac
Compare
|
I simplify the example. Server thread is started automatically, and cannot be stopped. |
benpicco
left a comment
There was a problem hiding this comment.
Thank you, that's much better.
An example shouldn't come with too many bells and whistles that will just confuse people.
cfb07ac to
33365ca
Compare
|
Murdock complains about small ROM and RAM for I add this board to the |
|
Could we add this PR to merge queue once again ... |
|
Thx for adding this example ❤️ |
Contribution description
This PR adds new example which shows how to implement LWIP IPv4 client and server.
Moreover, this example can be used in CI to check, if new code do not break LWIP IPv4.
In the previous few months at least two problems with LWIP IPv4 are detected manually,
and they should be detected by CI - see PR #21316 or PR #21342.
Testing procedure
Check
clientandservercommands as described in theREADME.md.Issues/PRs references
None