Implementation of Observe-Composite

[Michal-Wadowski]

Hello everyone!

I want to participate in the implementation of Observe-Composite functionality.

I'm working on prototype of this functionality and I have a problem:

In COAP module there is org.eclipse.californium.core.server.ServerMessageDeliverer.deliverRequest() method. This method gets Resource according to UriPath passed in request.
Then method checkForObserveOption() addeds ObserveRelation relation to the exchange. This is necessary to correlate observe client responses with server observe request.

But for the Observe-Composite should be passed multiple paths in payload. I could set LwM2mPath.ROOTPATH as target and paths in payload like it's done in CoapRequestBuilder.visit(ReadCompositeRequest request), but in that case in method ServerMessageDeliverer.deliverRequest() I will get RootResource and checkForObserveOption() will not pass, because RootResource is not observable.

Am I doing this in wrong way, or implementation of COAP may be incomplete?

Thank you in advance for your answer.

[boaks]

AFAIK, this is not implemented in Californium. It may be possible to replace the ServerMessageDeliverer with implementation, which can deal with that. If you're interested to implement such an alternative ServerMessageDeliverer, let me know, if some changes (e.g. removing the final in checkForObserveOption) are required to do so.

[sbernard31]

I want to participate in the implementation of Observe-Composite functionality.

Good to know. 👍

I didn't look deeply at the LWM2M Observe-Composite feature. By the past OMA didn't really get the spirit of CoAP Observe (rfc7641), I hope this new observe feature will not be a new nest of issues 😅.

I will get RootResource and checkForObserveOption() will not pass, because RootResource is not observable.

If I understand you correctly you want to make the root resource of LWM2M client observable ?
If I'm correct, just edit : org.eclipse.leshan.client.californium.RootResource constructor :

    public RootResource(RegistrationEngine registrationEngine, CaliforniumEndpointsManager endpointsManager,
            BootstrapHandler bootstrapHandler, CoapServer coapServer, LwM2mRootEnabler rootEnabler,
            LwM2mNodeEncoder encoder, LwM2mNodeDecoder decoder) {
        super("", registrationEngine, endpointsManager);
        this.bootstrapHandler = bootstrapHandler;
        setVisible(false);
        // ---- add the line below ----
        setObservable(true);
        // ----------------------------
        this.coapServer = coapServer;
        this.rootEnabler = rootEnabler;
        this.encoder = encoder;
        this.decoder = decoder;
    }

HTH

[Michal-Wadowski]

Thank you a lot :) The solution setObservable(true) allows me to go one step further. I hope we can set observable flag on client root. I can't find any documentation that forbid this.

[boaks]

forbid this

I'm not sure, what the scope of "forbid" should be.

• LwM2M TS?
• Californium?
• RFC7641?

Do you have a link to the LwM2M TS part, which maps that "Observe-Composite" to RFC7641??

[sbernard31]

I don't know if RFC7641 allow to observe root. (I don't remember anything which forbid it but I didn't look at this for a while).

About californium, I guess nothing prevent this but we will see. @boaks do you know anything about this ?

About LWM2M, I understand that "Observe-Composite" is an observe on root but not crystal clear to me.

Here is the LWM2M v1.1 spec about this (the one we currently try to implement):

• lwm2m-v1.1@core§6.4.4-Observer-Composite-Operation
• lwm2m-v1.1@transport§6.4.5-Information Reporting Interface

Here is the LWM2M v1.2 spec which could help to better understand, in §transport there is an example with query params which targets root :

• lwm2m-v1.2@core§6.4.4-Observer-Composite-Operation
• lwm2m-v1.2@transport§6.4.5-Information Reporting Interface

@boaks, if Observe-Composite targets / does it means that we could have only 1 Observe-Composite at a time ?

I will maybe ask more clarification about this feature to OMA 🤔 .

[boaks]

FMPOV, RFC7641 doesn't mention that, mainly because there is no difference in the root or other resources. What seems to be special in LwM2M is the relation of the payload of resource and their child resources and the expectations resulting out of that. Until now, I considered that to be limited to the way, "changes are detected". Hope that is still true.

[boaks]

Some hints:
Though the composite-operation seems to depend on the payload of the read-request, that may cause additional changes in order to separate multiple of such observer-requests. My feeling is, that multiple observer may cause some implementation incompatibilities, if the implementations are not aware of that.

core-mailing-list

[boaks]

if Observe-Composite targets / does it means that we could have only 1 Observe-Composite at a time ?

For Californium, that seems to be somehow "in between" :-). Using different tokens should enable to have multiple observes. Calling "changed(ObserveRelationFilter)" then hopefully works, to filter the intended relations of such a change. But I'm not that sure.

My concern in the comment above is more, that usually lwm2m-clients are not Californium based, so it may get important, how other implementations deal with that.

[sbernard31]

I opened an issue to talk about "1 Observe-Composite at a time" : OpenMobileAlliance/OMA_LwM2M_for_Developers#528

There is also a discussion about Active Cancel Observe-Composite where I didn't get some OMA choices : OpenMobileAlliance/OMA_LwM2M_for_Developers#481

@boaks, I don't know if you notice that Observe-Composite is using FETCH (and not GET), I don't know if californium support this correctly ?

[boaks]

I don't know if you notice by Observe-Composite is using FETCH (and not GET)

AFAIK, GET should not come with payload (at least in a strict interpretation, see FETCH .)

I don't know if californium support this correctly ?

I just added the codepoints on a request. I don't use it, therefore I don't know, if the "support" is correct.

[sbernard31]

@MichalWadowskiOrange, if there are other feature/issue that Orange is working on, please let us know. (Even if there is not so much commitment, this is just to be sure we will not start to work on same thing)

[Przem83]

Hi,
For now Orange will focus mainly on verifying the already implemented LWM2M 1.1 functionality and on the Observe-Composite implementation. After the holidays we also intend to start some work on the NIDD transport for Lora and 3GPP - as it seams that no body is working on it currently. For the remaining LWM2M 1.1 topics please let us know were you could use some more manpower and we'll do our best to help. Our main goal is to speed up the Leshan 2.0 release as much as possible - so we'll do our best to not disturb the ongoing works unless our help is welcome.

[sbernard31]

@Przem83 let continue this discussion at #1049 and keep this issue for Observe-Composite.

[Michal-Wadowski]

I have a question: In interface ObservationListener on Observe-Composite event should we call newObservation() and onResponse() multiple times (as many as items are in response), or rather should we create CompositeObservation type and create newObservation() onResponse() equivalents?

public interface ObservationListener {

///...

    void newObservation(CompositeObservation observation, Registration registration);
    void cancelled(CompositeObservation observation);
    void onResponse(CompositeObservation observation, Registration registration, ObserveResponse response);
    void onError(CompositeObservation observation, Registration registration, Exception error);
}

[sbernard31]

I have a question: In interface ObservationListener on Observe-Composite event should we call newObservation() and onResponse() multiple times (as many as items are in response)

I guess this is maybe not a good idea. I'm afraid that some users want to get all the data at same time to be able to process it all together. 🤔

or rather should we create CompositeObservation type and create newObservation() onResponse() equivalents?

This could be a solution but I guess there is a lot of code where you will need to handle those 2 type of Observation.

Maybe we should consider to have a hierarchy of class :
interface or abstract class :Observation
SingleObservation extends Observation with a LwM2mPath getPath() method
CompositeObservation extends Observation with a List<String> getPaths() method

Which could help to reuse some existing code 🤔

And so having something like this :

public interface ObservationListener {

///...

    void newObservation(Observation observation, Registration registration);
    void cancelled(Observation observation);
    void onResponse(SingleObservation observation, Registration registration, ObserveResponse response);
    void onResponse(CompositeObservation observation, Registration registration, ObserveCompositeResponse response);
    void onError(Observation observation, Registration registration, Exception error);
}

This is just an idea, I didn't dig this too much.
Do not hesitate to share code even if this is not finished.

[sbernard31]

At client side all the listener class hierarchy ( ObjectsListener, ObjectListener, ResourceListener) will maybe need to be modified too 🤔 ?

I'm currently look at "Add Resource Instance level support to Observe"(#1041) and I will need to modified it too.

[Michal-Wadowski]

I'm probing your proposition of SingleObservation/CompositeObservation/Observation, and I encounter another issue: in ObservationService interface we have Set<Observation> getObservations(Registration registration);

Should we split this declaration into getSingleObservations()/getCompositeObservations() or left getObservations() as is? The second option may end up in casting result into SingleObservation/CompositeObservation if we want to access path/paths.

Or maybe should we make the Observation path size agnostic I mean there will be always list of paths?

[Michal-Wadowski]

Do not hesitate to share code even if this is not finished.

We organized our work in the fork: https://github.com/Przem83/leshan and implementation of Observe-Composite is in dev_observe_composite branch. Of course implementation is only the proof of concept 😄

[sbernard31]

My idea about introducing an Observation hierarchy was to avoid to multiply method.
So I would go for the second. About casting result, we can either live with it or do pretty much as we do with LwM2mResource hierarchy.

Anyway, if you find any argument which makes you feel that first way is better share it because I have no strong opinion for now.

[Michal-Wadowski]

I'm want to announce that I just implemented observe-compose functionality (in repository that I mentioned earlier) 😄. It needs a lot of code cleaning and the Redis integration is not implemented yet, but you can check the solution in general.

[sbernard31]

👍 I could probably look at this next week.

[sbernard31]

Do directly linked but I just pushed a PR about Observe Resource Instance (#1054)

[sbernard31]

@MichalWadowskiOrange, I looked a little bit at your code.
===> 847fb11...741b5c2

I just made a high level review to look at the direction your take and so here my first remarks:

1) I would keep the name ObserveResponse (vs renaming to SingleObserveResponse because we are nearer to the LWM2M specification wording. (Observe-Request/Response and ObserveComposite-Request/Response)

2) About this part of the code
https://github.com/eclipse/leshan/blob/741b5c26f5443c9beff8be80d3f447db95950378/leshan-client-cf/src/main/java/org/eclipse/leshan/client/californium/RootResource.java#L137-L143

I'm not sure but I don't think this is the right approach. Because I feel we should see Observe-Composite as an only one observe relation. This would be more the CoAP spirit and the LWM2M specification says :

If any of the conditions attached to one or more resources under observation meets the notification criteria a notify will be generated by the LwM2M client, which will include the value for each of the resources listed in the "Observe-Composite" operation. Hence, the resources that have not met the notify condition will still be included in the notification message.

To implement this is that way, you probably need to create a custom ObserveRelationFilter (see ObjectResource and ResourceObserveFilter as example from #1054) which will select the relation if one resource which changed is in the ObserveCompositeRequest payload. (The issue here is that we will probably need to reparse the payload each time but maybe we can use store the list of path in Request.setUserContext(Map<String, String>))

@boaks, do you think I have the right approach or I missed something ? (or maybe you think that @MichalWadowskiOrange way is OK too?)

[Michal-Wadowski]

Thanks for the review 😄

Fortunately I can remove the whole code you mentioned without any impact for the functionality. It's residue of times when I was developing this solution.

According to SingleObserveResponse - I wanted to unify the naming to be similar to SingleObservation/CompositeObservation. But I agree that we should keep the naming convention as close to specification as possible 😄

[Michal-Wadowski]

I just want to note that the code is ready for review.

BTW, should I create separate issue and branch/PR for Cancel Observe-Composite operation?

[sbernard31]

I just want to note that the code is ready for review.

I think the next step is to provide PR(s) for this code (or maybe you want to experiment Cancel Observer-composite first)

To make easier review and get better feedback. I strongly encourage to try to do several small PRs or at least to several commits.

Personally,
1.) I always start by trying to implement my design idea. Just to be sure It's doable and trying to find issue that I didn't anticipate.
2.) Then once I have a clear idea about what I need to do, I try to separate my code in small consistent commits. (this also the step where I clean my code)
2.1) Generally there was several small commits about refactoring which prepare the ground for real new features.
2.2) Then I add the feature itself (separating client and server code)
2.3) Then I add some integration tests
2.4) when needed I add demo code. (optional)

You could look at #1054, to have an example, in this PR I push all commit on same PR but you can do more PR if needed (maybe easier to review)

(We don't use merge so PR must not contains merged commit, you should use rebase instead)

Some examples of commit/PRs :

• Add SingleObservations/CompositeObservation classes. (Not that I would keep Observation name instead of AbstractObservation, it sounds contradictory with my previous comment but I was just talking about request/response 😅 any not a big deal and we can discuss about that in the first PR)
• Add CompositeObserve-Request/Response classes.
• Add Composite Observe feature at server side.
• Add Composite Observe feature at client side.
• Add integration tests.

(This is just example and maybe you can find better or more suitable to you, the idea is to separate small consistent modifications)

Hoping you approve the approach. 😬

BTW, should I create separate issue and branch/PR for Cancel Observe-Composite operation?

Yes this should be done in other PRs.

[Michal-Wadowski]

My conclusion from splitting the functionality into separated pull requests:

Its easy to discuss every separate piece. It's probably easier to code reveiw, because changes are smaller.

But it's very hard to maintain. Splitting work into pieces needs a lot of work, and after changes in one PR it also needs a lot of work to propagate changes into another PRs. The same situation if we need to synchronize with master.

I think, creating multiple commits in on PRs is better solution :)

Btw. I'm going to holidays :), so I can't participate in project for three weeks. I hope these pull requests will be final :)

[sbernard31]

@Michal-Wadowski Enjoy your Holidays 😉

[sbernard31]

I will start to (re)review your PRs and integrate it in a composite_observe branch.
If I found issue, I will fix it directly (and let a comment on each PR for each modification I will do and push it in separated commit)

Once this will be done, I will let you "validate" the branch (if wanted) before to integrate it in master.

[sbernard31]

Branch can be reviewed at #1083.

[sbernard31]

But it's very hard to maintain.

I agree there is a more extra cost to work like this.

But in this particular case I feel there are some issues which could explain this was more painful than it could be :

1. you have some formatting issue (this generates a lot of conflict)
2. first time you work and the project and the feature chosen was not so easy and pretty much impacting.
3. this work contains some refactoring/renaming which changes the code a lot.
4. you separate work on several commits/PR but you push all of them pretty much at the same time.

My advice :
Configuring tooling correctly to avoid this formatting issue is very important. (captain obvious 😁)

If you want to do some code improvement, this is OK and very welcomed but prefer to make it in separate PRs/task or just open an issue.

In a general way to not hesitate to do more commit than needed as merging commits (even if there are not consecutive) is often easier than splitting.

This is for big/complex feature only :
Doing a kind of draft to experiment the feature (crappy code) and see if this is doable and how this should be done. (This is what you done)
Then sharing the idea (or the crappy code) to see if this is the right direction. (as you do)
Once it's OK, it's time for cleaning and pushing the code :
Splitting the code helps a lot for peer review but this is also a good time to you to review you own code, rethink about it and see if all changes you propose make sense. (this will also make easier to review your own code) As this stage some time I rewrite the code from scratch keeping the general idea.
For this step, you should probably not push all PRs at the same time, but push first step, then review, then fix PR, then work on the next cleaning.

HTH

Anyway, I hope that was not a too unpleasant experience, and thx for your work and patience, it is really appreciated 🙏 !

Just to let you know, I will be out of office at the end of day, I'm back begin of September.

[Michal-Wadowski]

Thank you for cleaning the code in PR #1083 and make one more similar to the currently written code 😄

According to my issues with the code style - I tried to use IntelliJ to do whole the work, but I think it will be much easier to just use Eclipse to polish final code next time.

[sbernard31]

@Michal-Wadowski, your work is now integrated in master.
Do you have a list of missing feature or remaining improvement related to this feature ?
This list does not aim to defined what should be done next but just what is missing.
"should we implement some items of this list or not" is another question.

I have in mind the "support of timestamped value", I'm not sure if there is something else ?

[sbernard31]

Last jenkins job failed because of a test failure:

[INFO] Running org.eclipse.leshan.integration.tests.observe.ObserveCompositeTest
[ERROR] Tests run: 8, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 6.281 s <<< FAILURE! - in org.eclipse.leshan.integration.tests.observe.ObserveCompositeTest
[ERROR] org.eclipse.leshan.integration.tests.observe.ObserveCompositeTest.can_composite_observe_on_multiple_resources  Time elapsed: 0.018 s  <<< FAILURE!
java.lang.AssertionError: expected:<LwM2mSingleResource [id=14, value=+02, type=STRING]> but was:<LwM2mSingleResource [id=14, value=Z, type=STRING]>
	at org.eclipse.leshan.integration.tests.observe.ObserveCompositeTest.can_composite_observe_on_multiple_resources(ObserveCompositeTest.java:200)

I'm working on a fix.
Fixed by 33f200b.

[Michal-Wadowski]

No, I have no opinion about what should be done yet (except timestamped values). If I find one I will note here.

[sbernard31]

I created a dedicated issue for timestamped values feature : see #1089.

I think we can close this one now ?

If I find one I will note here.

Please create a new issue instead.

@Michal-Wadowski let me know if you are working or plan to work on something else.

[Michal-Wadowski]

Yes, I think we can close this issue. For now I'm going to work on #1046