Fix cordep eventcbs #12996
Fix cordep eventcbs #12996
Conversation
This changes allows to call any cord_ep API function from with the event callbacks provided by the standalone module.
haukepetersen
added
Type: bug
|
Sure I can verify that. I will do that at the beginning of January! |
There was a problem hiding this comment.
One other thought I have is, that the cord_ep_standalone_signal() function can only be called with a bool. Therefore you do not know which command actually led to the current situation (registered, deregistered). This circumstance makes it hard in the callback to know if the deregistered state is the wanted state (after a remove from the resource directory), or if the actual command failed (was trying to update).
I do not know what the wanted behavior would be, but I think there should be some clarification/documentation what is supposed to be triggered when and with what information (only in code, also after executing a shell command, ...). When these questions are clarified, it can be changed to a consistent and documented version.
The current version seems to work, but contains some inconsistencies and depending on how it is used, does not give you all the information you might need and could receive.
| _rd_loc[0] = '\0'; | ||
| } | ||
| mutex_unlock(&_mutex); | ||
|
|
||
| #ifdef MODULE_CORD_EP_STANDALONE | ||
| if (res != CORD_EP_OK) { |
There was a problem hiding this comment.
To only signal the standalone module when updating has been failed seems rather unwanted behavior, since you would have to handle the success case yourself. Furthermore triggering an update from the shell does not trigger the callback, which seems inconsistent.
Maybe you could signal the success case too in here and remove triggering the callback on success in the standalone module (see https://github.com/RIOT-OS/RIOT/pull/12996/files#diff-aa4a4d29f6ba7d1ecd4374a7eb15d331R77).
| @@ -76,9 +76,6 @@ static void *_reg_runner(void *arg) | |||
| _set_timer(); | |||
| _notify(CORD_EP_UPDATED); | |||
There was a problem hiding this comment.
Remove _notify, since it should be triggered in ep_cord.c
|
Yepp, you have a point! I don't know why I haven't seen this earlier, but right now a proper solution seems obvious to me: we simply add a proper event callback to the base Will look into this as soon as I find a minute or two... |
haukepetersen
removed
the
CI: ready for build
|
Done: I decided to open a new PR for the proposed changes: #13128 |
So can this one be closed? |
|
I would like to get some feedback on #13128 first. In case that new PR is bullshit, we still have this one :-) |
|
What's the status here and with #13128? Given the new approach by @bergzand in #20040 I would opt to close both PRs as @haukepetersen does not seem to be active anymore? |
|
Miri and koen agreed to close those. |
Contribution description
fixes #12884
Testing procedure
a) connect to a RD, watch for the
"RD endpoint event: successfully updated client registration"messages, kill the RD, wait for the"RD endpoint event: dropped client registration"message -> it should only appear onceb) @MatthiasBraeuer could you verify that reconnecting from the event callback does not lead to deadlocks anymore? Thanks
Issues/PRs references
fixes #12884