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

[petition] return ID of existing active commissioner if one exists #36

Merged
merged 4 commits into from
Mar 18, 2020

Conversation

wgtdkp
Copy link
Member

@wgtdkp wgtdkp commented Mar 14, 2020

This PR returns the commissioner ID when petition failed due to an already-active commissioner in the Thread Network.

@wgtdkp wgtdkp requested review from bukepo and librasungirl March 14, 2020 08:56
@codecov-io
Copy link

codecov-io commented Mar 16, 2020

Codecov Report

Merging #36 into master will decrease coverage by 0.24%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   59.92%   59.67%   -0.25%     
==========================================
  Files          54       53       -1     
  Lines        5397     5379      -18     
==========================================
- Hits         3234     3210      -24     
- Misses       2163     2169       +6
Impacted Files Coverage Δ
src/app/commissioner_app.hpp 75% <ø> (ø) ⬆️
include/commissioner/commissioner.hpp 100% <ø> (ø) ⬆️
src/library/commissioner_impl.hpp 100% <100%> (ø) ⬆️
src/app/commissioner_app.cpp 29.76% <100%> (ø) ⬆️
src/library/commissioner_impl.cpp 61.9% <57.14%> (-0.16%) ⬇️
src/app/cli/interpreter.cpp 33.96% <75%> (+0.17%) ⬆️
src/library/commissioner_safe.cpp 63.77% <85.71%> (+0.17%) ⬆️
src/library/uri.hpp

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c6d936...3419572. Read the comment docs.

Copy link
Member

@bukepo bukepo left a comment

Choose a reason for hiding this comment

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

LGTM:+1:

@librasungirl librasungirl linked an issue Mar 17, 2020 that may be closed by this pull request
@wgtdkp wgtdkp requested a review from jwhui March 17, 2020 08:23
@jwhui jwhui changed the title [petition] returns its ID if there is already an active commissioner [petition] return ID of already active commissioner if one exists Mar 17, 2020
@jwhui jwhui added the enhancement New feature or request label Mar 17, 2020
src/app/cli/interpreter.cpp Outdated Show resolved Hide resolved
src/app/commissioner_app.cpp Outdated Show resolved Hide resolved
@wgtdkp wgtdkp requested a review from jwhui March 18, 2020 05:20
@jwhui
Copy link
Member

jwhui commented Mar 18, 2020

@wgtdkp, thanks for the updates!

Would it be possible to implement a functional test on Travis for this feature?

@wgtdkp
Copy link
Member Author

wgtdkp commented Mar 18, 2020

@wgtdkp, thanks for the updates!

Would it be possible to implement a functional test on Travis for this feature?

I has been thinking about this but find it to be not possible for now. We currently doing functional tests with OTBR and opethread simulation. But border agent on device side has only one DTLS server and a new connection will be rejected if the DTLS session is active. In this case, the connection between a new commissioner and border agent cannot be established and we got no chance to get the existing commissioner ID. So, with current openthread border agent implementation, the user actually can never see existing commissioner ID.

We can add one more DTLS session to border agent to allow a second commissioner to get connected to border agent but later rejected because of existing active commissioner. How about I create an issue for this and add the functional tests back after making the change to border agent?

@jwhui jwhui changed the title [petition] return ID of already active commissioner if one exists [petition] return ID of existing active commissioner if one exists Mar 18, 2020
@jwhui
Copy link
Member

jwhui commented Mar 18, 2020

How about I create an issue for this and add the functional tests back after making the change to border agent?

I am fine to defer this as long as we track it. I see you already submitted #41.

@jwhui jwhui merged commit 3b0cd47 into openthread:master Mar 18, 2020
@wgtdkp wgtdkp deleted the fix-petition branch March 20, 2020 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handler for indicating presence of another active commissioner
5 participants