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

Add support to "/" for Read-Composite. #989

Closed
sbernard31 opened this issue Mar 23, 2021 · 11 comments
Closed

Add support to "/" for Read-Composite. #989

sbernard31 opened this issue Mar 23, 2021 · 11 comments
Labels
client Impact LWM2M client new feature New feature from LWM2M specification server Impact LWM2M server
Milestone

Comments

@sbernard31
Copy link
Contributor

Currently this is not supported :

[{"n":"/"}]

(source : http://www.openmobilealliance.org/release/LightweightM2M/V1_1_1-20190617-A/HTML-Version/OMA-TS-LightweightM2M_Core-V1_1_1-20190617-A.html#Table-745-10-SenML-JSON-payload-in-the-request-to-all-objects)

@sbernard31 sbernard31 added new feature New feature from LWM2M specification client Impact LWM2M client server Impact LWM2M server labels Mar 23, 2021
@sbernard31 sbernard31 added this to the 2.0.0 milestone Jul 1, 2021
@adamsero
Copy link
Contributor

Together with @JaroslawLegierski we came up with an implementation for this case. Let me know what you think about this kind of solution.

@sbernard31
Copy link
Contributor Author

I'm not sure I get it 🤔

It looks like a kind of HACK at leshan-server-demo side as you change a ReadCompositeRequest on / by ReadCompositeRequest on all specific object supported by the client (1a782ce#diff-8f94b871acdc39fb01247b73d4d7814389187176c1af3b0a6628efe7e1f4f62e)
The idea is more to add support of this feature in the library itself.
I mean we want to be able to create a new ReadCompositeRequest("/");

⚠️ I don't think so much about this feature but I guess the harder part is about making Encoder/Decoder compatible with it. This needs not so easy changes because current API is not adapted.

E.g for decoder :

Map<LwM2mPath, LwM2mNode> decodeNodes(byte[] content,
                                      ContentFormat format, List<LwM2mPath> paths, LwM2mModel model)
                                 throws CodecException;

when you call it with paths = ["/3","/2/0","/5/0/1"]
You should get something like a map { "/3"=> LwM2mObject, "2/0"=>LwM2mObjectInstance, "5/0/1"=>LwM2mSingleResource}

And so when you call it with paths = ["/3","/2/0","/5/0/1"] you should get {"/"=> LwM2mRoot}

But a LwM2mRoot which extends LwM2mNode does not exist for now.

@adamsero
Copy link
Contributor

adamsero commented Aug 1, 2022

Finally I was able to create a rough PoC, you can find it here. There's obviously a lot to improve in the code, but let me know if you even like my approach.

@sbernard31
Copy link
Contributor Author

I looked at this quickly and I feel it goes in the right direction.

The main design issue is how we should handle LwM2mRoot.getId() ?

(By the way I will be unavailable until 22th August)

@adamsero
Copy link
Contributor

adamsero commented Sep 6, 2022

@sbernard31 do you think we could advance with this issue?

@sbernard31
Copy link
Contributor Author

I currently have hard time working on Transport Layer abstraction. 😅

But I can review PR or code and give some feedback is needed from time to time.

Keep in mind that I will not integrate new feature in master before to get transport layer integrated. (bug fix can be integrated)

@mgdlkundera
Copy link
Contributor

@sbernard31 Here is a proposal of displaying read-composite on root path. I’ve implemented it as a list, below is a link to changes.
master...JaroslawLegierski:leshan:opl/composite_on_root

@sbernard31
Copy link
Contributor Author

I look at you work and with a Leshan client demo this looks like :
Capture d’écran de 2023-08-28 18-06-42

That changes a lot of other composite display.

Should we try to have something which looks more like others :
Capture d’écran de 2023-08-28 18-06-26

It's also totally OK if some feature supported by Leshan library is not in demo OR have no UI in demo. So are we sure we want a UI for it ?

Also maybe better to first focus on a PR about adding support of "/" to read composite in library with some integration tests.
Then once this is done, focus on the UI ?

@mgdlkundera
Copy link
Contributor

@sbernard31 I added integration tests for root case. Could you take a look on it?

@sbernard31
Copy link
Contributor Author

I looked at it quickly, I think this is the right direction.
Maybe you can review your code, clean it if needed, remove everything about demo then create a PR about it ? 🙂

@sbernard31
Copy link
Contributor Author

This is implemented by #1534 and will be available in next release 2.0.0-M14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Impact LWM2M client new feature New feature from LWM2M specification server Impact LWM2M server
Projects
None yet
Development

No branches or pull requests

3 participants