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

Discover on instance level not implemented? #369

Closed
bolddp opened this issue Aug 23, 2017 · 17 comments
Closed

Discover on instance level not implemented? #369

bolddp opened this issue Aug 23, 2017 · 17 comments
Labels
client Impact LWM2M client new feature New feature from LWM2M specification

Comments

@bolddp
Copy link

bolddp commented Aug 23, 2017

If my Lwm2m server implementation needs to check if the connected device supports the Factory Reset resource on the Device object, my understanding is that I should perform a Discover on the Device Instance, e.g. /3/0. Then I need to examine the returned resource links to see if /3/0/5 is present. Is this correct?

If the answer is Yes, Discover doesn't seem to be implemented on the Instance level, e.g. in the LwM2mInstanceEnabler interface. Is this correct, or am I looking in the wrong place?

If it isn't implemented yet, is anybody working on it? If not, I might have a look at it myself and then some pointers on where to call the LwM2mInstanceEnabler.discover method would be much appreciated!

@sbernard31
Copy link
Contributor

Hi,
As I understand the spec, the way you propose is correct.
I think a discover on 3/0/5 should work too (205 for supported, 4.04 for not supported), but it's not so clear in the specification. You should ask at OMA.

Currently the Leshan Client discover implementation is only based on model description.
(See BaseObjectEnabler).
So using a custom model is a possible workaround. (See ObjectInitializer )
If you want a more dynamic behavior, you should implement your own LwM2mObjectEnabler.

As far as I know nobody is working on that. If you want to provide a PR for Leshan you're welcome :).
I suppose a discover method can be added to LwM2mInstanceEnabler and should used in ObjectEnabler.
The questions are :

  • should we still use the model ?
  • should we keep the default behavior with model if user didn't implement discover in LwM2mInstanceEnabler ?

JFYI :

  • LwM2mObjectEnabler is the interface which must be implemented. This is the real contract.
  • BaseObjectEnabler is an abstract class which can be used as base for a LwM2mObjectEnabler implementation.
  • ObjectEnabler is a possible implementation which chose to match LwM2m object instance concept to LwM2mInstanceEnabler interface.

HTH

@sbernard31 sbernard31 added client Impact LWM2M client new feature New feature from LWM2M specification labels Aug 28, 2017
@bolddp
Copy link
Author

bolddp commented Aug 29, 2017

@sbernard31 Thx for the detailed feedback! We are using the LeshanClient for a simulator at the moment, and it is in our interest that it can discover on instance level so it behaves according to spec.
When we implement this, I will try to solve it in a way that is general enough for a PR.

@sbernard31
Copy link
Contributor

Ok good to know :)

is it an open source simulator ?

@bolddp
Copy link
Author

bolddp commented Aug 29, 2017

Unfortunately not, it's proprietary. It's designed to simulate Lwm2m clients that comply with a messaging protocol that we're building on top of Lwm2m.

@danielhqv
Copy link
Contributor

I'm ready to have a new go at this now, it doesn't seem like anybody else is working on it? What you wrote above still applies, as far as I can see, e.g. Discover is currently just based on the ObjectModel and would need to be extended all the way down to LwM2mInstanceEnabler.

@danielhqv
Copy link
Contributor

danielhqv commented Feb 23, 2018

After looking into the specification a little more, I will also need to take a look at the handling of attributes on object and instance level, since they should be in Discover on all levels.

@sbernard31
Copy link
Contributor

Sry for delayed response.

Yep, nobody is currently working on that, so you can go on it.
About "attributes", currently the clients does support any attributes except the version one you added. Am I wrong ?

@danielhqv
Copy link
Contributor

You're right, and since the attributes must be reported in the Discover response along with the links, I will also have a look at any improvements that can be made to the attribute handling.

What exists is basically the ObserveSpec class, that represents the NOTIFICATION class attributes in the Lwm2m spec. But it's more optimized for serving the CoAP layer, e.g. through the toQueryParams() method.

ObserveSpec is also used in the WriteAttributesRequest constructors. It serves as a wrapper around some rules that are mentioned in the spec, e.g. throw an error if pmin > pmax.

I will open a pull request as soon as I have something worth looking at and then work on from there.

@sbernard31
Copy link
Contributor

Do you need support of attributes too ? I mean we can envisage a better support of Discover at instance level without support of attributes in a first time.

If you need both and those 2 features are not too interwoven. It will be easier to review if we make this step by step.

@danielhqv
Copy link
Contributor

danielhqv commented Feb 26, 2018

You might be right that it would be better to split these features up, but then I suppose we should reverse the order and start with attribute support, which is perhaps what you mean as well?

I'm just a little afraid that it will be hard to understand the attribute requirements fully without looking into the Discover functionality requirements at the same time. For instance, if you perform Discover on a resource, the attributes on both the object level and instance level should be inherited and included in the DiscoverResponse. This means that a first implementation of only Discover on instance level may be hard to pull off too.

But I will try to find a way to split it up into separate units of work.

@sbernard31
Copy link
Contributor

I suppose we should reverse the order and start with attribute support

I don't know. To be honest I'm not sure to see the real interest about attributes. Especially as we don't support writteAttributes at client side.

But I let you investigate that :)

@danielhqv
Copy link
Contributor

I've looked into it a little further now. We rely heavily on observe in our solution, and to be able to control the data volumes that are generated it's imperative for us to be able to set the pmin and pmax attributes. And because of that it's also very important for us to be able to simulate both WriteAttributes and full Discover.

Please consider the following interface and implementations to handle attributes in a consistent way:

public interface Attribute {
    String getCoRELinkParam();
    Attachment getAttachment();
    Set<AssignationLevel> getAssignationLevels();
    AttributeClass getAttributeClass(); 
    AccessMode getAccessMode(); 
    Object getValue(); 
    boolean isWritable(); 
    boolean canBeAssignedTo(AssignationLevel assignationLevel);
}

Abstract base implementation


public abstract class BaseAttribute implements Attribute {

    private final String coRELinkParam;
    private final Attachment attachment;
    private final EnumSet<AssignationLevel> assignationLevels;
    private final AttributeClass attributeClass;
    private final AccessMode accessMode;
    private final Object value;
    
    protected BaseAttribute(String coRELinkParam, Attachment attachment, EnumSet<AssignationLevel> assignationLevels,
            AttributeClass attributeClass, AccessMode accessMode, Object value) {
        this.coRELinkParam = coRELinkParam;
        this.attachment = attachment;
        this.assignationLevels = assignationLevels;
        this.attributeClass = attributeClass;
        this.accessMode = accessMode;
        this.value = value;
    }

    
    public String getCoRELinkParam() {
        return coRELinkParam;
    }

    
    public Attachment getAttachment() {
        return attachment;
    }
    
    public Set<AssignationLevel> getAssignationLevels() {
        return assignationLevels;
    }

    public AttributeClass getAttributeClass() {
        return attributeClass;
    }


    public AccessMode getAccessMode() {
        return accessMode;
    }
   
    public Object getValue() {
        return value;
    }
    
    @Override
    public boolean isWritable() {
        return accessMode == AccessMode.W || accessMode == AccessMode.RW;
    }
    
    @Override
    public boolean canBeAssignedTo(AssignationLevel assignationLevel) {
        return assignationLevels.contains(assignationLevel);
    }
}

And then one implementation for each supported attribute:

public class MinimumPeriodAttribute extends BaseAttribute {

    protected MinimumPeriodAttribute(Integer value) {
        super("pmin", Attachment.RESOURCE,
                EnumSet.of(AssignationLevel.OBJECT, AssignationLevel.INSTANCE, AssignationLevel.RESOURCE),
                AttributeClass.NOTIFICATION, AccessMode.RW, value);
    }

}

My idea then is that when you create a WriteAttributesRequest, you inject a group of parameters in the constructor, and a rule engine makes sure that they're writable and that they're consistent, e.g. pmin < pmax etc.
You would also be able to validate that the attributes are written on the correct level, e.g. throw an error if there is an attempt to write "ver" on a resource.

@sbernard31
Copy link
Contributor

So you want to define a kind of model for Attributes (I means objects containing metadata about attributes) A bit like we do for LwM2mModel ?
I'm not totally convinced but let's see what it will looks like.

FMPOV if we go in those way the model should not contains the value (or this instance should not contains the metadata). I mean we need only 1 instance in memory which describes what is the MinimumPeriodAttribute metadata and several instance of MinimumPeriodAttribute with theirs value.
Do you see what I mean ?

@danielhqv
Copy link
Contributor

OK, I see your point about duplicated metadata. I'll take that into consideration moving forward.

@danielhqv
Copy link
Contributor

Pull request for this... #478

@rkimsb2
Copy link
Contributor

rkimsb2 commented Jun 21, 2018

+1

@sbernard31
Copy link
Contributor

The orginal topic was about "Discover on instance level not implemented?".
I think this is done in #532.

About write attributes work is still in progress so I create an issue for this #534.

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
Projects
None yet
Development

No branches or pull requests

4 participants