8000 How to address a tagged CBOR number value not being deserialized as base time in SenMLRecord? · Issue #1631 · eclipse-leshan/leshan · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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

Closed
mjviljan opened this issue Jul 11, 2024 · 11 comments
Labels
question Any question about leshan

Comments

@mjviljan
Copy link
mjviljan commented Jul 11, 2024

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:

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

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

@sbernard31
Copy link
Contributor

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

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 😞

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

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

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

Ok no problem, enjoy your vacation 😉

@mjviljan
Copy link
Author
mjviljan commented Aug 5, 2024

[…]
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
Copy link
Contributor

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 😉

@sbernard31
Copy link
Contributor

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 8000 PR about that : #1640

@mjviljan
Copy link
Author

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

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.

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

2 participants
0