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

READ RESPONSE with timestamped SenML #1553

Closed
JaroslawLegierski opened this issue Nov 30, 2023 · 42 comments
Closed

READ RESPONSE with timestamped SenML #1553

JaroslawLegierski opened this issue Nov 30, 2023 · 42 comments
Labels
question Any question about leshan

Comments

@JaroslawLegierski
Copy link
Contributor

JaroslawLegierski commented Nov 30, 2023

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
Copy link
Contributor

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 😞.

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
Copy link
Contributor

(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
Copy link
Contributor Author

JaroslawLegierski commented Dec 1, 2023

(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 😉 , 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
Copy link
Contributor

sbernard31 commented Dec 1, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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. 🙂
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 : #1553 (comment)
And I think this is reasonable to think that time is supposed to be used only with historical representation.
(see : OpenMobileAlliance/OMA_LwM2M_for_Developers#538 (comment))

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
Copy link
Contributor

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

@sbernard31
Copy link
Contributor

I'm not sure I get what you mean ?

@mgdlkundera
Copy link
Contributor

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
Copy link
Contributor

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) ? #1553 (comment)

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
Copy link
Contributor

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
Copy link
Contributor

sbernard31 commented Feb 23, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

  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
Copy link
Contributor

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

@mgdlkundera
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

I look at it and as I explain at #1553 (comment), 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
Copy link
Contributor

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

master...JaroslawLegierski:leshan:read_response_senml

@sbernard31
Copy link
Contributor

I take a very quick look at it.

Some remarks at first sight :

  1. Following READ RESPONSE with timestamped SenML #1553 (comment), 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
Copy link
Contributor

A PR related to that feature is available at : #1610

@mgdlkundera
Copy link
Contributor

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

@sbernard31
Copy link
Contributor

And me from 29.04 to 01.05 (back 02.05) 😉

@sbernard31
Copy link
Contributor

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

@sbernard31
Copy link
Contributor

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
Copy link
Contributor

Thx all for your contributions 🙏

@slaft
Copy link

slaft commented Jun 5, 2024

To continue some discussion that started here #1612 (comment).

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
Copy link
Contributor

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 : OpenMobileAlliance/OMA_LwM2M_for_Developers#538 (comment))

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
Copy link
Contributor

sbernard31 commented Jun 6, 2024

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 🕵️

@sbernard31
Copy link
Contributor

@slaft, at #1612 (comment), 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 #1553 (comment)) 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
Copy link

slaft commented Jun 6, 2024

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
Copy link
Contributor

Please allow me some time to confirm this.

No problem 🙂

sbernard31 added a commit that referenced this issue Jun 7, 2024
-fix list order getTimestampedLwM2mNodes when null-timestamp value is
used : null-timestamp value should be first (as considered as most
recent one)

- when "Notification Storing When Disabled or Offline" is used
getTimestampedLwM2mNode should return the most recent value. So this
could be a null-timestamp one.

see : #1553 (comment)
@sbernard31
Copy link
Contributor

Some clarification about what I think of timestamp usage in LWM2M : #1623

sbernard31 added a commit that referenced this issue Jun 17, 2024
-fix list order getTimestampedLwM2mNodes when null-timestamp value is
used : null-timestamp value should be first (as considered as most
recent one)

- when "Notification Storing When Disabled or Offline" is used
getTimestampedLwM2mNode should return the most recent value. So this
could be a null-timestamp one.

see : #1553 (comment)
@mgdlkundera
Copy link
Contributor

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
Copy link
Contributor

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

Note that I'm currently working on a big rename(#1295), so we can discuss about that but I do not plan to integrate changes during that work.

@mgdlkundera
Copy link
Contributor

mgdlkundera commented Jun 26, 2024

I'm talking about method validateNoTimestampedRecord() in LwM2mNodeSenMLDecoder :

protected void validateNoTimestampedRecord(LwM2mResolvedSenMLRecord resolvedRecord) {
if (resolvedRecord.getTimeStamp() != null) {
throw new CodecException("Unable to decode node[path:%s] : value should not be timestamped",
resolvedRecord.getPath());
}
}

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

@sbernard31
Copy link
Contributor

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 : #1623)

@mgdlkundera
Copy link
Contributor

Version A is fine for us

@sbernard31
Copy link
Contributor

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
Copy link
Contributor

I forgot to let you know that refactoring is done (#1628) and so you can work on that if wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Any question about leshan
Projects
None yet
Development

No branches or pull requests

4 participants