search

eclipse-leshan / leshan

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

How to address a tagged CBOR number value not being deserialized as base time in SenMLRecord? #1631

Closed mjviljan closed 3 months ago

mjviljan commented 4 months ago

Question

Hi,

I have a meter that sends data encoded in SenML/CBOR. The first entry in an array of SenML records has a base time value, but it's tagged with tag 1 (i.e. for "Epoch-based date/time"). A minimal example of such a value:

[{-3: 1(1720696276)}]

The CBOR library Leshan uses doesn't recognize it as a number – because of that tag – thus making the BigDecimal-type base time value in the resulting deserialized SenMLRecord null.

This happens because the bt value of the CBORObject isn't considered a number, and it's expected to be one here: https://github.com/eclipse-leshan/leshan/blob/e673ac7533fad7bb75ead5ae295804874ec8e5d4/leshan-lwm2m-core/src/main/java/org/eclipse/leshan/senml/cbor/upokecenter/SenMLCborPackSerDes.java#L68

I know the underlying problem is in the CBOR library, and there has been an issue opened there about it. Unfortunately it has been closed as there's a workaround available. CBORNumber.IsNumber() is classifying many numbers as 'not number'

(I also noticed you had discussed support for Date values there and talked about tag 1: add support of Date type. — but as SenMLRecord uses BigDecimal for the time, I don't think that's really related to this issue.)

What would be the best way to get that base time value included in my SenML records?

The options I can think of:

  1. Customize SenML/CBOR decoding on my server so that it would allow tagged values as numbers for base time. I currently use SenMLCborUpokecenterEncoderDecoder.fromSenML method to get the values, and re-creating the logic would mean replacing several classes Leshan now nicely offers me, so it would definitely be some work.

  2. Customize SenML/CBOR decoding on my server so that it would use a Date for base time in a customized SenML record object. This would be even more work, as it would still require deserialization changes, plus custom SenMLPack and SenMLRecord classes.

  3. Contribute a change to SenMLCborPackSerDes for handling tagged base time values in Leshan. This would involve some new steps in the part of the code I linked above, but should be rather simple, and would cause less customizations on my end. However, if I'm the first user who has ever encountered this problem, it might not be feasible to change Leshan for everybody just because of this single case. Edit: This could actually be as small change as updating the linked code from bt.isNumber() to bt.Untag().isNumber() — but it should be checked it this caused any unwanted side effects.

  4. Contact the author of CBOR-Java (Peter O.) and try to make that library handle such values as numbers in the future. I feel this would be the optimal choice, and I'll most likely post a comment on the earlier issue about this in the repo in any case. Based on the original message there it just seems like this might be quite a big change in the library's logic. 🤷

Any thoughts on how to address this in my device server?

sbernard31 commented 4 months ago

Sorry for delay I was unavailable last few weeks. I will look at this now.

sbernard31 commented 4 months ago

After looking at it, my first bet is that your device doesn't follow the Sensor Measurement Lists (SenML) - RFC8428.

If you looked at RFC8428§4.3. SenML Labels You see that base time and time are encoded with Number.

And looking at RFC8428§6. CBOR Representation :

For JSON Numbers, the CBOR representation can use integers, floating-point numbers, or decimal fractions (CBOR Tag 4); however, a representation SHOULD be chosen such that when the CBOR value is converted to an IEEE double-precision, floating-point value, it has exactly the same value as the original JSON Number converted to that form. For the version number, only an unsigned integer is allowed.

Nothing about CBOR Tag 1.

Looking at example just above you will see that tag is not used for base time (-3) and time (6), which go in that way too.

Another point which go in that way, Tag is just about adding semantic, here this is not needed as we already have label which say we are talking about time. So using Tag 1 is just about making the payload 1 byte bigger for no benefits.

Let me know if it makes sense to you ?

sbernard31 commented 4 months ago

So my advises :

Why not supporting Tag 1 for date ? If this is really out of specification (which is what I thought for now) ==> https://github.com/eclipse-leshan/leshan/wiki/How-Leshan-should-behave-with-Non-Compliant-Implementations-%3F

sbernard31 commented 4 months ago

This also makes me think that just ignoring the base time is a not so good behavior for SenMLCborPackSerDes which should be more strict and rise exception instead of hiding the issue.

mjviljan commented 4 months ago

Sorry for delay I was unavailable last few weeks.

No worries, I saw you announcing that in another discussion.

I'm on vacation now myself, so I just briefly read through your replies. I'll go through them again with care once I get back to work (5th of August). Thank you for looking into this!

sbernard31 commented 4 months ago

Ok no problem, enjoy your vacation :wink:

mjviljan commented 3 months ago

[…] Nothing about CBOR Tag 1.

Looking at example just above you will see that tag is not used for base time (-3) and time (6), which go in that way too.

Another point which go in that way, Tag is just about adding semantic, here this is not needed as we already have label which say we are talking about time. So using Tag 1 is just about making the payload 1 byte bigger for no benefits.

Let me know if it makes sense to you ?

It does. While I'm not sure if using CBOR Tag 1 is against the specification per se, it indeed seems redundant as the field itself tells the data type already. And as you wrote, the CBOR diagnostic notation example in RFC 8428 does not have tags in the time fields.

So my advises :

  • At long term : report the issue to client, it should be fixed.
  • At mid term : add a SenMLCborPackSerDes parameter to SenMLCborUpokecenterEncoderDecoder constructor in Leshan, so you can provide a custom SenMLCborPackSerDes class which should limit the code change. (ideally for 2.0.0-M17 because I didn't plan to add new feature in 2.0.0-M16)
  • At short term : The only solution I see create a custom SenMLCborUpokecenterEncoderDecoder 😞

I think these make sense from Leshan's point of view.

I've notified the meter manufacturer about this but didn't report an actual issue yet. I'll see if I can discuss it better with them at some point. However, as they have a bunch of meters that most likely use this approach already, they're probably not very eager to change it... But it's always worth asking, of course.

If a new constructor (or constructor parameter) could be added to SenMLCborUpokecenterEncoderDecoder for using a custom SenMLCborPackSerDes in 2.0.0-M17, that would be great! If that isn't released before I'll be really needing this, I'll consider creating a custom SenMLCborUpokecenterEncoderDecoder myself. 👍 (Right now I'm still preparing the support for the said meter so it's not in actual use yet.)

sbernard31 commented 3 months ago

While I'm not sure if using CBOR Tag 1 is against the specification per se

For me chapter RFC8428§6. CBOR Representation is very clear

For JSON Numbers, the CBOR representation can use integers, floating-point numbers, or decimal fractions (CBOR Tag 4); however, a representation SHOULD be chosen such that when the CBOR value is converted to an IEEE double-precision, floating-point value, it has exactly the same value as the original JSON Number converted to that form. For the version number, only an unsigned integer is allowed.

Tag 1 is not mention at all. You can see that there is some specific line for version number (last sentence). If Tag 1 was allowed then it should be mentioned that Tag 1 must, should or could be used for Time. But nothing about that.

If a new constructor (or constructor parameter) could be added to SenMLCborUpokecenterEncoderDecoder for using a custom SenMLCborPackSerDes in 2.0.0-M17, that would be great!

We will try to make it happened :wink:

sbernard31 commented 3 months ago

At mid term : add a SenMLCborPackSerDes parameter to SenMLCborUpokecenterEncoderDecoder constructor in Leshan, so you can provide a custom SenMLCborPackSerDes class which should limit the code change.

I created a PR about that : https://github.com/eclipse-leshan/leshan/pull/1640

mjviljan commented 3 months ago

I checked the PR and tried using it quickly in my test case. Splitting the record deserialization into those small, overridable methods made it really quick and easy to override just the functionality I needed! All in all, it looks great and works perfectly in my case. 👍 Thank you! 🙂

sbernard31 commented 3 months ago

I integrated #1640 in master, it should be available in 2.0.0-M17, so I close this issue.

I've notified the meter manufacturer about this but didn't report an actual issue yet. I'll see if I can discuss it better with them at some point. However, as they have a bunch of meters that most likely use this approach already, they're probably not very eager to change it... But it's always worth asking, of course.

Please try to insist, because I seriously think this is not allowed in SenML and so they probably don't want to face interoperability issue with their device.