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

TCP improvements #1595

Merged
merged 6 commits into from
Feb 6, 2019
Merged

TCP improvements #1595

merged 6 commits into from
Feb 6, 2019

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Feb 5, 2019

Consistent use of braces
Tidy #includes
NULL -> nullptr
Move trivial code into header
Some methods are const
Ensure ALL member variables have initializers; remove corresponding redundant assignments from constructors
Used unsigned types where appropriate

Remove TcpServer as friend of TcpConnection, use accessor methods instead

TcpConnection: Move callback handler code out of static methods

@mikee47 mikee47 changed the title Fix/tcp improvements TCP improvements Feb 5, 2019
@slaff slaff added this to the 3.7.2 milestone Feb 5, 2019
slaff
slaff requested changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@slaff slaff left a comment

Choose a reason for hiding this comment

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

Small things to double-check.

Sming/SmingCore/Network/MqttClient.h Outdated
#include "Data/ObjectQueue.h"
#include "Mqtt/MqttPayloadParser.h"
#include "../mqtt-codec/src/message.h"
#include "../mqtt-codec/src/serialiser.h"
#include "../mqtt-codec/src/parser.h"
#include <functional>
//#include <functional>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the commented line.

Sming/SmingCore/Network/MqttClient.h
@@ -316,9 +322,9 @@ class MqttClient : protected TcpClient

#ifndef MQTT_NO_COMPAT
// @deprecated
MqttMessageDeliveredCallback onDelivery = 0;
MqttMessageDeliveredCallback onDelivery = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are the old-style C++ delegates I think that using nullptr instead of 0 will not work as expected. I seem to recall that we already had such an issue but I am not 100% certain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should match Delegate(FunctionDeclaration m) constructor as FunctionDeclaration is a pointer type ReturnType (*FunctionDeclaration)(ParamsList...). Can't see how else it could be interpreted?

Sming/SmingCore/Network/MqttClient.h
// @deprecated
MqttStringSubscriptionCallback subscriptionCallback = 0;
MqttStringSubscriptionCallback subscriptionCallback = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using nullptr instead of 0 for old-style C++ delegates might not work as expected. Has to be checked.

Sming/SmingCore/Network/TcpConnection.h Outdated
@@ -228,6 +268,16 @@ class TcpConnection
virtual err_t onSslConnected(SSL* ssl);
#endif

// These do most of the work - called from static methods
err_t tcpOnConnected(err_t err);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename the tcpOn* to interalOn*? Otherwise having staticOnConnect, tcpOnConnect, onConnect will cause additional confusion as to which method is the right one to use.

While internalOn* might be a bit better than tcpOn* and not quite happy with it so I am also open to suggestions from your side.

Copy link
Contributor Author

@mikee47 mikee47 Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We could adopt the LWIP convention e.g. tcp_connected instead of tcpOnConnect to indicate it's all happening at a lower level. Could do the same with the static methods, e.g. static_tcp_connected.

It would be helpful to have a consistent naming convention for these 'glue' functions/methods. Closures could replace the static methods but in this instance I think keeping the static methods makes the code easier to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noticed, we already have an internalTcpConnect!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will come up again handling HTTP and Websocket parser callbacks. These are simpler though as the static methods are all virtually identical (just a null pointer check then call the method). Both parsers use a function table, so I adopted the same names as those to help make things clearer, e.g. HttpConnection::on_message_begin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed, we already have an internalTcpConnect!

Nice catch :)

For consistency I can suggest the following method renames:

internalTcpConnect -> internalConnect
tcpOn* -> internalOn* (Ex: tcpOnConnected -> internalOnConnected )

Can you do that change? And don't forget to remove the commented line 20 in Sming/SmingCore/Network/MqttClient.h.

@mikee47
Minor refactoring
8101b12 
Consistent use of braces
Tidy #includes
NULL -> nullptr
Move trivial code into header
Some methods are const
Ensure ALL member variables have initializers; remove corresponding redundant assignments from constructors
Used unsigned types where appropriate
@mikee47
Remove TcpServer as friend of TcpConnection, use accessor methods…
d148873 
… instead
@mikee47
TcpConnection: Move callback handler code out of static methods
ef7ae74
@mikee47
Fix codacy issues
77d11e6
@mikee47 mikee47 force-pushed the fix/tcp_improvements branch from b7c40bd to d879a47 Compare February 6, 2019 10:39
@mikee47
Rename tcpOn* -> internalOn*, and internalTcpConnect -> `internalCo…
5e88167 
…nnect`

Remove unused statements
Replace commented-out `debug_d` statements with `debug_tcp`, conditionally defined
@mikee47
Replace single-use static callback methods with lambdas
41041f2
@mikee47 mikee47 force-pushed the fix/tcp_improvements branch from d879a47 to 41041f2 Compare February 6, 2019 10:50
@slaff
Copy link
Contributor

slaff commented Feb 6, 2019

@mikee47 Is the PR tested from your side and ready for merging or you plan to add more changes?

@mikee47
Copy link
Contributor Author

mikee47 commented Feb 6, 2019

@mikee47 Is the PR tested from your side and ready for merging or you plan to add more changes?

That's it for this PR.

@slaff slaff merged commit fa42205 into SmingHub:develop Feb 6, 2019
@slaff slaff removed the 3 - Review label Feb 6, 2019
@mikee47 mikee47 deleted the fix/tcp_improvements branch February 6, 2019 12:45
@slaff slaff mentioned this pull request Feb 27, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slaff slaff slaff requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@mikee47 @slaff