eclipse-leshan / leshan

Java Library for LWM2M
https://www.eclipse.org/leshan/
BSD 3-Clause "New" or "Revised" License
647 stars 407 forks source link

READ RESPONSE with timestamped SenML #1553

Closed JaroslawLegierski closed 3 months ago

JaroslawLegierski commented 9 months ago

Question

We have some lwm2m devices using standard IoTerop SDK which are sending a response to a READ using SenML with a timestamp value inside (timestamp from SenML specification).

Leshan server DOES NOT accept this type of responses. Leshan server accept it only from a SEND or a NOTIFY, but not from a READ response:

org.eclipse.leshan.core.node.codec.CodecException :
Unable to decode node[[path:/3300/0/5700](http://path/3300/0/5700)] : value should not be timestamped

Is it possible to allow such payload with an embedded timestamp from READ responses ?

sbernard31 commented 9 months ago

This is another part of LWM2M specification which is unclear to me for which I have been unable to obtain clarification.

See relative OMA and Leshan issue about that :

More generally, I tried several times to interact with the OMA authors to find a solution to this global situation where we failed to get an answer to our LWM2M misunderstanding, without success ... I'm not even able to get answer using private mail ... this is like this since several years now and I must confess the situation become really frustrating :disappointed:.

Let's try to better understand together what could means supporting ReadResponse with timestamp.

Currently, I understand a read request is about sending current value on the device. With this is mind, it's hard to me to understand how timestamp can be used on Read.

Do you have use case in mind ? What exactly did IoTerop SDK ?

sbernard31 commented 9 months ago

(In your log there is something strange : [path:/3300/0/5700](http://path/3300/0/5700)], I don't get what this is this http is that a Leshan log bug ?)

JaroslawLegierski commented 9 months ago

(In your log there is something strange : [path:/3300/0/5700](http://path/3300/0/5700)], I don't get what this is this http is that a Leshan log bug ?)

This is not bug - this log is from our backend :wink: , but in leshan server we also observing this error:

Caused by: org.eclipse.leshan.core.node.codec.CodecException: 
Unable to decode node[path:/3303/0/5700] : value should not be timestamped

We agree that specification does not tell anything specific on READ response, excepted a mention that " ...are not supported by the LwM2M Server MUST be silently ignored."

But specification in the SenML chapter does not make any difference, and says "When a LwM2M Client is supporting the SenML JSON data format and the format is used to transport Object Instance(s), multiple resource and single resource values for both "Read" and "Write" operations, SenML JSON payload MUST use the format defined in this section. This format MAY be used also for transporting a single value of a Resource." and it does not says that it is forbidden to use "t" or "bt" on READ response, so it should be allowed.

sbernard31 commented 9 months ago

We agree that specification does not tell anything specific on READ response, excepted a mention that " ...are not supported by the LwM2M Server MUST be silently ignored."

The whole sentence is : "Any optional resources included in the "Read" operation response that are not supported by the LwM2M Server MUST be silently ignored.." (source : LWM2M-v1.1.1@core§6.3.1. Read Operation)

So this talk about optional resource from LWM2M model that server could not know so this is not relevant here.

it does not says that it is forbidden to use "t" or "bt" on READ response, so it should be allowed.

Even if I understand your argument, it can also be used to say that t and bt can be used in SENML for Write request because "this is not forbidden so this is allowed". But for me this doesn't make sense to put timestamp in write request. So I don't think this is a strong argument.


So from my point of view specification/authors doesn't help us to know if this should be supported and how this should be supported.

My current point of view on read is that it doesn't have so much sense but it seems we disagree so I would like to better understand you.

Is it possible to allow such payload with an embedded timestamp from READ responses ?

Could you precise the behavior/API changes you would like to see in Leshan ? and also you didn't answer to : Do you have use case in mind ? What exactly did IoTerop SDK ? (I mean what is the meaning of this timestramp ?)

mgdlkundera commented 7 months ago

This topis is transferred from Jarek to me 🙂

From Technical point of view: When there was only lwm2m 1.0, senML transport format did not exists in lwm2m, and the only way to send a "measure timestamp" was to use specific timestamp lwm2m resource (ex: object 3300 resource 5518). But with lwm2m 1.1 senML transport format allows a device to send the "measure timestamp" at any time, and for all objects, (not only the one where a specific "timestamp" resource exists). That is why also in Orange use case we also need to get the "measure timestamp" when doing just a READ.

From business case point of view: Battery-powered devices allow for adjusting sensor sampling frequency and transmission frequency to optimize power consumption. Message and measurement timestamps may differ. The timestamp of the measurement returned by a READ is important because it allows us to determine the age of the measurement taken by the device. This helps us to better correlate it with the analysis context. For instance, if an electricity meter value received at 5pm corresponds to a measurement taken at midday, it is normal to have only half the daily electricity consumption. This is the case for instance with Adeunis Comfort Serenity device: https://www.adeunis.com/en/produit/temperature-humidity-co2-voc-iot-sensor/

sbernard31 commented 7 months ago

When there was only lwm2m 1.0, senML transport format did not exists in lwm2m, and the only way to send a "measure timestamp" was to use specific timestamp lwm2m resource (ex: object 3300 resource 5518).

Not exactly true, I think we can consider we have exactly same "question" with old JSON format (application/vnd.oma.lwm2m+json, 11543) So this could also be a lwm2m 1.0 question.

But with lwm2m 1.1 senML transport format allows a device to send the "measure timestamp" at any time, and for all objects, (not only the one where a specific "timestamp" resource exists)

I disagree on that point. :slightly_smiling_face: The specification says nothing clear which allow to do that. I don't think that "this is not forbidden so this is allowed" is a good argument here. I explained why above : https://github.com/eclipse-leshan/leshan/issues/1553#issuecomment-1836128770 And I think this is reasonable to think that time is supposed to be used only with historical representation. (see : https://github.com/OpenMobileAlliance/OMA_LwM2M_for_Developers/issues/538#issuecomment-1924064487)

Now, I understand that we can have different interpretation as the specification is pretty ambiguity and I also understand your use case.


So now let's try to imagine how we could add support of that in Leshan Would it be OK if we support it like we support it with Observe but you can only have 1 timestamped node ?

So if we read a resource, you get a only 1 TimestampedLwM2mNode with a LwM2mResource as node. That's also mean if you read an object you get only 1 TimestampedLwM2mNode with a LwM2mObject as node. So timestamp should be the same for the whole object.

With an example with :

// A) This is OK, because there is 1 timestamp for the whole node
[
    { "bn":"/6/0/","n":"0","v":-21.0, "bt":1706888656},
    {"n":"1","v":141.0},
    {"n":"5","v":1706888656}
]
// B) This is not OK because there is several measurement timestamp in the same node : 
[
    { "bn":"/6/0/","n":"0","v":-21.0, "bt":1706888656}, 
    {"n":"1","v":141.0, "t":"10"}, 
    {"n":"5","v":1706888656}
]

If we want to support A) with current state of Leshan this should be doable. If we want to support B) this will break a lot of code and probably will need lot of re-design... I'm not sure we should do that...

mgdlkundera commented 6 months ago

I wanted to ask what is your point of view on how to simulate it?

sbernard31 commented 6 months ago

I'm not sure I get what you mean ?

mgdlkundera commented 6 months ago

I mean what is your opinion on how should I implement it, can I make changes at client side or should I create some test at the begining?

sbernard31 commented 6 months ago

Should we first agree about what we want to try to implement ? I didn't get if A) is fine for you ? or if you need B) ? https://github.com/eclipse-leshan/leshan/issues/1553#issuecomment-1924153991

I mean what is your opinion on how should I implement it

I understand you want to work on this now ? Does it mean that you will stop to work on write_attribute ?

mgdlkundera commented 6 months ago

Sorry, I didn't mention it earlier. We would like to implement A) - only with 1 timestamp. I will work on this issue and also on write_attribute

sbernard31 commented 6 months ago

Sorry, I didn't mention it earlier. We would like to implement A) - only with 1 timestamp.

OK so we could try to implement that :

  1. changing ReadResponse API. (inspired by ObserveResponse)
  2. implement the feature at server side (optionally with integration tests using LockStepLwM2mClient)
  3. decide IF we want to add a way to make leshan client send timestamp on read request and how it could looks like

But before to start to work on this, As Leshan is probably still pretty new for you, are you sure its a good idea for you to work on 2 task at same time ? Should it be better to focus on 1 then once it's done move on a new one ?

mgdlkundera commented 6 months ago

Maybe it's good idea. Because of our priorities I'll switch to this issue and when I finish I'll back to write attributes, ok?

sbernard31 commented 6 months ago

Because of our priorities I'll switch to this issue and when I finish I'll back to write attributes, ok?

Ok, no problem.

mgdlkundera commented 6 months ago
  1. implement the feature at server side (optionally with integration tests using LockStepLwM2mClient)

Is it possible to use LockStepLwM2mClient test without modification at client side?

sbernard31 commented 6 months ago

LockStepLwM2mClient is not really based on LeshanClient. It is mainly a way to receive and send CoAP message + some LWM2M facilities.

mgdlkundera commented 5 months ago

Can you help me with test read response using LockStepLwM2mClient? I looked through observe and observe composite tests (there is a hack with triggering notification which I can't make in read case). I don't know how to simulate read response with time stamp, could you give me some tips or example?

sbernard31 commented 5 months ago

I didn't investigate too much but maybe something like :

// Do all the stuff about registering

// Server send Read Request

// Wait for server READ request and store CoAP Token and MID
client.expectRequest().storeToken("TKN").storeMID("MID").go();

// Send custom READ Response
// here you need to create the Coap Response and its payload on your own (you can not use LWM2M API)
// I see that maybe it miss a way to add content format an so maybe you need to add it.
client.sendResponse(Type.ACK, ResponseCode.CONTENT).loadMID("MID").loadToken("TKN")
                .payload(givenServerEndpointProvider).go();               

Let me know if help is enough.

mgdlkundera commented 5 months ago

Hi, I still have a problem, currently response is a null. Here is a link to my changes and test in LockStepTest: https://github.com/JaroslawLegierski/leshan/blob/d044723a764e21a1cad7b330a23576c5625d1be5/leshan-integration-tests/src/test/java/org/eclipse/leshan/integration/tests/lockstep/LockStepTest.java

Maybe I'm still doing something wrong.

sbernard31 commented 5 months ago

I look at it and as I explain at https://github.com/eclipse-leshan/leshan/issues/1553#issuecomment-2012547932, you should probably first send the Read Request before to call expectRequest because this is sync API. There is an async API to send request. (server.send())

Let me know if this solve your problem.

mgdlkundera commented 5 months ago

Could you take a look at my changes, I think it is almost finished. Here is a link:

https://github.com/eclipse-leshan/leshan/compare/master...JaroslawLegierski:leshan:read_response_senml

sbernard31 commented 4 months ago

I take a very quick look at it.

Some remarks at first sight :

  1. Following https://github.com/eclipse-leshan/leshan/issues/1553#issuecomment-1924153991, I think that ReadResponse constructor should have a TimestampedLwM2mNode instead of List<TimestampedLwM2mNode> as argument?
  2. Please do not use merge commit, use cherrypick or rebase instead.

    If you want more details review, you can open a PR (but please no merge commit)

sbernard31 commented 4 months ago

A PR related to that feature is available at : https://github.com/eclipse-leshan/leshan/pull/1610

mgdlkundera commented 4 months ago

Hi, I'm out of office from 29.04 to 05.05. I'll be back on moday (06.05).

sbernard31 commented 4 months ago

And me from 29.04 to 01.05 (back 02.05) :wink:

sbernard31 commented 4 months ago

And re-out of office from 06.05 to 14.05 (back 15.05)

sbernard31 commented 3 months ago

I think we can close this now as this is implemented in master and will be available in 2.0.0-M15. (see #1610)

sbernard31 commented 3 months ago

Thx all for your contributions :pray:

slaft commented 3 months ago

To continue some discussion that started here https://github.com/eclipse-leshan/leshan/issues/1612#issuecomment-2150107397.

This kind of payloads is not accepted in a Read and initial Observe response (with the error: should receive only 1 timestamped node but received 2):

[
  {"bn":"/3442/0/","n":"120","t":1717150530.123456789,"v":1},
  {"n":"110","vs":"a"}
]

But it is in a Notify, and is decoded as this ObserveReponse:

in M15:

content = LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]
timestampedValue = TimestampedLwM2mNode [timestamp=2024-05-31T10:15:30.123456789Z, node=LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]]
timestampedValues = [TimestampedLwM2mNode [timestamp=2024-05-31T10:15:30.123456789Z, node=LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]], TimestampedLwM2mNode [timestamp=null, node=LwM2mObjectInstance [id=0, resources={110=LwM2mSingleResource [id=110, value=a, type=STRING]}]]]

in M14:

content = LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]
timestampedValues = [TimestampedLwM2mNode [timestamp=2024-05-31T10:15:30.123456700Z, node=LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]], TimestampedLwM2mNode [timestamp=null, node=LwM2mObjectInstance [id=0, resources={110=LwM2mSingleResource [id=110, value=a, type=STRING]}]]]
sbernard31 commented 3 months ago

This kind of payloads is not accepted in a Read and initial Observe response (with the error: should receive only 1 timestamped node but received 2

I guess this is about Read/Observe an object instance /3442/0.

Leshan API was thought with idea that when you try to Read/Observe a object instance, you get an LwM2mObjectInstance which should represent current value of that node at client side.

But there is another feature in LWM2M called : Notification Storing When Disabled or Offline and which allow client to store notification when offline and sent it later when online.

E.g. :

  1. Server observes /3442/0
  2. Client send current value of /3442/0 without timestamp.
  3. Client goes offline
  4. An event should generate a notification, but client is offline so it store object instance value and timestamp it.
  5. An other event, it does the same, ... , ...
  6. Client goes online and send an update request then the notification with historical values.

(you can read : https://github.com/OpenMobileAlliance/OMA_LwM2M_for_Developers/issues/538#issuecomment-1924064487)

Leshan API is adapted to support that. And so can handle timestamped value but only for notification. (to support when historical value is used because of Notification Storing When Disabled or Offline.

In your case

[
  {"bn":"/3442/0/","n":"120","t":1717150530.123456789,"v":1},
  {"n":"110","vs":"a"}
]

Leshan understand that it receives a notification with historical value which means : At 1717150530.123456789, object instance has only a value for the resource 120 which was 1 (it think that other resource had no value at this time) Then just now (no timestamp), object instance has now just a value for resource 110 which is "a" (it think that other resource had no value at this time)

But this is not what the client try to express. I suppose it want to mean which have a new value for instance /3442/0 with 2 resources 120=1 and 110=a and 120 was measured at 1717150530.123456789 and we don't know when 110 was measured (or this has maybe no sense)

From my point of view, that LWM2M client uses badly timestamp in SENML This is what I try to explain before. Still from my point of view, LWM2M specification was not thought to be used like this.

And I think the fact that some object model has a Timestamp Resource which means :

The timestamp of when the measurement was performed.

which is added only for resources which are sensor measurement is a hint that timestamps in SENML in LWM2M should not really used for that.

sbernard31 commented 3 months ago

in M15:

content = LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]
timestampedValue = TimestampedLwM2mNode [timestamp=2024-05-31T10:15:30.123456789Z, node=LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]]
timestampedValues = [TimestampedLwM2mNode [timestamp=2024-05-31T10:15:30.123456789Z, node=LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]], TimestampedLwM2mNode [timestamp=null, node=LwM2mObjectInstance [id=0, resources={110=LwM2mSingleResource [id=110, value=a, type=STRING]}]]]

It seems to me that timestampedValue has not the right value.

The javadoc says :

/**
 * {@inheritDoc}
 *
 * <p>
 * In case where client is using "Notification Storing When Disabled or Offline", the most recent value is returned.
 * If you want the full historical timestamped values, you should use use {@link #getTimestampedLwM2mNodes()}.
 */
@Override
public TimestampedLwM2mNode getTimestampedLwM2mNode() {
    return super.getTimestampedLwM2mNode();
}

And so most recent value (is no timestamp which means now) and so we should have :

timestampedValue =
- TimestampedLwM2mNode [timestamp=2024-05-31T10:15:30.123456789Z, node=LwM2mObjectInstance [id=0, resources={120=LwM2mSingleResource [id=120, value=1, type=INTEGER]}]]
+ TimestampedLwM2mNode [timestamp=null, node=LwM2mObjectInstance [id=0, resources={110=LwM2mSingleResource [id=110, value=a, type=STRING]}]

I need to investigate that :detective:

sbernard31 commented 3 months ago

@slaft, at https://github.com/eclipse-leshan/leshan/issues/1612#issuecomment-2150404184, you said

Perhaps simply ignoring timestamps would be an OK option

I can also look if making that check optional is easily doable ? Are you sure this is OK for you to just loose timestamp information ?

The other solution (called B at https://github.com/eclipse-leshan/leshan/issues/1553#issuecomment-1924153991) which supports :

[
 {"bn":"/3300/0/","n":"5700","t":1717505820,"v":529},
 {"n":"5601","v":529},
 {"n":"5602","v":720},
 {"n":"5701","vs":"ppm"},
 {"n":"5603","v":0},
 {"n":"5604","v":3e4},
 {"n":"5750","vs":"CO2 "}
]

will probably very impacting and will need maybe a lot of thinking especially how it lives with Notification Storing When Disabled or Offline ? I'm really not sure this will be a good move.

slaft commented 3 months ago

Thanks a lot for the detailed explanations!

I guess this is about Read/Observe an object instance /3442/0.

Yes

I suppose it want to mean which have a new value for instance /3442/0 with 2 resources 120=1 and 110=a and 120 was measured at 1717150530.123456789 and we don't know when 110 was measured (or this has maybe no sense)

Yes I suppose too

I can also look if making that check optional is easily doable ? Are you sure this is OK for you to just loose timestamp information ?

Please allow me some time to confirm this.

sbernard31 commented 3 months ago

Please allow me some time to confirm this.

No problem :slightly_smiling_face:

sbernard31 commented 3 months ago

Some clarification about what I think of timestamp usage in LWM2M : https://github.com/eclipse-leshan/leshan/issues/1623

mgdlkundera commented 2 months ago

Hello again Simon, I have one more question, can we do a small fix regarding read and observe composite operation with 1 timestamp for all? I look through the code and there is an if statement in LwM2mNodeSenMLDecoder, when there is a timestamped value it throw exception for observe operation. Can we try to work on this?

sbernard31 commented 2 months ago

Hi, I'm not sure I get your point :thinking:. Can you provide a link to the code ?

Note that I'm currently working on a big rename(https://github.com/eclipse-leshan/leshan/issues/1295), so we can discuss about that but I do not plan to integrate changes during that work.

mgdlkundera commented 2 months ago

I'm talking about method validateNoTimestampedRecord() in LwM2mNodeSenMLDecoder : https://github.com/eclipse-leshan/leshan/blob/2683ca6826d627d6410c1c0e23e0b9d3a9e1154f/leshan-core/src/main/java/org/eclipse/leshan/core/node/codec/senml/LwM2mNodeSenMLDecoder.java#L174-L179

When we try do observe with 1 timpestamp, we can't retrive response, because of Exception - "Unable to decode node..."

sbernard31 commented 2 months ago

OK and so you would like to support to be able to have 1 timestamp like for Simple (not composite) Read and Observe ?

So If you do a Composite-Read : /3/0/1 and /6/0, then you wanted to be able to have something like :

// This is OK 
[
    { "n":"3/0/1","v":11234, "bt":1706888656 },
    { "bn":"/6/0/","n":"0","v":-21.0 }, 
    { "n":"1","v":141.0 }, 
    { "n":"5","v":1706888656 }
]

// This is NOT OK  :
[
    { "n":"3/0/1","v":11234, "bt":1706888656 },
    { "bn":"/6/0/","n":"0","v":-21.0  "bt":1706880000 }, 
    { "n":"1","v":141.0 }, 
    { "n":"5","v":1706888656 }
]

(for same : https://github.com/eclipse-leshan/leshan/issues/1623)

mgdlkundera commented 2 months ago

Version A is fine for us

sbernard31 commented 2 months ago

OK so probably we need to create a dedicated issue describing the need. I try to finish rename refactoring soon and I will let you know when this is ready.

sbernard31 commented 2 months ago

I forgot to let you know that refactoring is done (https://github.com/eclipse-leshan/leshan/pull/1628) and so you can work on that if wanted.

JaroslawLegierski commented 1 month ago

OK so probably we need to create a dedicated issue describing the need. I try to finish rename refactoring soon and I will let you know when this is ready.

I created dedicated issue #1637