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

[example] add minimum thread commissioner example #167

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wgtdkp
Copy link
Member

@wgtdkp wgtdkp commented Feb 3, 2021

This PR adds a minimum Thread Commissioner example that demos how to build a Commissioner Application with the OT Commissioner library.

resolves issue: #165

@wgtdkp wgtdkp added the enhancement New feature or request label Feb 3, 2021
@wgtdkp wgtdkp mentioned this pull request Feb 3, 2021
@wgtdkp wgtdkp force-pushed the mini-commissioner branch 2 times, most recently from 571de82 to 523346c Compare February 3, 2021 14:21
@krbvroc1
Copy link

krbvroc1 commented Feb 3, 2021

Here are my initial comments:

Installing the static libraries appear to work now. And uninstall also works.

-rw-r--r--  1 root root  25757164 Feb  3 15:34 libcommissioner.a
-rw-r--r--  1 root root   3420392 Feb  3 15:34 libcommissioner-common.a
-rw-r--r--  1 root root    778910 Feb  3 15:33 libevent_core.a
-rw-r--r--  1 root root     15028 Feb  3 15:33 libevent_pthreads.a
-rw-r--r--  1 root root   2787040 Feb  3 15:34 libfmtd.a
-rw-r--r--  1 root root   5580184 Feb  3 15:34 libmbedcrypto.a
-rw-r--r--  1 root root   1884926 Feb  3 15:34 libmbedtls.a
-rw-r--r--  1 root root   1080600 Feb  3 15:34 libmbedx509.a
-rw-r--r--  1 root root     27516 Feb  3 15:34 libmdns.a

I followed the instructions in BUILDING.md and it does not work. It seems the CMakeList.txt is not complete.

$ cmake -GNinja ..
CMake Error at CMakeLists.txt:29 (project):
  VERSION not allowed unless CMP0048 is set to NEW

CMake Warning (dev) in CMakeLists.txt:
  No cmake_minimum_required command is present.  A line of code such as

    cmake_minimum_required(VERSION 3.13)

  should be added at the top of the file.  The version specified may be lower
  if you wish to support older CMake versions for this project.  For more
  information run "cmake --help-policy CMP0000".
This warning is for project developers.  Use -Wno-dev to suppress it.

I created my own out of tree mini commissioner and I needed the following (I am a cmake novice):

target_link_libraries(mini_commissioner
  commissioner
  commissioner-common
  event_core
  event_pthreads
  mbedtls
  mbedx509
  mbedcrypto
  mdns
  fmtd
  pthread
)

For a command line compile, rather than what is in BUILDING.md, I had to use the following:

c++ main.cpp -o mini_commissioner -lcommissioner -lcommissioner-common -levent_core -levent_pthreads -lmbedtls -lmbedx509 -lmbedcrypto -lmdns -lfmtd -lpthread

Not sure how difficult it is or if you have a suggestion... It would be nice if the ninja install of the libraries would install the associated fmt/fmtlib include files to /usr/local/include so the mini_commissioner can use the installed library. Maybe there is a better way, but right now I have to include ot-commissioner/third_party/fmtlib/repo/include/fmt/ to be able to use the installed library for printing. Maybe this is out of scope.

I also noticed as part of this PR you made a change to src/library/commissioner_impl.cpp and how OnJoinerRequest() is called. What is the purpose of that change?

@wgtdkp
Copy link
Member Author

wgtdkp commented Feb 4, 2021

@krbvroc1

I followed the instructions in BUILDING.md and it does not work. It seems the CMakeList.txt is not complete.

What platform and CMake version are you using? For whatever failure reason, I updated the CMake file to define the mininum required version, please let me know if this still not working for your platform.

Not sure how difficult it is or if you have a suggestion... It would be nice if the ninja install of the libraries would install the associated fmt/fmtlib include files to /usr/local/include so the mini_commissioner can use the installed library. Maybe there is a better way, but right now I have to include ot-commissioner/third_party/fmtlib/repo/include/fmt/ to be able to use the installed library for printing. Maybe this is out of scope.

I think this should not be addressed by the commissioner libary. We don't include any third party headers in the commissioner public headers, so there is no necessary to (and should not) install those thrid-party headers. If your project depends on fmtlib, then I think it is your project that should worry about installing and including fmtlib, either the one in commissioner third-party or another independent copy (e.g. installed in system directories).

I also noticed as part of this PR you made a change to src/library/commissioner_impl.cpp and how OnJoinerRequest() is called. What is the purpose of that change?

It is to make the OnJoinerRequest() called only for the first time a packet from joiner is received, but not every time: We just need the pskd for the first time.

For a command line compile, rather than what is in BUILDING.md, I had to use the following:
c++ main.cpp -o mini_commissioner -lcommissioner -lcommissioner-common -levent_core -levent_pthreads -lmbedtls -lmbedx509 -lmbedcrypto -lmdns -lfmtd -lpthread

Sorry I was building shared libraries, I updated the document to to build shared libraries to get rid of the long list third-party libs.

@wgtdkp wgtdkp force-pushed the mini-commissioner branch from 523346c to 5672a21 Compare February 4, 2021 02:31
@krbvroc1
Copy link

krbvroc1 commented Feb 4, 2021

What platform and CMake version are you using? For whatever failure reason, I updated the CMake file to define the mininum required version, please let me know if this still not working for your platform.

That change is working.

cmake version 3.13.4 - running on the latest Raspbian version. (Compiling on an OTBR).

Thank you!

@wgtdkp
Copy link
Member Author

wgtdkp commented Feb 4, 2021

cmake version 3.13.4 - running on the latest Raspbian version. (Compiling on an OTBR).

That's weird, I am using a CMake with lower version but has no errors. Maybe new CMake is more strict about this...

@wgtdkp wgtdkp force-pushed the mini-commissioner branch 2 times, most recently from 4f5cfb2 to a18491e Compare February 4, 2021 06:16
Base automatically changed from master to main March 8, 2021 21:58
@wgtdkp wgtdkp force-pushed the mini-commissioner branch from a18491e to 62a3a20 Compare April 18, 2021 04:47
@google-cla google-cla bot added the cla: yes label Apr 18, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2021

Codecov Report

Merging #167 (4fde540) into main (79ad31d) will not change coverage.
The diff coverage is n/a.

❗ Current head 4fde540 differs from pull request most recent head 92ce01f. Consider uploading reports for the commit 92ce01f to get more accurate results

@@           Coverage Diff           @@
##             main     #167   +/-   ##
=======================================
  Coverage   68.90%   68.90%           
=======================================
  Files          55       55           
  Lines        4933     4933           
=======================================
  Hits         3399     3399           
  Misses       1534     1534           

@wgtdkp wgtdkp force-pushed the mini-commissioner branch 6 times, most recently from f9865d1 to df55462 Compare April 18, 2021 10:58
@wgtdkp
Copy link
Member Author

wgtdkp commented Apr 18, 2021

This PR depends on #188: the second commit is a cherry-pick of #188.

@wgtdkp wgtdkp force-pushed the mini-commissioner branch 2 times, most recently from c563d66 to 8fecf35 Compare April 18, 2021 12:08
@wgtdkp wgtdkp changed the title [example] Adds minimum thread commissioner example [example] add minimum thread commissioner example Apr 18, 2021
@wgtdkp wgtdkp force-pushed the mini-commissioner branch from 8fecf35 to e9f8a58 Compare June 8, 2021 09:38
@wgtdkp wgtdkp force-pushed the mini-commissioner branch from e9f8a58 to 193bb9a Compare June 8, 2021 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants