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.

Sign up for GitHub

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

Jump to bottom

Read Composite support for timestamped data. #1639

Closed
wants to merge 7 commits into from
Closed

Read Composite support for timestamped data. #1639

wants to merge 7 commits into from

Conversation

JaroslawLegierski
Copy link
Contributor

The main aim of this PR is to support Read Composite using timestamped data described in #1637

@JaroslawLegierski JaroslawLegierski mentioned this pull request Aug 9, 2024
Closed
@JaroslawLegierski JaroslawLegierski force-pushed the opl/read_response_with_timestamped_SenML branch from 020fb17 to 5edaaa4 Compare August 9, 2024 14:30
sbernard31
sbernard31 reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@sbernard31 sbernard31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading that I think maybe we need to clarify which behavior we want for different case.
I guess this should be more or less consistent with ReadResponse and ObserveResponse ?

...egration-tests/src/test/java/org/eclipse/leshan/integration/tests/lockstep/LockStepTest.java Outdated Show resolved Hide resolved
...egration-tests/src/test/java/org/eclipse/leshan/integration/tests/lockstep/LockStepTest.java Outdated Show resolved Hide resolved
...egration-tests/src/test/java/org/eclipse/leshan/integration/tests/lockstep/LockStepTest.java Outdated Show resolved Hide resolved
leshan-lwm2m-core/src/main/java/org/eclipse/leshan/core/response/ReadCompositeResponse.java Outdated Show resolved Hide resolved
leshan-lwm2m-core/src/main/java/org/eclipse/leshan/core/response/ObserveCompositeResponse.java Outdated Show resolved Hide resolved
leshan-lwm2m-core/src/main/java/org/eclipse/leshan/core/response/ReadCompositeResponse.java Show resolved Hide resolved
leshan-lwm2m-core/src/main/java/org/eclipse/leshan/core/response/ReadCompositeResponse.java Outdated Show resolved Hide resolved
leshan-lwm2m-core/src/main/java/org/eclipse/leshan/core/response/ReadCompositeResponse.java Outdated Show resolved Hide resolved
leshan-lwm2m-core/src/main/java/org/eclipse/leshan/core/response/ReadCompositeResponse.java Outdated Show resolved Hide resolved
@JaroslawLegierski JaroslawLegierski force-pushed the opl/read_response_with_timestamped_SenML branch from 5edaaa4 to cc9e4bc Compare August 23, 2024 17:51
@JaroslawLegierski
Copy link
Contributor Author

I added some code fixes in dedicated commit

@sbernard31
Copy link
Contributor

@JaroslawLegierski I add some modification, I let you review it.

@sbernard31 sbernard31 force-pushed the opl/read_response_with_timestamped_SenML branch from 913e27e to 05d6d10 Compare September 3, 2024 14:25
@JaroslawLegierski
Copy link
Contributor Author

I see that the content in ObserveCompositeResponse has been divided into:

  • content – typical content (without a timestamp)
  • timestampedContent – data with timestamps
  • historicalValues – only for notifications

I tested this new feature using a Leshan demo client I created some time ago, which has very limited timestamp support, and the results are OK (If there is one timestamp in the content, the data is decoded, if there are more, we get an exception).

@sbernard31
Copy link
Contributor

sbernard31 commented Sep 6, 2024 •
edited
Loading

@JaroslawLegierski Let me know if you think this can be integrated in master

(Maybe 🤷, you should check if the code is customizable enough, so you can have desired behavior which seems different than default one : #1621 (comment))

@sbernard31
Copy link
Contributor

@JaroslawLegierski, we have several opened PR and I would like to start to integrate some of them in master.

Let me know if I should wait for this one OR if I should integrate it ? 🙏

@JaroslawLegierski
Copy link
Contributor Author

@JaroslawLegierski, we have several opened PR and I would like to start to integrate some of them in master.

Let me know if I should wait for this one OR if I should integrate it ? 🙏

I am still waiting for feedback from my colleagues from FR. Can we please wait a few more days?

@sbernard31
Copy link
Contributor

Yep no problem. I just wanted to know if I should wait 🙂

@JaroslawLegierski
Copy link
Contributor Author

Yep no problem. I just wanted to know if I should wait 🙂

I think we can merge this PR with the master branch.

@sbernard31
Copy link
Contributor

This is now integrated in master (commit 02b7538) and so should be available in next release 2.0.0-M17.

Thx @JaroslawLegierski 😉 !

@sbernard31 sbernard31 closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbernard31 sbernard31 sbernard31 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JaroslawLegierski @sbernard31