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

[web] topology visualization #519

Merged
merged 226 commits into from
Sep 2, 2020
Merged

Conversation

tttttangTH
Copy link
Contributor

@tttttangTH tttttangTH commented Jul 10, 2020

It is the first PR of OpenThread Network Manager.

In general, it implements a new module -- OTBR-REST, which is directly embedded in OTBR-AGENT and provides RESTful API for a new feature of WEB UI -- Topology Visualization.

Topology Visualization aims to collect diagnostic information of all nodes in current network, visualize their relations and provide the display of all the details.

The frontend part is implemented based on the frontend of OTBR-WEB for the time being. We will then implement all the functionality OTBR-WEB provides and transplant the frontend static file of OTBR-WEB in this module. More details of the second PR is in #537 .

@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2020

This pull request introduces 2 alerts when merging 210dc18 into eefef71 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable
  • 1 for 'new[]' array freed with 'delete'

@tttttangTH tttttangTH requested a review from wgtdkp July 10, 2020 14:18
@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #519 into master will increase coverage by 1.36%.
The diff coverage is 84.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   76.85%   78.21%   +1.36%     
==========================================
  Files          57       68      +11     
  Lines        3850     4683     +833     
==========================================
+ Hits         2959     3663     +704     
- Misses        891     1020     +129     
Impacted Files Coverage Δ
src/rest/request.cpp 73.33% <73.33%> (ø)
src/rest/connection.cpp 73.88% <73.88%> (ø)
src/rest/json.cpp 77.86% <77.86%> (ø)
src/rest/parser.cpp 86.84% <86.84%> (ø)
src/rest/rest_web_server.cpp 88.04% <88.04%> (ø)
src/rest/resource.cpp 94.56% <94.56%> (ø)
src/rest/response.cpp 94.59% <94.59%> (ø)
src/agent/main.cpp 92.22% <100.00%> (+0.45%) ⬆️
src/rest/connection.hpp 100.00% <100.00%> (ø)
src/rest/request.hpp 100.00% <100.00%> (ø)
... and 14 more

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 bc8825d...474d15c. Read the comment docs.

@wgtdkp wgtdkp changed the title Topology visualization pre-review [web] topology visualization Jul 11, 2020
@simonlingoogle
Copy link
Member

Why not use git submodules for cJSON and http_parser ? @tttttangTH @wgtdkp

src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.hpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
src/agent/rest_web_server.cpp Outdated Show resolved Hide resolved
@wgtdkp
Copy link
Member

wgtdkp commented Jul 12, 2020

Why not use git submodules for cJSON and http_parser ? @tttttangTH @wgtdkp

I think the benefits of adding source code is that we don't need to make a mirroring of each third_party library when porting to product environment where we cannot depends on external libraries directly. This is how OpenThread add third_party dependencies. OpenThread is added as a git submodule, because there is already an OpenThread mirroring in internal code base.

@wgtdkp
Copy link
Member

wgtdkp commented Jul 12, 2020

@tttttangTH I think the first thing for you is to format the code with script/make-pretty clang.

@lgtm-com
Copy link

lgtm-com bot commented Jul 13, 2020

This pull request introduces 2 alerts when merging 83eaafd into eefef71 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class
  • 1 for 'new[]' array freed with 'delete'

@lgtm-com
Copy link

lgtm-com bot commented Jul 13, 2020

This pull request introduces 3 alerts when merging 0b136da into eefef71 - view on LGTM.com

new alerts:

  • 1 for Resource not released in destructor
  • 1 for Unused variable, import, function or class
  • 1 for 'new[]' array freed with 'delete'

@lgtm-com
Copy link

lgtm-com bot commented Jul 14, 2020

This pull request introduces 1 alert when merging e38d30d into eefef71 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jul 14, 2020

This pull request introduces 1 alert when merging 77b0066 into eefef71 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jul 14, 2020

This pull request introduces 1 alert when merging 6329970 into eefef71 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

src/agent/rest_web_server.hpp Outdated Show resolved Hide resolved
src/web/web-service/frontend/res/js/app.js Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jul 15, 2020

This pull request introduces 1 alert when merging a895bd4 into eefef71 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

src/agent/CMakeLists.txt Outdated Show resolved Hide resolved
src/agent/main.cpp Outdated Show resolved Hide resolved
src/agent/main.cpp Outdated Show resolved Hide resolved
src/rest/CMakeLists.txt Outdated Show resolved Hide resolved
src/rest/connection.cpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.cpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.cpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.cpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.hpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.hpp Outdated Show resolved Hide resolved
@wgtdkp
Copy link
Member

wgtdkp commented Jul 17, 2020

@tttttangTH You marked comments resolved, but there is no new commits. Can you confirm that you pushed you changes successfully?

src/agent/CMakeLists.txt Outdated Show resolved Hide resolved
src/rest/connection.cpp Outdated Show resolved Hide resolved
src/rest/connection.cpp Outdated Show resolved Hide resolved
src/rest/connection.hpp Outdated Show resolved Hide resolved
src/rest/connection.hpp Outdated Show resolved Hide resolved
src/rest/request.hpp Outdated Show resolved Hide resolved
src/rest/connection.cpp Outdated Show resolved Hide resolved
void Connection::SentResponse(Response &aResponse)
{
std::string data = aResponse.SerializeResponse();
write(this->mFd, data.c_str(), data.size());
Copy link
Member

Choose a reason for hiding this comment

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

and we might not write all data, only part of it.

src/rest/rest_web_server.hpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.cpp Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2020

This pull request fixes 2 alerts when merging 4fef6cc into ecc2570 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2020

This pull request fixes 2 alerts when merging 59cf99b into ecc2570 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@tttttangTH
Copy link
Contributor Author

@tttttangTH please address comments by LGTM.com

Have fixed that!

third_party/http-parser/CMakeLists.txt Outdated Show resolved Hide resolved
src/rest/CMakeLists.txt Outdated Show resolved Hide resolved
src/rest/connection.cpp Show resolved Hide resolved
src/rest/types.hpp Outdated Show resolved Hide resolved
src/rest/types.hpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.cpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.cpp Show resolved Hide resolved
src/rest/rest_web_server.cpp Show resolved Hide resolved
src/rest/rest_web_server.hpp Outdated Show resolved Hide resolved
src/rest/rest_web_server.cpp Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2020

This pull request fixes 2 alerts when merging fd37cd7 into ecc2570 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2020

This pull request fixes 2 alerts when merging ea3e4d9 into ecc2570 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2020

This pull request fixes 2 alerts when merging 4968af8 into ecc2570 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

[cmake] add option for enabling legacy in cmake (openthread#544)
@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2020

This pull request fixes 2 alerts when merging 6ca68f1 into bc8825d - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2020

This pull request fixes 2 alerts when merging 474d15c into bc8825d - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

Copy link
Member

@wgtdkp wgtdkp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jwhui jwhui merged commit ea9cd69 into openthread:master Sep 2, 2020
simonlingoogle pushed a commit to simonlingoogle/ot-br-posix that referenced this pull request Nov 11, 2020
Bug: 167332999

* origin/github/master:
  [backbone-router] fix OT options for Backone Router (openthread#564)
  [script] add `REFERENCE_DEVICE` support (openthread#558)
  [test] increase the waiting time in dbus tests (openthread#563)
  [flags] configure backbone network interface name (openthread#553)
  [dockerfile] add ARG `OT_BACKBONE_CI` (openthread#556)
  [mac-filter] change whitelist/blacklist to allowlist/denylist (openthread#552)
  [third-party] only add web related modules when necessary (openthread#551)
  [web] topology visualization (openthread#519)
Change-Id: I73c08bf5bc0ebcc9ecb9903a282a2264e6754b0f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants