-
Notifications
You must be signed in to change notification settings - Fork 224
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
Update the Web service to reduce compilation time #67
Conversation
03c8328
to
0137873
Compare
src/web/web-service/mdns_service.cpp
Outdated
Json::Value root; | ||
Json::FastWriter jsonWriter; | ||
Json::Reader reader; | ||
static std::string response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any special consideration to make response
static?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't add static
, there will be a warning message like warning: reference to local variable ‘response’ returned
during compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is caused by returning reference. I think the prototype should be changed to return value instead of reference.
Setting as static variable is not thread safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your comments, I have updated this part.
src/web/web-service/mdns_service.cpp
Outdated
}); | ||
|
||
mdnsPublisherThread.detach(); | ||
(void) aNetworkName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aNetworkName
is actually used, same for aExtPanid
.
src/web/web-service/mdns_service.cpp
Outdated
ot::Mdns::Publisher::GetInstance().SetNetworkNameTxt(networkNameTxt.c_str()); | ||
ot::Mdns::Publisher::GetInstance().SetExtPanIdTxt(extPanIdTxt.c_str()); | ||
ot::Mdns::Publisher::GetInstance().UpdateService(); | ||
(void) aNetworkName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used.
src/web/web-service/mdns_service.cpp
Outdated
ot::Mdns::Publisher::GetInstance().SetExtPanIdTxt(extPanIdTxt.c_str()); | ||
ot::Mdns::Publisher::GetInstance().UpdateService(); | ||
(void) aNetworkName; | ||
(void) aExtPanId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used.
src/web/web-service/web_server.cpp
Outdated
|
||
WebServer::~WebServer(void) | ||
{ | ||
delete &mServer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be
delete mServer;
src/web/web-service/web_server.cpp
Outdated
void DefaultResourceSend(const HttpServer &aServer, const std::shared_ptr<HttpServer::Response> &aResponse, | ||
const std::shared_ptr<std::ifstream> &aIfStream) | ||
{ | ||
static std::vector<char> buffer(131072); // Safe when server is running on one thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magic number, and can this be smaller?
@@ -241,13 +255,18 @@ | |||
data: data, | |||
}); | |||
|
|||
data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var data
at function begining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done thanks.
0137873
to
249e2f6
Compare
@@ -219,12 +233,12 @@ | |||
.targetEvent(ev) | |||
.ok('Okay') | |||
.cancel('Cancel'); | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: line only space.
src/web/web-service/wpan_service.cpp
Outdated
otbrLog(OTBR_LOG_ERR, "wpan service error: %d", ret); | ||
root["result"] = "failed"; | ||
} | ||
root["result"] = "successful"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to define "successful"
and "fail"
as macro or const string?
src/web/web-service/wpan_service.cpp
Outdated
otbrLog(OTBR_LOG_ERR, "wpan service error: %d", ret); | ||
root["result"] = "failed"; | ||
} | ||
root["result"] = "successful"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems line 198 has no effect with this line.
src/web/web-service/wpan_service.cpp
Outdated
otbrLog(OTBR_LOG_ERR, "wpan service error: %d", ret); | ||
root["result"] = "failed"; | ||
} | ||
root["result"] = "successful"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
src/web/web-service/wpan_service.hpp
Outdated
ot::Dbus::WpanNetworkInfo mNetworks[DBUS_MAXIMUM_NAME_LENGTH]; | ||
int mNetworksCount; | ||
char mIfName[IFNAMSIZ]; | ||
std::string mNetworkName, mExtPanId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although not listed in the code style, it seems OpenThread defines member variables one by one.
@@ -68,7 +68,7 @@ DBusMessage *DBusBase::GetMessage(void) | |||
{ | |||
int ret = kWpantundStatus_Ok; | |||
|
|||
VerifyOrExit(mDestination != NULL, ret = kWpantundStatus_InvalidArgument); | |||
VerifyOrExit(strlen(mDestination) != 0, ret = kWpantundStatus_InvalidArgument); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strnlen()
preferred.
9105e04
to
4e793ef
Compare
src/web/web-service/mdns_service.cpp
Outdated
} | ||
root["result"] = mResponseSuccess; | ||
exit: | ||
(void)aMdnsRequest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used
@@ -127,6 +147,15 @@ class Publisher | |||
return publisherInstance; | |||
} | |||
|
|||
/** | |||
* This method returns the boolean result of mDNS publisher. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this line exactly describe the functionality?
src/web/web-service/mdns_service.cpp
Outdated
{ | ||
ret = StartMdnsService(networkName, extPanId); | ||
} | ||
root["result"] = mResponseSuccess; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this line take effect?
|
||
int MdnsService::UpdateMdnsService(const std::string &aNetworkName, const std::string &aExtPanId) | ||
{ | ||
int ret = kMndsServiceStatus_OK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this method will always success?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Thanks.
src/web/web-service/wpan_service.cpp
Outdated
ret = ot::Dbus::kWpantundStatus_SetGatewayFailed); | ||
|
||
ot::Utils::Long2Hex(mNetworks[index].mExtPanId, extPanId); | ||
root["result"] = mResponseSuccess; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this line take effect?
src/web/web-service/wpan_service.cpp
Outdated
wpanController.SetInterfaceName(mIfName); | ||
VerifyOrExit(wpanController.AddGateway(prefix.c_str(), defaultRoute) == ot::Dbus::kWpantundStatus_Ok, | ||
ret = ot::Dbus::kWpantundStatus_SetGatewayFailed); | ||
root["result"] = mResponseSuccess; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this take effect?
src/web/web-service/wpan_service.cpp
Outdated
wpanController.SetInterfaceName(mIfName); | ||
VerifyOrExit(wpanController.RemoveGateway(prefix.c_str()) == ot::Dbus::kWpantundStatus_Ok, | ||
ret = ot::Dbus::kWpantundStatus_SetGatewayFailed); | ||
root["result"] = mResponseSuccess; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
src/web/web-service/wpan_service.cpp
Outdated
Json::FastWriter jsonWriter; | ||
ot::Dbus::WPANController wpanController; | ||
std::string response, networkName, extPanId; | ||
int ret = ot::Dbus::kWpantundStatus_Ok; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems this variable will not be changed at all.
src/web/web-service/wpan_service.cpp
Outdated
status = kWpanStatus_Down; | ||
ExitNow(); | ||
} | ||
if (wpantundState == "associated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to list all possible states and define them as cons string or macros?
src/web/web-service/wpan_service.hpp
Outdated
* WPAN parameter constants | ||
* | ||
*/ | ||
#define OT_BORDER_ROUTER_PORT 49191 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This port is defined twice, is it possible to define it only once?
e1ca276
to
781799d
Compare
src/web/web-service/wpan_service.hpp
Outdated
* This method gets status of wpan service. | ||
* | ||
* @param[in] aNetworkName The pointer to the network name. | ||
* @param[in] aIfName The pointer to the extended PAN ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[inout]
src/web/web-service/wpan_service.hpp
Outdated
* @param[in] aIfName The pointer to the extended PAN ID. | ||
* | ||
* @retval kWpanStatus_OK Started the wpan service. | ||
* @retval kWpanStatus_OffLine Not started the wpan service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kWpanStatus_Offline
Offline is a single word.
src/web/web-service/wpan_service.hpp
Outdated
#define OT_EXTENDED_PANID_LENGTH 8 | ||
#define OT_HARDWARE_ADDRESS_LENGTH 8 | ||
#define OT_NETWORK_NAME_LENGTH 16 | ||
#define OT_PANID_LENGTH 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing, OT_PANID_LENGTH
means the hex length of PANID, while other *_LENGTH
means the bytes length.
Shall we keep consistent?
src/web/web-service/wpan_service.hpp
Outdated
enum | ||
{ | ||
kPropertyType_String = 0, | ||
kPropertyType_Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kPropertyType_Data
=> kPropertyType_Data,
?
src/web/web-service/wpan_service.cpp
Outdated
{ | ||
aNetworkName = wpanController.Get(kWPANTUNDProperty_NetworkName); | ||
aExtPanId = wpanController.Get(kWPANTUNDProperty_NetworkXPANID); | ||
aExtPanId = aExtPanId.substr(2, aExtPanId.length() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this line should be
aExtPanId = aExtPanId.substr(2, aExtPanId.length() - 2);
or simply
aExtPanId = aExtPanId.substr(2);
else if (wpantundState.find(kWPANTUNDStateAssociating) != std::string::npos) | ||
{ | ||
status = kWpanStatus_Associating; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Thanks.
src/web/web-service/wpan_service.cpp
Outdated
break; | ||
|
||
default: | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in this case?
6b52dec
to
705be5e
Compare
src/web/web-service/web_server.hpp
Outdated
WebServer(void); | ||
|
||
/** | ||
* This method is disconstructor to free the WebServer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
705be5e
to
0ddf407
Compare
*/ | ||
enum | ||
{ | ||
kMdnsPublisher_OK = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add doxygen for each enum.
@@ -127,6 +147,15 @@ class Publisher | |||
return publisherInstance; | |||
} | |||
|
|||
/** | |||
* This method returns the boolean result of mDNS service status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to: "This method indicates whether or not the mDNS service is running."
* @retval false Not started the mdns service. | ||
* | ||
*/ | ||
bool IsStarted() const { return mIsStarted; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer: IsRunning()
.
* | ||
* @param[in] aNetworkName The reference to the network name. | ||
* @param[in] aExtPanId The reference to the extend PAN ID. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document return value(s).
* | ||
* @param[in] aNetworkName The reference to the network name. | ||
* @param[in] aExtPanId The reference to the extend PAN ID. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document return value(s).
|
||
/** | ||
* This method handles the http request to add on-mesh prefix. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document parameters.
|
||
/** | ||
* This method handles the http request to delete on-mesh prefix http request. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document parameters.
src/web/web-service/wpan_service.hpp
Outdated
* This method gets status of wpan service. | ||
* | ||
* @param[inout] aNetworkName The pointer to the network name. | ||
* @param[inout] aIfName The pointer to the extended PAN ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix alignment.
src/web/web-service/mdns_service.hpp
Outdated
|
||
/** | ||
* This method handles http request to advertise service. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document parameters.
src/web/web-service/mdns_service.hpp
Outdated
* @retval false Not started the mdns service. | ||
* | ||
*/ | ||
bool IsStartedService() { return ot::Mdns::Publisher::GetInstance().IsStarted(); }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All void functions or methods shall explicitly declare and specify the void type keyword.
0ddf407
to
318ddb9
Compare
- Divide the web service into HTTP server, WPAN service and mDNS service. - Support configurable HTTP server port (default 80). - Show services status on Web page. - Support google-chrome display the Web page normally.
318ddb9
to
cd37163
Compare
The previous version involved http server and backend business logic (such as wpan and mDNS) into the same module, which is not suitable for code maintenance and migration, in addition, it cost lots of resource during compilation.
This PR implements the following features: