kernel: msgq: adding support for k_msgq_put_front and sample code

[alexpaschoaletto]

Following the examples of FreeRTOS and RTEMS, this PR introduces a k_msgq_put_front API for sending messages to the head of the queue, in a LIFO scheme. It also adds a simple sample code that demonstrates how it works.

Due to the fact that both versions of k_msgq_put share most of the code, this contribution creates an internal put_msg_in_queue function which inserts the message at the front or back of the queue depending on the flag put_at_back passed to it. Now the implementation looks like this:

/* internal function */
int put_msg_in_queue(struct k_msgq *msgq, const void *data, k_timeout_t timeout, bool put_at_back)
{
        /*
            puts message to front or back of the queue
            depending on the value or put_at_back
        */
}

int z_impl_k_msgq_put(struct k_msgq *msgq, const void *data, k_timeout_t timeout)
{
	return put_msg_in_queue(msgq, data, timeout, true);
}

int z_impl_k_msgq_put_front(struct k_msgq *msgq, const void *data, k_timeout_t timeout)
{
	return put_msg_in_queue(msgq, data, timeout, false);
}

note: some work concerning mainly docs and ci/cd still needs updating, but the bulk of the contribution is here. I'd like to have some opinions on this contribution before taking the time to do everyhing properly.

Closes #46339

[andyross]

A design nitpick around the cut/paste feel of the change.

Also: please isolate changes to the sample to a separate commit from the semantic change in the kernel.

And while a sample can be used as a test case, it's really best to put an actual test case of this functionality into tests/kernel/msgq/msgq_api

[andyross]

Not loving the duplication of the implementation code. Ideally all this should be shared. In particular maintenance-quality-issues are at their most important in code surrounding memory copies into/out-of caller pointers (which can cross security boundaries if CONFIG_USERSPACE=y!). Might take some refactoring to make that happen, obviously.

[alexpaschoaletto]

I understand and agree, but at first I really didn't want to touch in the already existing k_msgq_put function. So should I do something like this, then?

int _copy_to_queue ( ... , bool should_copy_to_back) {
  // shared implementation goes here
}

int z_impl_k_msgq_put( ... ) {
  return _copy_to_queue( ... , true);
}

int z_impl_k_msgq_put_urgent( ... ) {
  return _copy_to_queue( ..., , false);
}

[alexpaschoaletto]

FWIW I have done just that to avoid code duplication.

[andyross]

While this is a good note for understanding, it would be even better for this to be a test that actually checked messages were received in the expected order.

[alexpaschoaletto]

I agree. For now I have refactored the sample code to have a regex to check the sample output (with the message sequence) on the tests. I intend to do the proper ZTEST thing as you suggested as well, but I'd like to settle on the implementation before that.

[TaiJuWu]

Please add reference: #46339 to PR description.
Also this feature is good to be optional and it need more test.

[TaiJuWu]

Add an additional trace log to facilitate easier debugging.

[alexpaschoaletto]

I like this idea, however I'm not certain on how to refactor this part. would this be just changing put by put_urgent (or whatever naming convention we end up deciding)? Like this:

SYS_PORT_TRACING_OBJ_FUNC_EXIT(k_msgq, put_urgent, msgq, timeout, 0);

[TaiJuWu]

You can refer #71195
I think I finished most implements at that time.

[alexpaschoaletto]

Please add reference: #46339 to PR description. Also this feature is good to be optional and it need more test.

done! not sure what you mean by "good to be optional", though. Proper tests will be done soon - I wanted some feedback on this contribution before taking the effort (if for some reason this was immediately discarded by reviewers, then it would be a waste of energy).

[TaiJuWu]

Please add reference: #46339 to PR description. Also this feature is good to be optional and it need more test.

done! not sure what you mean by "good to be optional", though. Proper tests will be done soon - I wanted some feedback on this contribution before taking the effort (if for some reason this was immediately discarded by reviewers, then it would be a waste of energy).

In my mind, the feature is not required for every developer so we can be optional and make the user to decide enabling it or not, this can reduce code size.

[TaiJuWu]

urgent or front?

[alexpaschoaletto]

in the context of the sample, the publisher thread sends normal and urgent messages. It's not related to the API naming.

[TaiJuWu]

Got it, thanks.

[peter-mitsis]

Once the minor compliance check is fixed, I expect I will be giving this a +1.

[JarmouniA]

Mostly nits

[TaiJuWu]

it is a test failure, is it excepted?

The reason is described in #92865 (comment)

/**
 * @brief Put a message to a full queue for behavior test
 *
 * @details
 * - Thread A put message to a full message queue and then sleep
 * Thread B put a new message to the queue then pending on it.
 * - Thread A get all messages from message queue and check the behavior.
 *
 * @see k_msgq_put(), k_msgq_put_front()
 */
ZTEST(msgq_api_1cpu, test_msgq_pending)
{
	uint32_t rx_data;
	int pri = k_thread_priority_get(k_current_get()) - 1;
	int ret;

	k_msgq_init(&msgq1, tbuffer, MSG_SIZE, 2);
	ret = k_sem_init(&end_sema, 0, 1);
	zassert_equal(ret, 0);

	ret = IS_ENABLED(CONFIG_TEST_MSGQ_PREPEND) ?
		k_msgq_put_front(&msgq1, &data[1], K_NO_WAIT) :
		k_msgq_put(&msgq1, &data[0], K_NO_WAIT);
	zassert_equal(ret, 0);
	ret = IS_ENABLED(CONFIG_TEST_MSGQ_PREPEND) ?
		k_msgq_put(&msgq1, &data[0], K_NO_WAIT) :
		k_msgq_put_front(&msgq1, &data[1], K_NO_WAIT);
	zassert_equal(ret, 0);

	k_tid_t tid = k_thread_create(&tdata2, tstack2, STACK_SIZE,
					prepend_full_entry, &msgq1, NULL,
					NULL, pri, 0, K_NO_WAIT);

	/* that putting thread is being blocked now */
	k_sem_take(&end_sema, K_FOREVER);
	zassert_equal(tid->base.thread_state, _THREAD_PENDING);
	ret = k_msgq_get(&msgq1, &rx_data, K_FOREVER);
	zassert_equal(ret, 0);
	zassert_equal(rx_data, data[1]);

	ret = k_msgq_get(&msgq1, &rx_data, K_FOREVER);
	zassert_equal(ret, 0);
	zassert_equal(rx_data, msg3);

	ret = k_msgq_get(&msgq1, &rx_data, K_FOREVER);
	zassert_equal(ret, 0);
	zassert_equal(rx_data, data[0]);
	k_thread_abort(tid);
}

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
14.4% Duplication on New Code

See analysis details on SonarQube Cloud

[alexpaschoaletto]

it is a test failure, is it excepted?

The reason is described in #92865 (comment)

I don't know if I understood. What exactly is to fail here?

[andyross]

Refresh +1

[TaiJuWu]

it is a test failure, is it excepted?
The reason is described in #92865 (comment)

I don't know if I understood. What exactly is to fail here?

This test covers the behavior of putting a message into the message queue when it is full.
In theory, it should pass, but in reality, this test fails.
The reason is that when the buffer is full, Thread A gets pended (blocked).
If Thread B later calls the get function, it will wake up Thread A, allowing it to resume and put the message into the queue.
In this situation, we need to know whether Thread A should continue with put to front or put to end.

Does this scenario make sense?

[github-actions]

Hi @alexpaschoaletto!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁