-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net/gnrc/sock: Implement sock_aux_local #14704
Conversation
I changed the implementation a bit (I think it is now more elegant) and used |
I rebased on master and solved merged conflict (all regarding the documentation fixes in the API, not relevant to this PR). I did however not squash |
I messed up the rebase. This time it should have worked. |
OK, only the expected failures: The ds3231 test requires hardware to be plugged in to work (and was blacklisted on the CI in #15554). And the issue with libhydrogen on clang is also preexisting. |
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.
One last thing:
Please squash. I will give the tests a final local run. |
I ran the tests with the following script on TEST_RUNS=0
for test in tests/gnrc_sock_{ip,udp}; do
RIOT_CI_BUILD=1 AUX_LOCAL=1 make -C ${test} --no-print-directory -j clean flash test || break
RIOT_CI_BUILD=1 AUX_LOCAL=0 make -C ${test} --no-print-directory -j clean flash test || break
TEST_RUNS=$((TEST_RUNS+1))
done
[ ${TEST_RUNS} -eq 2 ] && echo "success" || echo "fail" Success was printed in the end and the build sizes differed:
Non- |
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.
Static tests failed because of a missing doc.
#if !IS_USED(MODULE_SOCK_AUX_LOCAL) | ||
uint8_t this_struct_is_not_empty; | ||
#endif |
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.
Murdock wants this to be documented. Maybe just hide it from it?
#if !IS_USED(MODULE_SOCK_AUX_LOCAL) | |
uint8_t this_struct_is_not_empty; | |
#endif | |
#if !IS_USED(MODULE_SOCK_AUX_LOCAL) && !IS_ACTIVE(DOXYGEN) | |
uint8_t this_struct_is_not_empty; | |
#endif |
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'm not sure if Doxygen is so smart that it can parse IS_ACTIVE()
- it would need a C99 compliant preprocessor for this.
Maybe it is indeed the easiest to just document it.
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'm not sure if Doxygen is so smart that it can parse
IS_ACTIVE()
- it would need a C99 compliant preprocessor for this.
I think in that case it is just enough to check if it is not defined
#if !IS_USED(MODULE_SOCK_AUX_LOCAL) | |
uint8_t this_struct_is_not_empty; | |
#endif | |
#if !IS_USED(MODULE_SOCK_AUX_LOCAL) && !defined(DOXYGEN) | |
uint8_t this_struct_is_not_empty; | |
#endif |
Provide address the IP packet / UDP datagram was received on in the auxiliary data, if module sock_aux_local is used.
I extended the doc of the struct and squashed right in. See here for the diff. |
I think the revert commit can also squashed into the commit it reverts ;-). |
Good catch. Maybe I should stop blindly trusting |
It's awesome, but I never understood why |
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.
ACK. Code is sane and I provided tests as well as ran the tests on Murdock.
Thanks :-) |
Contribution description
This PR adds the implementation of the
sock_aux_local
for GNRC, that provides access to the local address via the extended SOCK API of #14703.Testing procedure
make -C tests/gnrc_sock_udp flash test
make -C tests/gnrc_sock_ip flash test
Issues/PRs references
Depends on #14703
Same as #14705, but for GNRC