Skip to content
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

tests: coap_client: optimize/reduce sleep time #79369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fgervais
Copy link
Contributor

@fgervais fgervais commented Oct 3, 2024

The goal of this commit is to reduce the overall test time by optimizing the time spent sleeping as suggested here: #78904 (comment).

However while doing so, a few glitches became apparent and those have been fixed as well.

  1. The way the ZSOCK_POLLIN event is managed has been modified

In most tests, the event would stick for the whole test and this would sometimes results in the client receiving more data than intended which in turn would results in various unexpected warnings and errors.

The management of the ZSOCK_POLLIN event has now been mostly moved to the send/receive overrides which better represent how things would happen outside of the test scenario.

For example, when sending data, if this send would normally result in receiving some response, the send function sets the ZSOCK_POLLIN event. Then when receiving, we clear the event as the data has been received.

There are a few exceptions to this for cases where we need to simulate some specific abnormal behavior.

  1. The messages_needing_response queue now includes a valid bit

The test manages a queue of messages id to be able to respond to the correct id in the receive hooks.

It was built in a way that the id 0 would be treated as invalid although it is a valid id.
In practice, it would not be an issue in most cases however, it would result in an unexpected behavior if the test_multiple_requests test would be run first.

To fix this issue in the simplest way, the queue has been changed to uint32_t which gives us 16 extra bits for the management of the queue, 1 of which is used to tell if the entry is valid or not.

@jukkar
Copy link
Member

jukkar commented Oct 4, 2024

Just a tiny compliance issue

 C99_COMMENTS: do not use C99 // comments
File:tests/net/lib/coap_client/src/main.c
Line:392

@fgervais
Copy link
Contributor Author

fgervais commented Oct 4, 2024

Just a tiny compliance issue

 C99_COMMENTS: do not use C99 // comments
File:tests/net/lib/coap_client/src/main.c
Line:392

Ah yes that line. I kind of wanted to do away with doing a runtime float and cast calculation while instructing others what I'm doing.

What to you think if I go with this?

k_sleep(K_MSEC(CONFIG_COAP_INIT_ACK_TIMEOUT_MS + CONFIG_COAP_INIT_ACK_TIMEOUT_MS/2));

The compiler should figure that it can stay all int and do var + var >> 1. Even the precompiler might do it but I'm not sure.

But it's a bit unattractive.

The goal of this commit is to reduce the overall test time by optimizing
the time spent sleeping.

However while doing so, a few glitches became apparent and those have
been fixed as well.

1. The way the ZSOCK_POLLIN event is managed has been modified

In most tests, the event would stick for the whole test and this would
sometimes results in the client receiving more data than intended which
in turn would results in various unexpected warnings and errors.

The management of the ZSOCK_POLLIN event has now been mostly moved to
the send/receive overrides which better represent how things would
happen outside of the test scenario.

For example, when sending data, if this send would normally result in
receiving some response, the send function sets the ZSOCK_POLLIN event.
Then when receiving, we clear the event as the data has been received.

There are a few exceptions to this for cases where we need to simulate
some specific abnormal behavior.

2. The `messages_needing_response` queue now includes a valid bit

The test manages a queue of messages id to be able to respond to the
correct id in the receive hooks.

It was built in a way that the id 0 would be treated as invalid
although it is a valid id.
In practice, it would not be an issue in most cases however, it would
result in an unexpected behavior if the `test_multiple_requests` test
would be run first.

To fix this issue in the simplest way, the queue has been changed to
uint32_t which gives us 16 extra bits for the management of the queue,
1 of which is used to tell if the entry is valid or not.

Signed-off-by: Francois Gervais <[email protected]>
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the patch 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants