discover and tests by danielhqv · Pull Request #484 · eclipse-leshan/leshan

danielhqv · 2018-03-07T13:41:51Z

To be able to get feedback on this, I submit this pull request before the "added Attribute" pull request have been merged, so the contents of that pull request is also present here for now.

This adds discover capability and attribute support to existing interfaces and base classes. The contents of the "added Attribute" pull request is included here.

It's a rather large pull request, but I think it might be hard to find any suitable fragments to break it up into.

Signed-off-by: Daniel Persson daniel.p.persson@husqvarnagroup.com

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

danielhqv · 2018-03-07T13:45:55Z

+     * @param instanceId The instance id of this instance
+     * @return An array of Link objects, one for the instance and one for each available resource in the instance
+     */
+    Link[] discoverInstance(int objectId, AttributeSet objectAttributes, int instanceId);


This might feel like it breaks the API by not returning a DiscoverResponse, but I think it's the cleanest way to solve it. When performing discover on the object level, this method will also need to be called and then the links from all instances should be merged into one DiscoverResponse at the object level.

The idea about returning a Response instead of a content is about handle error case at instance level.
So if we could find a solution with a Discover Response I would prefer.

We already to this kind of merger for read response (see ObjectEnabler.getLwM2mObjectInstance())

sbernard31

It's a rather large pull request, but I think it might be hard to find any suitable fragments to break it up into.

Yes it's a pretty large PR, so my feedback would be a bit confused.
I see several tasks to separate this PR :

add attributeSet
use attributeSet for writeAttributeRequests.
remove deprecate ObserveSpec
add support of writte attribute at client side
enhance support of discover to return attributes.

You can find several feedbacks below.

About the design in the outline, I ask myself if we can not just handle all the attributes stuff at ObjectEnabler level.
I mean do we really want to customize by instance the behavior ?
We maybe could have a kind of AttributesTree in ObjectEnabler and just build object link with this ?

sbernard31 · 2018-03-07T14:09:05Z

-    <packaging>bundle</packaging>
-    <name>leshan - client core</name>
-    <description>A LWM2M client implementation which abstracts transport layer. A transport implementation like &quot;leshan-client-cf&quot; is needed.</description>
+	xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">


Please avoid to change all the formatting.

sbernard31 · 2018-03-07T14:14:52Z

        return DiscoverResponse.badRequest(null);
    }
-
+    


Unnecessary spaces

sbernard31 · 2018-03-07T14:16:43Z

        return instance.write(path.getResourceId(), (LwM2mResource) request.getNode());
    }
-
+    


unnecessary spaces

sbernard31 · 2018-03-07T14:23:24Z


-    public WriteAttributesRequest(int objectId, ObserveSpec observeSpec) throws InvalidRequestException {
-        this(new LwM2mPath(objectId), observeSpec);
+    public WriteAttributesRequest(int objectId, AttributeSet attributes) {


Throws InvalidRequestException should be added to all the new constructors.

About the InvalidRequestException... I thought that the convention was to only add throws if it is necessary (checked exceptions?), but I may very well be wrong about this.

I think most people are doing like you thought. I mean "adding throws only for checked exceptions". I also think that most people use checked exception for "normal" flow and so force their users to catch exception. (which could make sense)

In Leshan, there are some cases where we choose to use "unchecked/runtime" Exception for exception which could make sense to catch in a normal execution flow. We don't want to force users to catch this exception but we would also give it visibility as we thought that this could be useful to catch it.

I don't know if it's clear :) but this is not a "so strange" way as even in the JDK code you could find something like this (e.g. Integer.parseInt)

I will reinstate them. Our linter tells me to remove them, though... :)

sbernard31 · 2018-03-07T14:34:42Z

-    public WriteAttributesRequest(String path, ObserveSpec observeSpec) throws InvalidRequestException {
-        this(newPath(path), observeSpec);
+    @Deprecated
+    public WriteAttributesRequest(String path, ObserveSpec observeSpec) {


I would remove this. maybe in another PR/commit.
Do not remove throws InvalidRequestException

sbernard31 · 2018-03-07T15:33:36Z

+     * attributes themselves.
+     * @return the response object representing the outcome of the operation.
+     */
+    WriteAttributesResponse writeInstanceAttributes(AttributeSet attributes);


I feel this could be handle directly in ObjectEnabler like we do for other request.
I mean instance level request is handle in ObjectEnabler, resource level request by instanceEnabler

sbernard31 · 2018-03-07T15:35:46Z

        }
    }

+    public ObjectEnabler(int id, ObjectModel objectModel, Map<Integer, LwM2mInstanceEnabler> instances,


I do not like so much the idea about adding this constructor just for test

It's the only way to set the object attributes. Do you suggest a setter? I'm thinking that the attributes will be known at object creation?

sbernard31 · 2018-03-07T15:38:44Z


    private Map<Integer, LwM2mInstanceEnabler> instances;
    private LwM2mInstanceEnablerFactory instanceFactory;
+    private AttributeSet objectAttributes;


Should we handle this at BaseObjectEnabler ?

sbernard31 · 2018-03-07T15:42:04Z

+package org.eclipse.leshan.client.util;
+
+import java.util.ArrayList;
+import java.util.Arrays;


unused import

sbernard31 · 2018-03-07T15:59:25Z

+     * @param instanceId The instance id of this instance
+     * @return An array of Link objects, one for the instance and one for each available resource in the instance
+     */
+    Link[] discoverInstance(int objectId, AttributeSet objectAttributes, int instanceId);


I feel this could be handle directly in ObjectEnabler like we do for other request.
I mean instance level request is handle in ObjectEnabler, resource level request by instanceEnabler

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

danielhqv · 2018-03-09T16:36:06Z

When implementing Discover, incl. full attribute support, there are several things to consider:

On what levels should the attributes be stored by default (in ObjectEnabler, BaseInstanceEnabler etc.)
Would a LwM2mResourceEnabler interface with a default implementation be reasonable?
How should we make the result of the full attribute chain (object / instance/ resource) available to the LwM2mInstanceEnabler implementation, for instance to implement a polling observe pattern on the client where the correct pmin and pmax needs to be known.

Regarding the fact that you would prefer DiscoverResponse discoverInstance(..., I agree. But how should we solve the fact that both a Discover on object level and instance level should return the same links for any instance, only without attributes if it's performed on the object level?

But having the ObjectEnabler own the entire attribute hierarchy, as I interpreted one of your comments, seems counter-intuitive to me. What is wrong with letting each instance own its own attributes, especially since the same resource id in two different instances could have different attribute values?

I thought about opening an issue where this could be discussed, but I'll keep this here for now.

sbernard31 · 2018-03-19T17:07:25Z

Here a little description about the design :

LwM2mObjectEnabler interface is the contract. This is what must be implemented to add a new LWM2M object.
BaseObjectEnabler is an abstract class which could be used as base for implementing a new LWM2M object.
ObjectEnabler is an object implementation which delegate the "real" logic to LwM2mInstanceEnabler.

But having the ObjectEnabler own the entire attribute hierarchy, as I interpreted one of your comments, seems counter-intuitive to me.

I even think about putting the hierarchy at BaseObjectEnabler level. This advantage is to provide those features even for users which does not use ObjectEnabler and LwM2MInstanceEnabler. I do not go as deeper as you and so I could be wrong. But I feel this is possible because, we doesn't not need any information from LwM2MInstanceEnabler except available instances and resources supported (for this one we should add a kind of availableResource(instanceID) to the LwM2mObjectEnabler). Once we now that, we can store all the Attributes data at BaseObjectEnabler.

I feel that observe part could be implement at this level too playing around notifySender.

The risk I see about this solution.

maybe the BaseObjectEnabler would become too large. (delegate the code to others classes, like AttributeTree could help)
I'm not sure of this but maybe keep synchronized AttributeTree with the real resource tree could be a problem(on deletion or addition).

Regarding the fact that you would prefer DiscoverResponse discoverInstance(..., I agree. But how should we solve the fact that both a Discover on object level and instance level should return the same links for any instance, only without attributes if it's performed on the object level?

I can not see the issue. Exactly the same way you do now ? get the link and remove attribute is needed. (I feel with the solution exposed above you don't need to do this at all)

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

danielhqv · 2018-03-23T16:33:17Z

I've pushed updates now that fixes some comments you had, including letting the LwM2mInstanceEnabler.discoverInstance and discoverResource return DiscoverResponse instead.

I'll have to think about how to move forward with this. We're in big need of being able to simulate Discover, but being able to write attributes are just a minor part of that. I'm currently using the ObjectEnabler as a base class for our simulator objects, but that may have been a mistake since that also makes me dependent on LwM2mInstanceEnabler. It might be better for me to roll my own interfaces beyond LwM2mObjectEnabler, but I haven't realised that until just now.

And since I'm pausing this work, I think the "added Attribute" could be merged?

sbernard31 · 2018-04-05T09:35:18Z

@danielhqv, sry for the delay.

If I well understand you will focus at short term on implementing discover for your simulator based on LwM2mObjectEnabler ?

And since I'm pausing this work

Do you plan to rework on this PR later ?

I think the "added Attribute" could be merged?

Following thoses subtasks :

add attributeSet
use attributeSet for writeAttributeRequests.
remove deprecate ObserveSpec
add support of writte attribute at client side
enhance support of discover to return attributes.

I think this does not make so much sense to integrate add attributeSet if we don't use it.
I feel the minimum integration in master would be add attributeSet + use attributeSet for writeAttributeRequests and optionally remove deprecate ObserveSpec

WDYT ?

danielhqv · 2018-04-10T10:36:43Z

Yes, that integration sequence could make sense. I'll see if I can get time in the near future to work more on it.

danielhqv · 2018-06-20T16:52:35Z

I will probably not be able to work on this for some more months (vacation etc.), so you can decide whether you would like to merge the AttributeSet PR or not, and then close this. I see it as a fairly solid implementation of the LwM2m spec on attributes, so if there isn't any major changes planned for attributes in the next LwM2m releases, this could have a value even though it's not used anywhere yet.

We're still using read requests on individual resources as a method of discover, since we don't want to add real discover in our backend until we have full simulation support for it. For now, this works fairly well, which is why this haven't been prioritized the last few months.

It was a while since I worked actively on this, but I'll try to summarize my concerns for the future:

Link not being allowed to reference AttributeSet? I'm having a very hard time understanding this reasoning. It could be a very convenient first use case that is basically implemented already.
No mocking framework allowed in the tests? This really made me confused, it seems like an extremely uncontroversial addition to me.
I've had a very clear and rather advanced use case for the client parts of Leshan (our simulator), but making progress in the contributions feels hard and time-consuming. This might very well be me not being used to participating in open source projects, but if it's new functionality that's being added to fulfill the object specification (attribute handling and before also with the object versioning), it feels a bit discouraging to end up in implementation detail discussions like whether to put the attribute support in BaseObjectEnabler or ObjectEnabler. This can quite easily be corrected later, if we make a suboptimal decision in the first implementation.

I hope that we will find time in our project to prioritize Discover again later, then I will revisit this and try to break it up into smaller PRs.

sbernard31 · 2018-06-21T15:38:59Z

Ok, thx a lot for @danielhqv for the clarification.
I still think that the minimum set of feature to start to integrate this in master is :

add attributeSet
use attributeSet for writeAttributeRequests
remove deprecate ObserveSpec
If I find time, I will try to take your work and integrate at least this part in master.

making progress in the contributions feels hard and time-consuming

I try to give valuable feedback as soon as possible, this is time-consuming to me too.
It seems you still feel that sometime my remarks are nitpicking, I'm sorry about that. I just share my ideas, It seems better to me than just integrate your works then rewrite it.

Anyway thx a lot for all your contributions. See you in few months ;)

sbernard31 · 2018-07-12T13:46:45Z

I finally find time to integrate a part of your work in #533.

So to sump-up :

add attributeSet => Add new classes Attribute and AttributeSet to replace the old ObserveSpec #533
use attributeSet for writeAttributeRequests. => Add new classes Attribute and AttributeSet to replace the old ObserveSpec #533
remove deprecate ObserveSpec => Add new classes Attribute and AttributeSet to replace the old ObserveSpec #533
add support of writte attribute at client side => partially done in this PR but not yet integrated
enhance support of discover to return attributes. => partially done in this PR but not yet integrated

About discover, it was fixed to support optional resource but still not support attributes #532.

sbernard31 · 2021-10-29T15:25:36Z

I close this PR as it will not be integrated in this state.

I think someone which would like to implement #534 could be interested by this PR and so he will find a link to it.

discover and tests

2d1602c

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

danielhqv commented Mar 7, 2018

View reviewed changes

sbernard31 reviewed Mar 7, 2018

View reviewed changes

danielhqv added 2 commits March 9, 2018 15:40

added attribute

5462343

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

Merge branch 'attribute' into client_discover

cc8dca4

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

updates after code review

0814e97

Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>

danielhqv force-pushed the client_discover branch from 21bfc14 to 0814e97 Compare March 23, 2018 14:59

sbernard31 mentioned this pull request Apr 4, 2018

Queue Mode Integration Tests #487

Closed

sbernard31 force-pushed the master branch 2 times, most recently from a8f1ad0 to b6da9cc Compare June 11, 2018 13:41

sbernard31 mentioned this pull request Jul 12, 2018

Add new classes Attribute and AttributeSet to replace the old ObserveSpec #533

Merged

This was referenced Jul 13, 2018

DiscoverResponse should represent exist resources. #530

Closed

Add support of write attributes at client side. #534

Open

sbernard31 mentioned this pull request Nov 15, 2018

observe and attribute for leshan client #617

Closed

sbernard31 force-pushed the master branch from 843c5dc to 4ad71b1 Compare December 4, 2018 14:54

sbernard31 force-pushed the master branch 2 times, most recently from a0a98f6 to 6ac723c Compare September 18, 2019 08:57

sbernard31 added the orphan PR with no more activity label Nov 22, 2019

sbernard31 force-pushed the master branch 3 times, most recently from c455ed1 to c70bd8d Compare August 24, 2020 13:15

sbernard31 marked this pull request as draft October 29, 2021 15:06

sbernard31 closed this Oct 29, 2021

		return instance.write(path.getResourceId(), (LwM2mResource) request.getNode());
		}

Conversation

danielhqv commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernard31 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhqv commented Mar 9, 2018

Uh oh!

sbernard31 commented Mar 19, 2018

Uh oh!

danielhqv commented Mar 23, 2018

Uh oh!

sbernard31 commented Apr 5, 2018

Uh oh!

danielhqv commented Apr 10, 2018

Uh oh!

danielhqv commented Jun 20, 2018

Uh oh!

sbernard31 commented Jun 21, 2018

Uh oh!

sbernard31 commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbernard31 commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sbernard31 left a comment •

edited

Loading

sbernard31 commented Jul 12, 2018 •

edited

Loading