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

net: openthread: add pskd join loop #25589

Closed
wants to merge 1 commit into from
Closed

net: openthread: add pskd join loop #25589

wants to merge 1 commit into from

Conversation

Philipp-Wohlgenannt-Tridonic-com
Copy link

restart joiner if in autostart mode and join hasn't succeded yet or if joiner is back to idle

Signed-off-by: Philipp Wohlgenannt philipp.wohlgenannt@tridonic.com

@Philipp-Wohlgenannt-Tridonic-com Philipp-Wohlgenannt-Tridonic-com changed the title add pskd join loop net: openthread: add pskd join loop May 25, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented May 25, 2020 •
edited

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@Philipp-Wohlgenannt-Tridonic-com Philipp-Wohlgenannt-Tridonic-com force-pushed the feature-pskd-joinloop branch 2 times, most recently from f399e2c to e9be59e Compare May 25, 2020 15:24
jukkar
jukkar previously requested changes May 25, 2020
View reviewed changes
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
rlubos
rlubos requested changes May 26, 2020
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Some high-level comments for now.

subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
@Philipp-Wohlgenannt-Tridonic-com Philipp-Wohlgenannt-Tridonic-com force-pushed the feature-pskd-joinloop branch 2 times, most recently from fc08d40 to 37c0c92 Compare May 27, 2020 08:40
@Philipp-Wohlgenannt-Tridonic-com
Copy link
Author

@rlubos :
We made a new implementation which is considering all comments from above. I accidentally pushed a branch without commits to Philipp-Wohlgenannt-Tridonic-com:feature-pskd-joinloop and that closed the pull request. Sorry about that... Should I open a new pull request or can you reopen this?

In Philipp-Wohlgenannt-Tridonic-com:feature-pskd-joinloop there is the new commit (bc0add9130598d94b708e929db1ae1e0b23abd47) with the desired changes.

@rlubos rlubos reopened this Jun 5, 2020
@zephyrbot zephyrbot added the area: API Changes to public APIs label Jun 5, 2020
rlubos
rlubos reviewed Jun 5, 2020
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Thanks for the rework, the retransmit logic looks much better now. There are still some issues left though.

subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c

static void ot_joiner_restart_timer(void *dummy)
{
k_work_submit(&joiner_restart);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could simply use k_delayed_work_submit instead of timer + work combo.

Copy link
Author

Choose a reason for hiding this comment

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

We have tried that but that did not work with our zephyr devices.

subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
subsys/net/l2/openthread/openthread.c Outdated Show resolved Hide resolved
@Philipp-Wohlgenannt-Tridonic-com Philipp-Wohlgenannt-Tridonic-com force-pushed the feature-pskd-joinloop branch 2 times, most recently from 47c3603 to 318342a Compare June 5, 2020 15:24
rlubos
rlubos reviewed Jun 8, 2020
View reviewed changes
subsys/net/l2/openthread/openthread.c Outdated
/* for rebooting after a period if join not successful */
#include <power/reboot.h>
/* need to immediately print to stdout if rebooting */
#include <stdio.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

#include <stdio.h> is no longer needed, should be removed. While removing, please move #inlclude <power/reboot.h> to the top, together with other includes.

subsys/net/l2/openthread/openthread.c Outdated
if (CONFIG_OPENTHREAD_JOINER_REBOOT != 0 &&
join_attempts >= CONFIG_OPENTHREAD_JOINER_REBOOT) {
log_panic();
LOG_WRN("Joining failed %d times ... "
Copy link
Contributor

Choose a reason for hiding this comment

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

Please stick to networking logging macros for consistency, i. e. NET_WARN.

subsys/net/l2/openthread/openthread.c Outdated
/* need to immediately print to stdout if rebooting */
#include <stdio.h>

static char *joiner_pskd = OT_JOINER_PSKD;
Copy link
Contributor

Choose a reason for hiding this comment

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

static const char *

rlubos
rlubos reviewed Jun 8, 2020
View reviewed changes
subsys/net/l2/openthread/openthread.c Outdated
void ot_joiner_start_handler(otError error, void *context)
{
struct openthread_context *ot_context = context;

static u32_t restart_delay;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, looking at PR #25679 which got just merged,it seems we should use uint32_t now.

@Philipp-Wohlgenannt-Tridonic-com
Copy link
Author

We have reverted it to to timer implementation because the previous versions with k_work_submit() or k_delayed_work_submit() caused crashes on our local zephyr devices.

@pasi-ojala-tridonic
Copy link

We have reverted it to to timer implementation because the previous versions with k_work_submit() or k_delayed_work_submit() caused crashes on our local zephyr devices.

Just as a clarifying comment: it was the use k_delayed_work item that was causing us (so far non-understood) crashes in certain use cases of the joiner, so we went back to using the timer and k_work, which survives the same case fine.

rlubos
rlubos approved these changes Jun 16, 2020
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Just please address the Shippable failure (currently this code does not build due to timer handler function signature mismatch).

@Philipp-Wohlgenannt-Tridonic-com
net: openthread: add pskd join loop
0304b2d 
restart joiner if:
-in autostart mode and join hasn't succeeded yet
-joiner is back to idle

Signed-off-by: Philipp Wohlgenannt <philipp.wohlgenannt@tridonic.com>
@jukkar jukkar dismissed their stale review June 26, 2020 10:54

Removing my -1 in order not to block this during my vacation.

@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 30, 2020
@zephyrbot zephyrbot removed the has-conflicts Issue/PR has conflicts with another issue/PR label Jul 7, 2020
@github-actions
Copy link

github-actions bot commented Sep 6, 2020

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 6, 2020
@github-actions github-actions bot closed this Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukkar jukkar jukkar left review comments

@rlubos rlubos rlubos approved these changes

@pfalcon pfalcon Awaiting requested review from pfalcon

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

Assignees
No one assigned
Labels
area: API Changes to public APIs area: Networking Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Philipp-Wohlgenannt-Tridonic-com @zephyrbot @pasi-ojala-tridonic @jukkar @rlubos