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

Fix some behavior about Timestamped Node in SenML and Old JSON content Format #1621

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

sbernard31
Copy link
Contributor

This mainly fixes issue listed at : #1612 (comment)

@sbernard31
Copy link
Contributor Author

@slaft let me know if that works for you. 🙂

@slaft
Copy link

slaft commented Jun 7, 2024

Thank you @sbernard31

So doing:

public class CustomLwM2mNodeSenMLDecoder extends LwM2mNodeSenMLDecoder {
    public CustomLwM2mNodeSenMLDecoder(SenMLDecoder decoder, boolean permissiveNumberConversion) {
        super(decoder, permissiveNumberConversion);
    }

    public CustomLwM2mNodeSenMLDecoder(SenMLDecoder decoder, LinkParser linkParser, boolean permissiveNumberConversion) {
        super(decoder, linkParser, permissiveNumberConversion);
    }

    @Override
    protected void validateNoTimestampedRecord(LwM2mResolvedSenMLRecord resolvedRecord) {
        // Do nothing
    }
}
leshanServerBuilder.setDecoder(buildPatchedDecoder());

private LwM2mDecoder buildPatchedDecoder() {
        Map<ContentFormat, NodeDecoder> nodeDecoders = DefaultLwM2mDecoder.getDefaultNodeDecoders(false);
        nodeDecoders.put(ContentFormat.SENML_JSON, new CustomLwM2mNodeSenMLDecoder(new SenMLJsonJacksonEncoderDecoder(), true));
        nodeDecoders.put(ContentFormat.SENML_CBOR, new CustomLwM2mNodeSenMLDecoder(new SenMLCborUpokecenterEncoderDecoder(), false));
        return new DefaultLwM2mDecoder(nodeDecoders, DefaultLwM2mDecoder.getDefaultPathDecoder());
    }

Allows me to accept a Read-Composite/Observe-Composite response with timestamps (which was accepted by default before).


Just to be sure, this does not provide a way to accept this payload right? (discussed here #1553 (comment))

Read/Observe on /3442/0:

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

@sbernard31
Copy link
Contributor Author

@slaft You're right.

If you want to support :

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

This should work :

public class CustomLwM2mNodeSenMLDecoder extends LwM2mNodeSenMLDecoder {
    public CustomLwM2mNodeSenMLDecoder(SenMLDecoder decoder, boolean permissiveNumberConversion) {
        super(decoder, permissiveNumberConversion);
    }

    public CustomLwM2mNodeSenMLDecoder(SenMLDecoder decoder, LinkParser linkParser,
            boolean permissiveNumberConversion) {
        super(decoder, linkParser, permissiveNumberConversion);
    }

    @Override
    protected void validateNoTimestampedRecord(LwM2mResolvedSenMLRecord resolvedRecord) {
        // Do nothing
    }

    @Override
    public List<TimestampedLwM2mNode> decodeTimestampedData(byte[] content, LwM2mPath path, LwM2mModel model,
            Class<? extends LwM2mNode> nodeClass) throws CodecException {
        LwM2mNode node = decode(content, path, model, nodeClass);
        return Arrays.asList(new TimestampedLwM2mNode(null, node));
    }
}

(but you will lost timestamp data)

@slaft
Copy link

slaft commented Jun 10, 2024

Thank you @sbernard31

If my understanding is correct, this will also impact historical notifications for single Observations.
(No impact on timestamped data on composite Observations or Send however.)

@sbernard31
Copy link
Contributor Author

sbernard31 commented Jun 10, 2024

If my understanding is correct, this will also impact historical notifications for single Observations.
(No impact on timestamped data on composite Observations or Send however.)

That's right.

It's still possible to write a more smarter version of :

@Override
public List<TimestampedLwM2mNode> decodeTimestampedData(byte[] content, LwM2mPath path, LwM2mModel model,
        Class<? extends LwM2mNode> nodeClass) throws CodecException {
    LwM2mNode node = decode(content, path, model, nodeClass);
    return Arrays.asList(new TimestampedLwM2mNode(null, node));
}

in your CustomLwM2mNodeSenMLDecoder.

But I can't help because I'm not sure what should mean :

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

And how we can express it with current LWM2M node model in Leshan.

The other solution is maybe to revert #1610.
I think we should seriously thing about that because if that was an error better to rollback now.
If we rollback that just doing :

public class CustomLwM2mNodeSenMLDecoder extends LwM2mNodeSenMLDecoder {
    public CustomLwM2mNodeSenMLDecoder(SenMLDecoder decoder, boolean permissiveNumberConversion) {
        super(decoder, permissiveNumberConversion);
    }

    public CustomLwM2mNodeSenMLDecoder(SenMLDecoder decoder, LinkParser linkParser, boolean permissiveNumberConversion) {
        super(decoder, linkParser, permissiveNumberConversion);
    }

    @Override
    protected void validateNoTimestampedRecord(LwM2mResolvedSenMLRecord resolvedRecord) {
        // Do nothing
    }
}

will ignore timestamp for composite/read/observe response and historical notification will be kept for notification.
(No impact on timestamped data on composite Observations or Send however.)

@mgdlkundera
Copy link
Contributor

mgdlkundera commented Jun 11, 2024

In our case best solution would be to take the latest timestamp value for object/object instance with many timpestamped values in a read and observe and also for a composite operations.

Ignoring timestamp is not a good aproach for us, so would it be possible to get only one latest timestamp value?
What do you think about this idea?

@sbernard31
Copy link
Contributor Author

Ignoring timestamp is not a good aproach for us, so would it be possible to get only one latest timestamp value?
What do you think about this idea?

I understand that maybe this solves your particular problem but I can not see how this could be a good API/behavior for Leshan.

I'm a bit frustrating because I feel (maybe I'm wrong, I hope I'm wrong) that you try to push your needs without taking into account what is good for Leshan project itself without trying to maintain or improve consistency in the project.

So if your want this behavior, I think you can implement it more or less easily in a CustomLwM2mNodeSenMLDecoder

I will probably take time again to summarize issues about timestamp even if I'm not sure I'm understandable... 😞

@sbernard31
Copy link
Contributor Author

sbernard31 commented Jun 13, 2024

I tried to clarify my thoughts here : #1623

@sbernard31
Copy link
Contributor Author

If you read ☝️, you will see that I think we take so much time to try to solve a model/client behavior issue.

FMPOV, the right way to express Sensor Measurement Timestamp would be to use/create a dedicated resource in your object model. I strongly encourage you to try to change (or ask to change) your client behavior.

Waiting and even if I would prefer to revert #1610, I can understand that you could need a temporary workaround solution.

So finally maybe current state of the code is a good compromise :

  • we try to keep more or less a kind of API/behavior consistency in Leshan.
  • it allow you to implement your own behavior without rewritten too much code.

So let me know if I should relaunch the 2.0.0-M15 process OR if you're still stuck about that.

-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 sbernard31 merged commit 517cab0 into master Jun 17, 2024
1 check passed
@sbernard31
Copy link
Contributor Author

I integrate this is master. So it will be tested by sandbox user.

@mgdlkundera
Copy link
Contributor

Lets release M15 current implementation, without rollback.

@sbernard31
Copy link
Contributor Author

@mgdlkundera thank you for letting me know that 🙏

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

Successfully merging this pull request may close these issues.

3 participants