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

[srpl] integrate SRPL #1743

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[srpl] integrate SRPL #1743

wants to merge 1 commit into from

Conversation

superwhd
Copy link
Contributor

@superwhd superwhd commented Feb 6, 2023

This PR implements the platform code for supporting SRPL. It depends on the openthread PR openthread/openthread#8403.

This PR also contains the commit of #1742. Please review the most recent commit only.

Future work:

  • Currently the DSO transport is based on DNS-over-TCP. We need to support DNS-over-TLS later.
  • We may improve the way of handling multiple addresses of a single peer.
  • We need to let DNS-SD API handle the case when mDNS publisher is not ready.

Test:

  • I implemented a basic docker based BR test case for verifying the SRPL functionality. I'll send another PR to add the test case in openthread repo.
  • I set up 2 Raspberry Pi based OTBRs and manually verified some basic scenarios.

@superwhd superwhd force-pushed the srpl-github-pr branch 3 times, most recently from 953800a to 5eccff2 Compare February 8, 2023 04:15
@superwhd superwhd marked this pull request as ready for review February 8, 2023 05:54
abtink
abtink reviewed Feb 8, 2023
View reviewed changes
Copy link
Member

@abtink abtink left a comment

Choose a reason for hiding this comment

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

Thanks @superwhd. Overall looks great.
Some questions/suggestions below

src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Show resolved Hide resolved
@superwhd superwhd force-pushed the srpl-github-pr branch 2 times, most recently from f286297 to 853238d Compare February 10, 2023 08:15
superwhd
superwhd commented Feb 10, 2023
View reviewed changes
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Show resolved Hide resolved
src/sdp_proxy/advertising_proxy.cpp Outdated Show resolved Hide resolved
@superwhd superwhd requested a review from abtink February 13, 2023 02:52
abtink
abtink reviewed Feb 14, 2023
View reviewed changes
Copy link
Member

@abtink abtink left a comment

Choose a reason for hiding this comment

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

Looks great overall. Thanks @superwhd

src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
wgtdkp
wgtdkp reviewed Feb 14, 2023
View reviewed changes
src/dso/dso_transport.hpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
wgtdkp
wgtdkp reviewed Feb 17, 2023
View reviewed changes
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Outdated Show resolved Hide resolved
src/dso/dso_transport.cpp Show resolved Hide resolved
src/dso/dso_transport.hpp Show resolved Hide resolved
bukepo
bukepo reviewed Feb 22, 2023
View reviewed changes
third_party/openthread/CMakeLists.txt Outdated Show resolved Hide resolved
third_party/openthread/CMakeLists.txt Outdated Show resolved Hide resolved
third_party/openthread/CMakeLists.txt Outdated Show resolved Hide resolved
@superwhd superwhd force-pushed the srpl-github-pr branch 8 times, most recently from 1bc6caf to 0f2d158 Compare February 28, 2023 04:17
wgtdkp
wgtdkp reviewed Feb 28, 2023
View reviewed changes
tests/unit/test_dso_transport.cpp Outdated Show resolved Hide resolved
tests/unit/test_dso_transport.cpp Show resolved Hide resolved
tests/unit/test_dso_transport.cpp Show resolved Hide resolved
@superwhd superwhd force-pushed the srpl-github-pr branch 2 times, most recently from 5f323b2 to 719eafc Compare March 2, 2023 07:43
@superwhd superwhd requested a review from wgtdkp March 6, 2023 02:33
abtink
abtink approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@abtink abtink left a comment

Choose a reason for hiding this comment

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

Thanks @superwhd, looks great. 👍

src/srpl_dnssd/srpl_dnssd.cpp Show resolved Hide resolved
src/srpl_dnssd/srpl_dnssd.cpp
partnerInfo.mTxtLength = aInstanceInfo.mTxtData.size();
partnerInfo.mSockAddr.mPort = aInstanceInfo.mPort;
}
otPlatSrplHandleDnssdBrowseResult(mNcp.GetInstance(), &partnerInfo);
Copy link
Member

Choose a reason for hiding this comment

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

We have this requirement from platform API:

If the IPv6 address of the an already discovered/added service changes, then the platform MUST first call
otPlatSrplHandleDnssdBrowseResult() to remove the old address, before calling it again to add the new
address.

Will this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the logic to support this. Note that currently only 1 address is returned in a callback. When a service with N addresses is discovered, it triggers N add events and N - 1 remove events which results in unnecessary operations. It doesn't break my test though. I'll work on supporting multiple addresses later.

src/dso/dso_transport.cpp Show resolved Hide resolved
@superwhd superwhd mentioned this pull request Mar 16, 2023
Draft
@muralidhar-bn
Copy link

This PR implements the platform code for supporting SRPL. It depends on the openthread PR openthread/openthread#8403.

This PR also contains the commit of #1742. Please review the most recent commit only.

Future work:

  • Currently the DSO transport is based on DNS-over-TCP. We need to support DNS-over-TLS later.
  • We may improve the way of handling multiple addresses of a single peer.
  • We need to let DNS-SD API handle the case when mDNS publisher is not ready.

Test:

  • I implemented a basic docker based BR test case for verifying the SRPL functionality. I'll send another PR to add the test case in openthread repo.
  • I set up 2 Raspberry Pi based OTBRs and manually verified some basic scenarios.

We have created a environment with 2 OTBR's ( RPi4 with skyconnect RCP devices ) and 2 or more nanoleaf bulbs.
We want to test SPR replication using real environment. Can you provide test steps ?

  1. Taken both patches - [srpl] integrate SRPL #1743 & [srpl] adding support for SRP replication openthread#8403 in our local environment and build by enabling SRPL replication.
  2. We are getting build errors.

enthread-spinel-rcp.a third_party/openthread/repo/src/lib/url/libopenthread-url.a -lutil -lrt -lanl && :
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:723: error: undefined reference to 'otPlatSrplDnssdBrowse'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:755: error: undefined reference to 'otPlatSrplUnregisterDnssdService'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:723: error: undefined reference to 'otPlatSrplDnssdBrowse'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:745: error: undefined reference to 'otPlatSrplRegisterDnssdService'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:723: error: undefined reference to 'otPlatSrplDnssdBrowse'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:723: error: undefined reference to 'otPlatSrplDnssdBrowse'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:755: error: undefined reference to 'otPlatSrplUnregisterDnssdService'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:745: error: undefined reference to 'otPlatSrplRegisterDnssdService'
| /usr/src/debug/ot-br-posix/git/third_party/openthread/repo/src/core/net/srp_replication.cpp:755: error: undefined reference to 'otPlatSrplUnregisterDnssdService'
4. Can u suggest how to resolve these errors ? Above code is available in simulation folder could be the reason ?

@superwhd superwhd mentioned this pull request Sep 24, 2024
Open
@superwhd
Copy link
Contributor Author

Hi @muralidhar-bn let's continue the discussion in #2505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abtink abtink abtink approved these changes

@librasungirl librasungirl Awaiting requested review from librasungirl

@bukepo bukepo Awaiting requested review from bukepo

@wgtdkp wgtdkp Awaiting requested review from wgtdkp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@superwhd @muralidhar-bn @bukepo @wgtdkp @abtink