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

NIDD device support in leshan. #904

Closed
madhushreegc opened this issue Oct 8, 2020 · 27 comments
Closed

NIDD device support in leshan. #904

madhushreegc opened this issue Oct 8, 2020 · 27 comments
Labels
new feature New feature from LWM2M specification
Milestone

Comments

@madhushreegc
Copy link

madhushreegc commented Oct 8, 2020

Hi,

leshan version : 1.1.0

I have a usescase to integrate with the NIDD device.

  1. Binding Mode comes with the type N
  2. Content format is senml CBOR.
  3. Need to pass few parameters like externalId to the connector and then to proxy.

I have written encoder and decoder class to support cbor and own connector class.
Now, registration is failing because binding mode 'N' is not supported in leshan.

To support my usecase, following three changes are required in leshan .

  1. Add BindingMode N in BindingMode class of leshan-core.
  2. Add contentformat SENML_CBOR_CODE = 112 and SENML_JSON_CODE=110 in ContentFormat class of leshan-core.
  3. For every downlink , I am passing extra parameters in the context which are required to be passed to connector.
    If the context passed then we can have the same context to be passed to connector instead of creating new?

For example in method sendCoapRequest of RequestSender in leshan-server-cf ?

// Define destination
if(null == coapRequest.getDestinationContext()) {
      EndpointContext context = EndpointContextUtil.extractContext(destination, allowConnectionInitiation);
      coapRequest.setDestinationContext(context);
}

org.eclipse.leshan.server.californium.request.RequestSender

	public Response sendCoapRequest(Identity destination, String sessionId, Request coapRequest, long timeoutInMs,
            boolean allowConnectionInitiation) throws InterruptedException {

        // Define destination
    	if(coapRequest.getDestinationContext() == null) {
    		EndpointContext context = EndpointContextUtil.extractContext(destination, allowConnectionInitiation);
            coapRequest.setDestinationContext(context);
    	}

        // Send CoAP request synchronously
        CoapSyncRequestObserver syncMessageObserver = new CoapSyncRequestObserver(coapRequest, timeoutInMs);
        coapRequest.addMessageObserver(syncMessageObserver);

        // Store pending request to be able to cancel it later
        addOngoingRequest(sessionId, coapRequest);

        // Send CoAP request asynchronously
        if (destination.isSecure())
            secureEndpoint.sendRequest(coapRequest);
        else
            nonSecureEndpoint.sendRequest(coapRequest);

        // Wait for response, then return it
        return syncMessageObserver.waitForCoapResponse();
    }

   public void sendCoapRequest(Identity destination, String sessionId, Request coapRequest, long timeoutInMs,
            CoapResponseCallback responseCallback, ErrorCallback errorCallback, boolean allowConnectionInitiation) {

        Validate.notNull(responseCallback);
        Validate.notNull(errorCallback);

        // Define destination
    	if(coapRequest.getDestinationContext() == null) {
    		EndpointContext context = EndpointContextUtil.extractContext(destination, allowConnectionInitiation);
            coapRequest.setDestinationContext(context);
    	}
    	
        // Add CoAP request callback
        MessageObserver obs = new CoapAsyncRequestObserver(coapRequest, responseCallback, errorCallback, timeoutInMs,
                executor);
        coapRequest.addMessageObserver(obs);

        // Store pending request to be able to cancel it later
        addOngoingRequest(sessionId, coapRequest);
        
        // Send CoAP request asynchronously
        if (destination.isSecure())
            secureEndpoint.sendRequest(coapRequest);
        else
            nonSecureEndpoint.sendRequest(coapRequest);
    }

Could you please add these changes in leshan so that we can support nidd devices?

@sbernard31
Copy link
Contributor

This is LWM2M v1.1 feature so this should be added to 2.0.0 branch.
There will be frequent release but API will be probably break frequently.

Add BindingMode N in BindingMode class of leshan-core

I could add this to my priority list.

Add contentformat SENML_CBOR_CODE = 112 and SENML_JSON_CODE=110 in ContentFormat class of leshan-core

I plan to work on those new content format just after binding mode.
Do you want to share your encoder/Decoder ? is it inspired by #681 or LwM2mNodeSenMLJsonEncoder
Which JSON/CBOR library are you using ?

For every downlink , I am passing extra parameters in the context which are required to be passed to connector.
If the context passed then we can have the same context to be passed to connector instead of creating new?

I pretty sure I don't get it.
But can LowerLayerConfig do the job ?

@sbernard31 sbernard31 added the new feature New feature from LWM2M specification label Oct 8, 2020
@sbernard31 sbernard31 added this to the 2.0.0 milestone Oct 8, 2020
@madhushreegc
Copy link
Author

Yes, I can pass the cbor encoder/decoder but it does normal serialize/deserialize . It does not implement NodeEncoder . Let me know if this helps .
I have used jackson library.

com.fasterxml.jackson.dataformat
jackson-dataformat-cbor
${com.fasterxml.jackson}

I am directly framing coapRequest and setting destination context and sending.

       Request coapRequest = new Request(Code.GET);
		if(null != payload) {
			coapRequest = new Request(Code.FETCH);
			coapRequest.setPayload(payload);
		} 
		coapRequest.getOptions().setContentFormat(contentFormat);
		coapRequest.getOptions().setAccept(contentFormat);
		
		EndpointContext context = null;
		if(LeshanUtility.isNonIPTransport(registration)) {
			context = getEndpointContext(registration.getIdentity(), registration.getEndpoint());
		} 
      
                   coapRequest.setDestinationContext(context);
		
		LeshanServer leshanServer = leshanServerBean.getLeshanServer(registration.getBindingMode());
		Response response = leshanServer.coap().send(registration, coapRequest, SEND_TIMEOUT);

My downlink request looks like this . I am setting the endpointContext but it is always framed new in RequestSender class.

My request was to set the endpointContext only if it is null like below.

public void sendCoapRequest(Identity destination, String sessionId, Request coapRequest, long timeoutInMs,
CoapResponseCallback responseCallback, ErrorCallback errorCallback, boolean allowConnectionInitiation) {

    Validate.notNull(responseCallback);
    Validate.notNull(errorCallback);

    // Define destination
	if(coapRequest.getDestinationContext() == null) {
		EndpointContext context = EndpointContextUtil.extractContext(destination, allowConnectionInitiation);
        coapRequest.setDestinationContext(context);
	}
	
    // Add CoAP request callback
    MessageObserver obs = new CoapAsyncRequestObserver(coapRequest, responseCallback, errorCallback, timeoutInMs,
            executor);
    coapRequest.addMessageObserver(obs);

    // Store pending request to be able to cancel it later
    addOngoingRequest(sessionId, coapRequest);

@sbernard31
Copy link
Contributor

Yes, I can pass the cbor encoder/decoder but it does normal serialize/deserialize . It does not implement NodeEncoder . Let me know if this helps .
I have used jackson library.

We get recent discussion about that : #866 (comment)
And currently conclusion is to use com.upokecenter:cbor
Could you share why you choose : jackson-dataformat-cbor ?

I am directly framing coapRequest and setting destination context and sending.

I just get that you are sending coap request and not LWM2M request.
Is there any reason about that ?

I believe I get you point this time, if you set an endpoint context on your coap request you don't want leshan override it.
We could find a solution for this.
Maybe if I merge existing EnpointContext with new one ? (if there is conflict we keep user key + warn log?)

@madhushreegc
Copy link
Author

madhushreegc commented Oct 8, 2020

Maybe if I merge existing EnpointContext with new one ? (if there is conflict we keep user key + warn log?)

Yes this will be very helpful .

I just get that you are sending coap request and not LWM2M request.
Is there any reason about that?

Still composite operations are not supported , so I am using coapRequest.

Could you share why you choose : jackson-dataformat-cbor ?

We are using jackson for mapping json so using same for cbor as well.

@sbernard31
Copy link
Contributor

Add BindingMode N in BindingMode class of leshan-core.

Step 1 done. @madhushreegc, you could have a look at #908.

@sbernard31
Copy link
Contributor

sbernard31 commented Oct 20, 2020

@madhushreegc about :

Maybe if I merge existing EnpointContext with new one ? (if there is conflict we keep user key + warn log?)

I start some try. About key merging should not be an issue.
But an EndpointContext is also a peerAddress and some time a principal and a virtualhost.
So I'm not sure what could mean merging this ...
Maybe we need to give up this "merge idea" and go back to you simple "if not null" solution ?

@madhushreegc
Copy link
Author

@sbernard31 Yes, null check would be the good approach because user can pass their own endpointContext.

@sbernard31
Copy link
Contributor

@madhushreegc, I created #914.

There is a warning when you are using custom endpointcontext to avoid this is not done on purpose.
You can get rid of this warning by change log level of this class. Tell if this is OK for you or if we should introduce a way to remove warning by request (maybe by adding a specific key in the endpoint context ?)
(let's talk about that at #914)

@madhushreegc
Copy link
Author

@sbernard31, This is fine for me .

Thank you .

@madhushreegc
Copy link
Author

madhushreegc commented Nov 2, 2020

@sbernard31
I think this should be like this.

If it is not null, we are not doing anything and sending the context sent in request else Leshan is creating.

if clause should check == null like below.

if (**coapRequest.getDestinationContext() == null**) {
            EndpointContext context = EndpointContextUtil.extractContext(destination, allowConnectionInitiation);
            coapRequest.setDestinationContext(context);
        } else {
            LOG.warn(
                    "Destination context was not set by Leshan for this request. The context is used to ensure your talk to the right peer. Bad usage could bring to security issue. {}",
                    coapRequest);
        }

@sbernard31
Copy link
Contributor

Good catch. I thought I already fixed this but visibly I didn't push it ... 🤦
Thx to report it. (Do not hesitate to directly put the comment on the concerned PR next time 😉)

@sbernard31
Copy link
Contributor

sbernard31 commented Nov 2, 2020

So

We will probably need more step to add Composite operation. About that, which composite operation are you using ?

@madhushreegc
Copy link
Author

Ok . Thank you.

I am using all composite operations like composite-read, composite-write, composite-observe and cancel composite-observe .

@sbernard31
Copy link
Contributor

Hi, some updates :
step 1 : binding mode : done (#908),
step 2.1 SenML JSON : done (#911),
step 2.2 SenML Cbord : done (#922, #937)
step 3 : custom endpoint context : done (#914).

All of this should be available in 2.0.0-M2, so I think we can close this issue ?

I will probably release a 2.0.0-M2 either at the end of the week or after my vacation (begin of January).

Next feature I plan to work on will probably be "composite operation" or "send operation".
For composite operation, the first step will be to think about a consistent API 🤔

@madhushreegc
Copy link
Author

Yes,

we can close the case . Thank you so much.

@madhushreegc
Copy link
Author

madhushreegc commented Jun 9, 2021

Hi ,
Could you please add below line of code for CoapRequestBuilder setTarget methods as well as you did it for sendCoapRequest methods?

// Define destination
if (coapRequest.getDestinationContext() == null) {
    EndpointContext context = EndpointContextUtil.extractContext(destination, allowConnectionInitiation);
    coapRequest.setDestinationContext(context);
} else {
    LOG.warn(
            "Destination context was not set by Leshan for this request. The context is used to ensure you talk to the right peer. Bad usage could bring to security issue. {}",
            coapRequest);
}

If you add, I can use the sendLwm2mRequest method exposed for all single and composite downlink operations.

@madhushreegc madhushreegc reopened this Jun 9, 2021
@sbernard31
Copy link
Contributor

sbernard31 commented Jun 9, 2021

I'm not sure to get you 🤔

Previously in sendCoapRequest(),coapRequest.getDestinationContext() could be not null because you passed the request and so you could have set a context.

But in this case how coapRequest.getDestinationContext() could be not null ? how do you set the context ?

@madhushreegc
Copy link
Author

Yes, coapRequest.getDestinationContext() cannot be null because you are generating there, we have to make a provision to pass the destinationContext to this layer. I am using NIDD device , to support that I have to pass my own destinationContext to the connector .

@boaks
Copy link

boaks commented Jun 9, 2021

I have written encoder and decoder class to support cbor and own connector class.

If you have your own connector, maybe you implement also a EndpointContext and a EndpointContextMatcher for it. The available ones e.g. UdpEndpointContext and UdpEndpointContextMatcher may be a good point to start.

@sbernard31
Copy link
Contributor

sbernard31 commented Jun 9, 2021

Do you try to use LowerLayerConfig ? This should allow you to modify the CoAP request created.

See org.eclipse.leshan.server.californium.LeshanServer.send(Registration, DownlinkRequest<T>, LowerLayerConfig, long)

/**
 * ...
 * @param lowerLayerConfig to tweak lower layer request (e.g. coap request)
 * ...
 */

@sbernard31
Copy link
Contributor

If you have your own connector, maybe you implement also a EndpointContext and a EndpointContextMatcher for it. The available ones e.g. UdpEndpointContext and UdpEndpointContextMatcher may be a good point to start.

I guess this is pretty much what @madhushreegc did or maybe he just reuses existing one.
But in all case the issue for him is that Leshan create endpointContext "automagically", which is good most of time because this way users don't need to think about this.
But in his case, he need to create a custom endpointContext. Maybe LowerLayerConfig could help but probably the best way would be to think about a way to support new transport layer to Leshan based on californium (coap-tcp or NIDD) or some other framework. (for http or mqtt)

@sbernard31
Copy link
Contributor

@madhushreegc I created an issue to think about adding transport layer (see #1025)
If you want/can please share what you do to support your NIDD devices ?

@madhushreegc
Copy link
Author

madhushreegc commented Jun 10, 2021

Yes, We have implemented own connector with own EndpointContextMatcher. I am passing necessary details to connector via endPointContext for binding mode N .

@madhushreegc
Copy link
Author

I have not tried with LowerLayerConfig . I will try this . Thank you ..

@sbernard31
Copy link
Contributor

Ok let we know if it works with it 🙂

@sbernard31
Copy link
Contributor

Do you succeed to make it work ? should we close this issue ?

@madhushreegc
Copy link
Author

No, I did not try this . We can close now , I will try when I have time . Thank you.

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

No branches or pull requests

3 participants