net/gcoap: port to ztimer #17141
net/gcoap: port to ztimer #17141
Conversation
|
That's great, and could profit from the plans over on #16413 to avoid race conditions that are currently there somewhere. |
9b62ed0 to
17b8996
Compare
|
@Crysn: There is one USEC left for GCOAP_OBS_TICK can we avoid these? |
|
Yes, but not in this PR. Observation using time based observe values is not really practical, should be incrementing on its own scale on demand. Changing this is probably best done after this PR because a) it is then well motivated, and b) not creating cross-PR conflicts. |
| uint32_t now = ztimer_now(ZTIMER_USEC); | ||
| pdu->observe_value = (now >> GCOAP_OBS_TICK_EXPONENT) & 0xFFFFFF; |
There was a problem hiding this comment.
I'm wondering if we could get rid of the ZTIMER_USEC dependency here, if RTT is used and running at 32Khz, then the base timer tick would be 30us. This brings the need for this feature to be exposed again though... So I guess that would be a follow-up, but in that case ztimer_usec could be avoided altogether for gcoap..
There was a problem hiding this comment.
Definetly another pr (maybe we even do not need that see chrysn 2021-11-17)
also: be careful with increasing frequencys they may lead to needing extra module (ztimer_extended) additional wakeups (for that extended)
i am also not sure if we consider ztimers bases open for non ztimer use
Digest: lets get it to ztimer and later improve its ztimer use.
There was a problem hiding this comment.
Definetly another pr (maybe we even do not need that see chrysn 2021-11-17)
I think you forgot the link here :)
There was a problem hiding this comment.
#17141 (comment) in answer to #17141 (comment)
|
@fjmolinas @chrysn: Does one of you want to do the review? |
| uint32_t start = xtimer_now_usec(); | ||
| res = xtimer_msg_receive_timeout(&msg, timeout); | ||
| uint32_t start = ztimer_now(ZTIMER_MSEC); | ||
| res = ztimer_msg_receive_timeout(ZTIMER_MSEC, &msg, timeout); |
There was a problem hiding this comment.
The units where changed here, are you sure msec are enough?
There was a problem hiding this comment.
in all cases where i swap ed timeout of unamend unit (which would be usec in all cases) or usec i checked the typical default values if they mad to msec and only swaped if they did.
There is one exception GCOAP_OBS_TICK_EXPONENT that can not map to integer msec -> it is still usec
There was a problem hiding this comment.
I also changed all default timeout to (_MSEC) if applicable (see gcoap.h) and ether devided them by 1000 or changed their time factor. (most of these time outs are multiple seconds and msec is used to have enough resolution
In cases this will also save us of the inclusion of ztimer_sec, which would lead to ztimer extended and that may use extra power.
| uint32_t now = ztimer_now(ZTIMER_USEC); | ||
| pdu->observe_value = (now >> GCOAP_OBS_TICK_EXPONENT) & 0xFFFFFF; |
There was a problem hiding this comment.
Definetly another pr (maybe we even do not need that see chrysn 2021-11-17)
I think you forgot the link here :)
| event_callback_init(&_dtls_session_free_up_tmout_cb, | ||
| _dtls_free_up_session, NULL); | ||
| event_timeout_init(&_dtls_session_free_up_tmout, &_queue, | ||
| event_timeout_ztimer_init(&_dtls_session_free_up_tmout, ZTIMER_MSEC,&_queue, |
There was a problem hiding this comment.
| event_timeout_ztimer_init(&_dtls_session_free_up_tmout, ZTIMER_MSEC,&_queue, | |
| event_timeout_ztimer_init(&_dtls_session_free_up_tmout, ZTIMER_MSEC, &_queue, |
|
@fjmolinas: i added the new |
I can't reproduce, maybe rebase this branch? |
|
did you test with or without the last commit (which "fixes"/works around the issue)? to reproduce merge or rebase PR with master, without the last commit "test/unittest: no auto ztimer_periph_rtt" it seem like there is a problem in the conflict resolution |
Ahhh no I did not, but that is expected, the filtering is not done against conflicting modules, you must explicitly disable it. Filtering against it is another thing. Explicitly disabling it avoids dealing with the question of which one we should prefer. Could be improved upon later... |
|
ahh ok i thought there is a automatic conflict management in the module system |
Nope we didn't add that , we just added the warning. There is conflict management for FEATURES, modules happen with |
|
I just ran the gcoap example (native with gnrc-tap) after all these additional commits ✔️ -> ready for review term 2: term 3: |
75566a2 to
d7d1f7a
Compare
| @@ -26,6 +26,7 @@ | |||
| #include "net/cord/common.h" | |||
| #include "net/sock/util.h" | |||
| #include "net/ipv6/addr.h" | |||
| #include "xtimer.h" | |||
There was a problem hiding this comment.
Why add xtimer here if the PR is about removing xtimer?
There was a problem hiding this comment.
this example is using xtimer but didn't include it (was used via gcoap.h)
There was a problem hiding this comment.
Ok, won't block it in any case, I would have just have gone the other way around
There was a problem hiding this comment.
Ok, won't block it in any case, I would have just have gone the other way around
It would be awesome if you provided some test output when tests are ran, its good to have some history on the PR |
Timeouts and DTLS would need testing I guess |
|
i updated that comment does that help? I never used DTLS seems my libcoap doesn't support it either |
|
Maybe @leandrolanzieri could test this one? |
There was a problem hiding this comment.
Testing timeout is still 5s.
2021-12-08 20:24:51,172 # coap post fe80::ac8d:fee1:e02c:e32f:5 5683 /tmp "hello"
2021-12-08 20:24:51,174 # gcoap_cli: sending msg ID 43790, 17 bytes
> 2021-12-08 20:24:56,180 # gcoap: timeout for msg ID 43790
I tried testing dtls but never got a response or a timeout, @leandrolanzieri should this work out of the box when communicating between two nodes?
Yes, did you use port 5684? I was not particularly involved during the integration of DTLS to gcoap but it seems that indeed when the handshake fails there are no retries nor message is shown. We should look into that, but it is unrelated to the PR as it occurs on master as well. I tested with this PR and the example still works as expected, but I can't say anything about timeouts: |
Yes it seems it does not work for me with real hardware, but it does with native..
No message seems to be a matter of enabling debug messages though..., after enabling I verified the handshake timeout is OK: |
There was a problem hiding this comment.
Observing still works:
→ aiocoap-client -m GET "coap://[2001:db8::2]/cli/stats" --observe
0
---
1
---
2
main(): This is RIOT! (Version: 2022.01-devel-915-gb200f-pr-17141)
gcoap example app
All up, running the shell now
> ifconfig 6 add 2001:db8::2/64
ifconfig 6 add 2001:db8::2/64
success: added 2001:db8::2/64 to interface 6
> gcoap: Registered observer for: /cli/stats
coap get fe80::4c4c:91ff:fe2e:14da 5684 /cli/stats
coap get fe80::4c4c:91ff:fe2e:14da 5684 /cli/stats
gcoap_cli: sending msg ID 539, 16 bytes
gcoap_cli: creating /cli/stats notification
> coap: received timeout message
gcoap: timeout for msg ID 539
coap get fe80::4c4c:91ff:fe2e:14da 5684 /cli/stats
coap get fe80::4c4c:91ff:fe2e:14da 5684 /cli/stats
gcoap_cli: sending msg ID 541, 16 bytes
gcoap_cli: creating /cli/stats notification
> coap: received timeout message
gcoap: timeout for msg ID 541
There was a problem hiding this comment.
ACK!, I've tested what seemed testable. There is maybe an issue with dtls on real hardware but this is also in master... I'll open an issue once I get more info from the initial PR contributor.
Otherwise this makes me think that it's a shame https://github.com/kb2ma/riot-coap-pytest where never integrated in tree or in the release, maybe @miri64 might be interested in those since she has been working witj gcoap a bunch lately.
|
thank you for reviewing |
Contribution description
switches gcoap to use ztimer
Testing procedure
run and manualy test with examples/gcoap
Issues/PRs references
waiting for #16958 which would enable ztimer_msec use