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 #1610

Conversation

mgdlkundera
Copy link
Contributor

@mgdlkundera mgdlkundera commented Apr 19, 2024

Implementation of read response with timestamped SenML

This aims to implement : #1553

@mgdlkundera mgdlkundera changed the title Read Response Read Response with timestamped SenML Apr 19, 2024
@sbernard31
Copy link
Contributor

I will try to look at this tomorrow.

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.

My first quick review.

I didn't checkout / test the code yet.

Do you want to handle timestamped value for ObserveResponse too ?
(I mean for observe response, not notification (notification as already timestamp value)

@sbernard31
Copy link
Contributor

I rebase the code on master and push some fixes.

You could review it if wanted. (I advice to review commit by commit)

We still need to decide :

Do you want to handle timestamped value for ObserveResponse too ?
(I mean for observe response, not notification (notification as already timestamp value)

I didn't have time to test about reducing timeout value but I pretty sure that 3 seconds is not needed

@mgdlkundera
Copy link
Contributor Author

I reviewed the code and everything looks fine. I also pushed commit with timestamped for Observe Response.

@sbernard31
Copy link
Contributor

I reviewed the code and everything looks fine. I also pushed commit with timestamped for Observe Response.

🙏

I started to work on it again. I should be able to push some code tomorrow.

@sbernard31
Copy link
Contributor

@mgdlkundera, I added some more fix/features.

You could review/test it if wanted. (I advice to review commit by commit)

@mjviljan is possible to test it ? (let me know if this is a problem for you to test a PR)

@mjviljan
Copy link

@mjviljan is possible to test it ? (let me know if this is a problem for you to test a PR)

Yes, I can test this also. Unfortunately only some time next week, so if you want to merge this before that, don't wait for me.

But otherwise I'll come back to this once I've tried it out.

@sbernard31
Copy link
Contributor

@mjviljan thx for letting me know that. We can wait for next week.

@mjviljan
Copy link

Hi, sorry for the delay. I tested this now for my case. Unfortunately this doesn't fix that — but I guess that's just (more) proof that the data I receive from the device may not be really LwM2M standard-compliant.

Anyway, thanks for suggesting me to test this! At least I now have some new knowledge to continue with.

@mgdlkundera
Copy link
Contributor Author

I tested it and I also look through the code and it is ok.

@sbernard31
Copy link
Contributor

I tested it and I also look through the code and it is ok.

Thx 🙏

I rebased it on master, then squashed all commit in one (dceda02), then integrated in master.
It will be available in 2.0.0-M15.

@sbernard31 sbernard31 closed this May 23, 2024
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.

None yet

3 participants