zbus: ZBUS_WAITER_DEFINE

[JordanYates]

Add a new type of observer that can be used to wait on a channel until data is published.

Is your enhancement proposal related to a problem? Please describe.

Waiting until new data is published on a channel currently requires polling in a loop or defining an observer of a different type that gives a semaphore in a dedicated callback. This is verbose for such a simple use case.

Describe the solution you'd like

This PR

Describe alternatives you've considered

Static observers can use this pattern.

static K_SEM_DEFINE(chan_published, 0, 1);
static void callback1(const struct zbus_channel *chan)
{
   k_sem_give(&chan_published);
}
ZBUS_LISTENER_DEFINE(lis1, callback1);

Which is more verbose than:

static K_SEM_DEFINE(chan_published, 0, 1);
ZBUS_WAITER_DEFINE(waiter, &chan_published);

Runtime observers don't have a clear alternative, as there is no easy way to map the semaphore handle into the callback.

[rodrigopex]

@JordanYates, please describe the addition more.

Please follow this format #88499

What is the issue this feature is solving? Why is this important for the community?

[JordanYates]

@JordanYates, please describe the addition more.

Please follow this format #88499

What is the issue this feature is solving? Why is this important for the community?

Additional information added to the PR.
As for what it is solving, the most basic functionality in a pub-sub messaging system.
Waiting until the next message arrives in the simplest possible way with the least amount of required boilerplate.

[JordanYates]

Waiting until new data is published on a channel currently requires polling in a loop or defining an observer of a different type that gives a semaphore in a dedicated callback. This is verbose for such a simple use case.

I dont understand, why cant you use zbus_sub_wait() or zbus_sub_wait_msg()

1. zbus_sub_wait requires a zbus subscriber, which has a RAM cost (struct k_msgq is not the smallest object, even ignoring the RAM required for the queue data)
2. zbus_sub_wait is not a kernel object, so it cannot be used with k_poll

[rodrigopex]

@JordanYates, The first alternative is already good enough.

static K_SEM_DEFINE(chan_published, 0, 1);
static void callback1(const struct zbus_channel *chan)
{
   k_sem_give(&chan_published);
}
ZBUS_LISTENER_DEFINE(lis1, callback1);

I don't see the benefit in adding another type of observer just for that use case. Another benefit of your alternative is that you can reuse the same listener for all the semaphores of your system, like:

static K_SEM_DEFINE(chan1_published, 0, 1);
static K_SEM_DEFINE(chan2_published, 0, 1);
static K_SEM_DEFINE(chan3_published, 0, 1);
static K_SEM_DEFINE(chan4_published, 0, 1);
static void callback1(const struct zbus_channel *chan)
{
    if(chan == &chan1) {
        k_sem_give(&chan1_published);
    } else if(chan == &chan2) {
        k_sem_give(&chan2_published);
    } else if(chan == &chan3) {
        k_sem_give(&chan3_published);
    } else if(chan == &chan4) {
        k_sem_give(&chan4_published);
    } 
}
ZBUS_LISTENER_DEFINE(lis_waiter, callback1);

With just one listener, you can address the need you're pointing out without adding more complexity to zbus. I strongly suggest not adding the feature.

[JordanYates]

I don't see the benefit in adding another type of observer just for that use case.

The use case of natively giving the primary Zephyr synchronization object with the minimum boilerplate?
Seems like a benefit to me.

Another benefit of your alternative is that you can reuse the same listener for all the semaphores of your system, like:

Which you then can't enable or disable on a per channel basis.

With just one listener, you can address the need you're pointing out without adding more complexity to zbus

The complexity of this?

case ZBUS_OBSERVER_WAITER_TYPE: {
    k_sem_give(obs->sem);
    break;
}

I don't see the difference between this and #88499, it is already possible to achieve that functionality without adding more complexity to zbus as well. Why is a callback with k_work_reschedule any different from k_sem_give?

[rodrigopex]

@JordanYates

Again, you are trying to add something that breaks consistency. The first was the runtime observer. Maybe you don't care about it, but I do. People started asking me about what a "zbus_observer_node" is. Now I need to explain that, not you.

I mean usage and maintenance complexity. The more variability we have, the steeper the learning curve for new developers is, and the higher the maintenance cost.

All observers can observe more than one channel and behave the same. Zbus executes the listeners implicitly and notifies subscribers, so they need to call zbus_sub_wait*. This is naturally expected by using zbus. The only inconsistency with this rule was broken with the change in the runtime observers you added.

Thus, with your addition, we have a type of observer that:

• Aims to observe a single channel (more than one would be odd to know the channel origin).

• Uses a semaphore (explicitly) to wait for the channel notification.

• Has an unfamiliar name, it is neither a listener nor a subscriber.

These are unaligned with zbus.

Regarding the #88499, that is perfectly aligned with the current zbus way. Everyone who already uses a listener will be able to use that. With a bit of change (adding "ASYNC" to the definition), they can exchange between a LISTENER and an ASYNC_LISTENER. Asynchronous listeners solve a gap in executing a listener outside the ISR context, which is a problem today and often requires this boilerplate code.

Another clear alternative to your feature suggestion is to use a subscriber with just one element in the queue:

ZBUS_SUBSCRIBER_DEFINE(sub1, 1)

If you need something to use in the k_poll, you can use obs->queue.

Lastly, I suggest you change the feature to align with zbus or remove the PR.

[JordanYates]

Again, you are trying to add something that breaks consistency. The first was the runtime observer. Maybe you don't care about it, but I do. People started asking me about what a "zbus_observer_node" is. Now I need to explain that, not you.

I assume that you are referring to #84271. At no point was the problem of "consistency" raised, my reading was that people agreed that internal use of k_malloc was problematic, and that you (and others) approved it.

Now you are criticizing me for working through your feedback over the course of several months and getting the PR to the point of approval and merging? If the contribution was as worthless as you seem to feel it was, open a revert PR and get others to agree with you.

I apologize for attempting to improve your subsystem.

[rodrigopex]

I assume that you are referring to #84271. At no point was the problem of "consistency" raised, my reading was that people agreed that internal use of k_malloc was problematic, and that you (and others) approved it.

Did you forget the whole discussion #79777? I pointed out several times that the change would affect the way the observers work. I mentioned "inconsistencies" and "break compatibility". I was against that. However, when you created the new PR (#84271), I was exhausted and in a tough moment in my life. I was tired of discussing it, so I gave up and accepted it, which I regret. I apologise to the community for letting it be added without more debate.

Now you are criticizing me for working through your feedback over the course of several months and getting the PR to the point of approval and merging? If the contribution was as worthless as you seem to feel it was, open a revert PR and get others to agree with you.

A changing (not reverse) PR is on the way. The feature is essential, but the implementation did not consider the side effects.

I apologize for attempting to improve your subsystem.

It's not mine, you know that. However, as a maintainer, I will strive to make it the best, most consistent, and healthiest as possible.

[rodrigopex]

About this PR, I still think this has more cons than pros. Please, adjust that to be more consistent with the current zbus way.

[JordanYates]

Did you forget the whole discussion #79777? I pointed out several times that the change would affect the way the observers work.

The only inconsistency with this rule was broken with the change in the runtime observers you added.

I did not. You observed that the original proposal would break the ability to register the same observer against multiple channels. That was true, and I reworked the PR to fix that limitation.

There was no change, I don't understand your argument here. If you call k_malloc/k_free around zbus_chan_add_obs everything work EXACTLY the same as it did before.

About this PR, I still think this has more cons than pros. Please, adjust that to be more consistent with the current zbus way.

This is sufficiently vague to be useless. If we are just going to argue back and forth about the utility of giving a semaphore directly instead of needing to write a callback, this will never go anywhere since you have the final say. Unless you have concrete requests, I don't see the point.

A semaphore will always be a semaphore, and do the things a semaphore does. It you dislike the idea of semaphores being supported in zbus, there is nothing more I can do to change how a semaphore behaves.

Regardless, my response to the various "this is not aligned with zbus" points:

All observers can observe more than one channel and behave the same

This observer is exactly the same, it can "wait" on as many channels as the user desires.

Aims to observe a single channel (more than one would be odd to know the channel origin).

While observing a single channel is the simplest case (as is also true for listeners and subscribers), there is nothing stopping it being used on multiple channels. There are use-cases where it does not matter which channel was published to (e.g. triggering a state-machine iteration).

Uses a semaphore (explicitly) to wait for the channel notification.

That's the definition, no argument there

Has an unfamiliar name, it is neither a listener nor a subscriber.

"listener" and "subscriber" were unfamiliar names (in the zbus context) until zbus came into being as well.
I have confidence that people will understand that a "waiter" waits for something to happen.

[rodrigopex]

What do you think of calling that ZBUS_SEM_SUBSCRIBER? With that, the name would be in the "subscriber box".

static K_SEM_DEFINE(sem_chan_published, 0, 1);
ZBUS_SEM_SUBSCRIBER_DEFINE(waiter, &sem_chan_published);

To make that clear. Listeners are observers whose actions are known at the time of publication, since subscribers are asynchronous and do not state their actions at the time of publication. So, you are adding a different kind of subscriber. Sounds good to you? @JordanYates