net: lwm2m: Add callback to report observers added/deleted and notification ack/timeout

[MaikVermeulen]

No description provided.

[MaikVermeulen]

Two things crossed my mind while working on this:

1. Is the lwm2m_rd_client_start() okay like this or should I not interfere with its API? I chose to do it this way to make it clear that the observe callback exists, and is different from the registration event callback.
2. Perhaps we would like to make a dedicated function find to which observer object a token belongs, currently this code, or slight variations of it, is in 3 or 4 places:

	SYS_SLIST_FOR_EACH_CONTAINER(&ctx->observer, obs, node) {
		if (memcmp(obs->token, token, tkl) == 0) {
			found_obj = obs;
			break;
		}

		prev_node = &obs->node;
	}

	if (!found_obj) {
		return -ENOENT;
	}

[huelsenfruchtzwerg]

Thanks for picking this up!

@JPSELC pointed out in #38531 that the callback would need to report the token to the user, and I'm inclined to agree with him.
When the callback only gives the path to the object, there's no way for the application to now what data exactly has been delivered, i.e. if two notifications are created ~close to each other, but one times out and one is acknowleged.

[MaikVermeulen]

@JPSELC pointed out in #38531 that the callback would need to report the token to the user, and I'm inclined to agree with him. When the callback only gives the path to the object, there's no way for the application to now what data exactly has been delivered, i.e. if two notifications are created ~close to each other, one times out and one is acknowledged.

Ah, I read it and didn't think about it anymore. In my mind I thought it would be fine to just send one notification at a time, perhaps per path. I see you and @JPSELC 's point though, so I will change it. Luckily for me, I believe it's available in all 4 places I added the callback anyway!

I have 2 follow-up questions to make sure I understand you correctly:

1. Would you need a callback on notification generation too then, so you know which tokens are still floating around?
2. And what operations would we further like to do with the token? Just get the path from it, or perhaps more? Need to know for potential API

[huelsenfruchtzwerg]

Just had a quick look, it looks to me like notifications for the same observations will always have the same token. So this won't work for the use case I described anyway, in which case I believe the API is fine as is. But somebody should double check, I might just be missing something. :)
If so, this raises another question: Should the client be modified to never have >1 notification per observation in flight/unacknowledged so what data is/is not delivered is answerable?

[JPSELC]

Repeat sends of the same notification will have the same token, as far as I know, but that is not the same as a new notification from the same path.
@huelsenfruchtzwerg If you are limiting the notification sending until the previous notification is acknowledged, you have to store them, I guess, which might link in well with the original motivation of storing notifications when offline.

[MaikVermeulen]

For my use-case, and indeed the notification storing it would be okay to limit it. However, we might be going beyond spec in that case, as in, we maybe shouldn't impose our own limitations on the client.

Since the tokens are generated per coap message, so for every notification instance, they remain the same for retransmissions because it's still the same coap message. New notifications will be new coap messages with new tokens.

Hence, it should be possible to allow multiple notifications per path to be active at the same time. We could make a generic callback that says 'all notifications are done', or have the application keep track of it itself using the callbacks currently in the commits of this PR. In the latter case, it would need to do bookkeeping of the tokens itself I guess.

Instead of the path, or on top of the path, or the token, or whatever arguments we agree to pass on, we could also extend it with a user data pointer so each application can do something smart for buffering or other purposes itself. We probably won't need all those arguments, but I'm sure we can think of something nice and usable for all of us!

For me personally, I would like to e.g. store temperature measurements with a timestamp, and send those as a bundled (level 2) notification. Because they are timestamped, the order wouldn't even be important. I would have to be able to pair a certain buffered entry [temperature, timestamp] to the observe_cb() result so I know whether it's been delivered and can be discarded. In that case, I would need to know which token belongs to which entry I'm trying to send a notification for. In that case, a mapping of tokens could be used, but perhaps a user data field would be easier and less memory consuming?

[huelsenfruchtzwerg]

Since the tokens are generated per coap message, so for every notification instance, they remain the same for retransmissions because it's still the same coap message. New notifications will be new coap messages with new tokens.

Not for notifications though, they explicitly set the token to the same as the observation.

That is, unless I'm misunderstanding the code.

[JPSELC]

Ah yes, I think that's the case.

[huelsenfruchtzwerg]

This is probably just a convenience thing though to easily look up the original observation, and we could "fix" this by adding a explicit link between original Notification-Msg -> Observation-Token. (at the cost of memory consumption, obviously)

[huelsenfruchtzwerg]

One side note:
I believe a "is_observed(ctx, path)" function would also be useful, else the application would need to duplicate the state in situations where it's e.g. waiting for a number of observations to be established. (to address #38534)

[JPSELC]

This is probably just a convenience thing though to easily look up the original observation, and we could "fix" this by adding a explicit link between original Notification-Msg -> Observation-Token. (at the cost of memory consumption, obviously)

I just checked, and it's the standard (V1.0.2) Section 8.2.6 indicates "Token of CoAP layer is used to match the asynchronous
notifications with the Observe GET." The Server uses the Token to understand what observer the notification relates to.

[MaikVermeulen]

Great discussion guys, thanks for the clarification.

Personally I would like to propose to add a pointer to a user object to the notifications. That way, on timeout or ACK, we can notify the application. The user object e.g. could be a pointer to a structure { measurement, timestamp } or whatever. The application could even add a unique identifier to that structure. I think this would solve the data buffering case quite well, leaving storage to the users and just having LwM2M as a transport protocol. One could have a list of measurements in e.g. RAM and get feedback on which ones are done and which ones are not. That list would indicate cleanly the number of live notifications, and buffer the data until ACKed. One drawback I do see is that the data would be duplicate, as it's in the notifications in the LwM2M engine, and in the user application.

As for the use-cases described in #38531, I think this user object way could work as well?
And of course, if you see alternative approaches, please share!

[JPSELC]

This is the messages array variable in lwm2m_engine.c

static struct lwm2m_message messages[CONFIG_LWM2M_ENGINE_MAX_MESSAGES];

which limits the maximum number of messages that are "active", and so limits the memory used by Zephyr for notifications. I think this may default to 3 or something like that, but obviously is configurable.
I think this is the array that the user needs visibility into. e.g. how many active messages?
I also think that if a notification times out, or the CONFIG_LWM2M_ENGINE_MAX_MESSAGES limit is reached, you are effectively offline (even if temporarily), and should start storing notifications in user space.
Perhaps at this point:

zephyr/subsys/net/lib/lwm2m/lwm2m_engine.c

Line 4236 in d75b7a6

LOG_ERR("Unable to get a lwm2m message!");

[MaikVermeulen]

For everyone interested:

Currently on Github I see that there are 3 or so issues all related to buffering/handling intermittent connectivity.

For focussed discussion, I think it would be nicer to have a central thread on it.
I’ve made a thread ‘Client offline mode, data buffering, etc.’ in the LwM2M channel on the Discord.

[huelsenfruchtzwerg]

I also think that if a notification times out, or the CONFIG_LWM2M_ENGINE_MAX_MESSAGES limit is reached, you are effectively offline (even if temporarily), and should start storing notifications in user space.

Right now notifications will wait for space for new messages to be available and create them at that point.

[rlubos]

Hey, I was not able to dig in the detail into the code yet, but just to clarify:

• Each notification should use the same token as the one present in the initial GET message triggering the observation. So you cannot differentiate between individual notifications based on the token only (but I see you've already figured this out). This is covered by the CoAP observe spec.
• Individual notifications can be differentiated based on either CoAP Message ID value or the Observer option value. The latter should be incremented for each notification so that the observer can identify outdated notifications.

[pfalcon]

Can the title of this PR be updated please, so it was clear what it's about?