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

Enable clients requesting server time in CoAP responses #3503

Merged
merged 21 commits into from
Jun 29, 2023
Merged

Enable clients requesting server time in CoAP responses #3503

merged 21 commits into from
Jun 29, 2023

Conversation

StFS
Copy link
Contributor

@StFS StFS commented Jun 19, 2023

Creating this PR to get feedback on whether I'm on the right track.

@StFS StFS requested a review from sophokles73 as a code owner June 19, 2023 10:40
@sophokles73 sophokles73 added CoAP Adapter Feature Request A request for adding new functionality to Hono labels Jun 20, 2023
@sophokles73
Copy link
Contributor

Thanks for your interest in Hono and for creating this PR 👍

I have been reading through your question raised with Californium and I have to admit that I would actually also prefer to use a client provided CoAP Option or a request parameter in order to trigger the return of the system time. Turning this on for all requests at the adapter level feels a little too coarse grained to me. If there is no way the client could include such an Option/req param, then I could also live with defining the property at the tenant level (instead of the adapter level).

The system time returned by the adapter should in any case be included in a CoAP option.

@sophokles73
Copy link
Contributor

Oh, and BTW, you will need to sign an Eclipse ECA as described in the contribution guide and make sure to a Signed-off-by footer to your commit messages.

@StFS
Copy link
Contributor Author

StFS commented Jun 20, 2023

Thanks @sophokles73 I'll dig into this a bit deeper then (as well as the ECA stuff).

@StFS StFS marked this pull request as draft June 20, 2023 13:37
@StFS
Copy link
Contributor Author

StFS commented Jun 20, 2023

Addresses [#3502]

StFS added 2 commits June 20, 2023 18:01
@StFS
Second attempt at including a timestamp in the CoAP responses. This t…
c40c266 
…ime the devices request the option be included by either providing the time option in their CoAP request or by providing the 'hono-response-timestamp' request parameter.
@StFS
Clean up config from previous implementation.
7c1731c
@StFS
Copy link
Contributor Author

StFS commented Jun 20, 2023 •
edited
Loading

@sophokles73 : Take 2.

Now the device initiates the timestamp response either by sending its own time option in the request or by specifying the hono-response-timestamp request parameter.

Again, everything is very crude and will be cleaned up... I just want to bounce this around to see if this is something that would possibly be included eventually ;-)

I'm still just copying the option number from the Californium demo server implementation by @boaks so we'll need to come up with a new option number (see his suggestion for segmentation). Also, I'm open for suggestions for the name of the request parameter. It's a bit too verbose IMO.

@boaks
Copy link
Contributor

boaks commented Jun 20, 2023

Just to mention:
If it's mainly the same semantic, you may just use the number of Californium.
I will also try to contact the IANA "officer", maybe using some numbers in the none experimental range isn't that complicated ;-).

@sophokles73
Copy link
Contributor

Also, I'm open for suggestions for the name of the request parameter. It's a bit too verbose IMO.

I agree, let's try to cut this down. What about just hono-time?

@sophokles73 sophokles73 added this to the 2.4.0 milestone Jun 26, 2023
@sophokles73
Copy link
Contributor

@StFS an answer to a previous question:

Would it be okay to upgrade the Californium library to 3.8 as part of this PR?

No, not as part of this PR but I was planning to do the update anyway, so I will do this independently ...

@StFS
Copy link
Contributor Author

StFS commented Jun 26, 2023

@StFS an answer to a previous question:

Would it be okay to upgrade the Californium library to 3.8 as part of this PR?

No, not as part of this PR but I was planning to do the update anyway, so I will do this independently ...

Alright @sophokles73, I'll try to remember to do a separate PR for using the option registry once that update has been made. But for now I'll do a bit more cleanup and try to wrap this one up.

@StFS
Copy link
Contributor Author

StFS commented Jun 26, 2023

Closes #3502

@sophokles73
Copy link
Contributor

I'll try to remember to do a separate PR for using the option registry once that update has been made.

I have created #3506

StFS and others added 3 commits June 28, 2023 14:28
@StFS
Move the time option definitions into a dedicated class. Add JavaDoc.…
38534db 
… Clean up System.out.println. Add some negative tests for time option. Minor refactoring to tests. Add integration tests for the CoAP time option feature.
@StFS
Merge remote-tracking branch 'origin/add_coap_config_for_server_times…
e32348c 
…tamp_in_responses' into add_coap_config_for_server_timestamp_in_responses
@StFS
Merge branch 'eclipse-hono:master' into add_coap_config_for_server_ti…
44b1bbc 
…mestamp_in_responses
@StFS StFS changed the title WIP: first stab at an implementation for sending server time as an option in CoAP responses. Enable clients requesting server time in CoAP responses Jun 28, 2023
@StFS StFS marked this pull request as ready for review June 28, 2023 15:04
@StFS StFS requested a review from kaniyan as a code owner June 28, 2023 15:49
sophokles73
sophokles73 reviewed Jun 29, 2023
View reviewed changes
adapters/coap/src/main/java/org/eclipse/hono/adapter/coap/option/TimeOption.java Outdated Show resolved Hide resolved
adapters/coap/src/main/java/org/eclipse/hono/adapter/coap/option/TimeOption.java Outdated Show resolved Hide resolved
adapters/coap/src/main/java/org/eclipse/hono/adapter/coap/option/TimeOption.java Show resolved Hide resolved
adapters/coap/src/test/java/org/eclipse/hono/adapter/coap/CoapContextTest.java Show resolved Hide resolved
adapters/coap/src/main/java/org/eclipse/hono/adapter/coap/option/TimeOption.java Outdated Show resolved Hide resolved
tests/src/test/java/org/eclipse/hono/tests/coap/CoapTestBase.java Outdated Show resolved Hide resolved
tests/src/test/java/org/eclipse/hono/tests/coap/CoapTestBase.java Outdated Show resolved Hide resolved
tests/src/test/java/org/eclipse/hono/tests/coap/CoapTestBase.java Outdated Show resolved Hide resolved
adapters/coap/src/main/java/org/eclipse/hono/adapter/coap/CoapContext.java Outdated Show resolved Hide resolved
@sophokles73
Copy link
Contributor

Can you fix the Checkstyle violation that makes the CI build fail?
Also, can you please add your self/company to the list of copyright holders in legal/src/main/resources/legal/NOTICE.md?

@StFS
Copy link
Contributor Author

StFS commented Jun 29, 2023

Can you fix the Checkstyle violation that makes the CI build fail?

Doing that as we type. Going insane trying to figure out the correct import order ;-) But I figured it out eventually.

Also, can you please add your self/company to the list of copyright holders in legal/src/main/resources/legal/NOTICE.md?

Will do.

@sophokles73
Copy link
Contributor

you can use the eclipse/hono.importorder file to set up your Eclipse IDE so that it automatically fixes the import order when saving a file.

@StFS
Copy link
Contributor Author

StFS commented Jun 29, 2023

you can use the eclipse/hono.importorder file to set up your Eclipse IDE so that it automatically fixes the import order when saving a file.

Umm... yeah... well... I'm kinda using the "other" IDE ;)

sophokles73
sophokles73 approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

Great work :-) Thanks for contributing 👍

@sophokles73 sophokles73 merged commit b5c80fe into eclipse-hono:master Jun 29, 2023
@sophokles73 sophokles73 linked an issue Jun 29, 2023 that may be closed by this pull request
Provide ability to get server timestamp in CoAP responses #3502
Closed
@StFS StFS deleted the add_coap_config_for_server_timestamp_in_responses branch June 29, 2023 16:24
@StFS StFS mentioned this pull request Jun 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boaks boaks boaks left review comments

@sophokles73 sophokles73 sophokles73 approved these changes

@kaniyan kaniyan Awaiting requested review from kaniyan kaniyan is a code owner

Assignees
No one assigned
Labels
CoAP Adapter Feature Request A request for adding new functionality to Hono
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

Provide ability to get server timestamp in CoAP responses
3 participants
@StFS @sophokles73 @boaks