Skip to content

tests/socket_zep: fix automatic test#17432

Merged
benpicco merged 2 commits into
RIOT-OS:masterfrom
benpicco:tests/socket_zep-fix
Jan 3, 2022
Merged

tests/socket_zep: fix automatic test#17432
benpicco merged 2 commits into
RIOT-OS:masterfrom
benpicco:tests/socket_zep-fix

Conversation

@benpicco

Copy link
Copy Markdown
Contributor

Contribution description

The automatic test for socket ZEP was broken because it did not run on Murdock. This PR attempts to fix this:

  • use the API as expected by the Radio HAL
  • supply a EUI-64 so the native instance always uses the same address
  • switch to IPv4 as Murdock does not support IPv6. This allows to run the test in CI

Testing procedure

make -C tests/socket_zep test

should now work again.

Issues/PRs references

fixes #16946 (comment)

Murdock does not support IPv6, so use IPv4 for the automated test.
@benpicco benpicco requested a review from jia200x December 21, 2021 15:56
@github-actions github-actions Bot added the Area: tests Area: tests and testing framework label Dec 21, 2021
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Dec 21, 2021
@benpicco benpicco requested a review from kaspar030 December 21, 2021 22:52
@benpicco benpicco added this to the Release 2022.01 milestone Dec 24, 2021
@benpicco benpicco requested a review from kfessel January 3, 2022 11:22

@kfessel kfessel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread tests/socket_zep/main.c
_main_pid = thread_getpid();

test_init();
test_send__iolist_NULL();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this part of the test ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the radio HAL sending a NULL buffer will no longer result in any data being transmitted. Previously this would send a header-only packet (and this test would verify that).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe:

Elements of this list may have iolist_t::iol_size == 0 and (in this case only) iolist_t::iol_data == 0

should be tested instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes no sense. We are on layer 2 here, so for a real device this would mean no data on the bus / air.
Only with the (old) ZEP implementation this would write the ZEP header (that wraps the IEEE 802.15.4 frame).

Mind you that we are using the sub-mac now which does make assumptions about the contents being transmitted (namely them being a IEEE 802.15.4 frame) - we'd now need to add checks there, e.g in sys/net/link_layer/ieee802154/submac.c:414 where it checks if the ACK_REQ bit is set - for a case that will never happen in reality.

@kfessel kfessel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes are sound, test ran by murdock, removal of unneeded test explained -> question resolved

@benpicco benpicco merged commit 27f3261 into RIOT-OS:master Jan 3, 2022
@benpicco benpicco deleted the tests/socket_zep-fix branch January 3, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants