-
Notifications
You must be signed in to change notification settings - Fork 365
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
Bugfix tcp #120
Bugfix tcp #120
Conversation
The order of numberOfThreads and idleTimeout are exchange in TcpServerConnector. So moving numberOfThreads before idleTimeout will harmonice the API and fix the "parameter exchange", when TlsServerConnector calls super. Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
coap+tcp doesn't use the message type, but currently some other code rely on type being set (e.g. some Message Tracer). Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
If stop is called twice, a NullPointerException is caused by workerGroup being set to null at the first call. Add synchronize as in UDPConnector. Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
If stop is called twice, a NullPointerException is caused by workerGroup and bossGroup being set to null at the first call. Add synchronize as in UDPConnector. Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
Kai, this looks good to me. |
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.
just some minor formatting issues
@@ -106,13 +107,15 @@ public CoapTcpStack(NetworkConfig config, Outbox outbox) { | |||
|
|||
// delegate to top | |||
@Override public void sendRequest(Request request) { | |||
if (null == request.getType()) request.setType(Type.CON); |
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.
please put the statement on a new line and in between curly braces
top.sendRequest(request); | ||
// CoAP over TCP does not have acknowledgements. Everything is automatically acknowledged. | ||
request.setAcknowledged(true); | ||
} | ||
|
||
// delegate to top | ||
@Override public void sendResponse(Exchange exchange, Response response) { | ||
if (null == response.getType()) response.setType(Type.CON); |
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.
please put the statement on a new line and in between curly braces
@@ -26,6 +26,9 @@ | |||
@RunWith(Parameterized.class) | |||
public class TcpConnectorTest { | |||
|
|||
private static final int NUMBER_OF_THREADS = 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.
indentation?
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.
Hell, so californium uses tabs, but connector spaces?
@@ -35,6 +35,8 @@ | |||
|
|||
public class TlsConnectorTest { | |||
|
|||
private static final int NUMBER_OF_THREADS = 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.
indentation?
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.
About the formatting:
sorry I'm a little lost:
@Override public void destroy() {
for (Layer layer : layers)
layer.destroy();
}
@Override public void receiveRequest(Exchange exchange, Request request) {
// if there is no BlockwiseLayer we still have to set it
if (exchange.getRequest() == null)
exchange.setRequest(request);
if (hasDeliverer()) {
deliverer.deliverRequest(exchange);
} else {
LOGGER.severe("Top of CoAP stack has no deliverer to deliver request");
}
}
So, is it OK to correct only my changes, or is more required?
Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
I would like to squash all of these commits into one since they are all fixing small issues with the TCP support. This will also make it easier to cherry pick it on master. Any objections? |
OK, from my side. |
merged |
Correct parameter order in TcpServerConnector.
Correct "stop()" to be executable multiple times.
#118