add mqtt error type #136
add mqtt error type #136
Conversation
cosmetics
cosmetics
|
I am okay with this change, it seems to address the concerns. |
| @@ -67,6 +67,16 @@ enum ConnectionState | |||
| STATE_CONNECTED, | |||
| STATE_DISCONNECTED | |||
| }; | |||
| namespace MqttError { | |||
There was a problem hiding this comment.
no need in namespace
There was a problem hiding this comment.
Without namespace, the same element between ClientError and MqttError will conflict
| _mqttErrorHash.insert(MqttError::MqttServerUnavailableError, MqttServerUnavailableError); | ||
| _mqttErrorHash.insert(MqttError::MqttVadUserNameOrPasswordError, MqttVadUserNameOrPasswordError); | ||
| _mqttErrorHash.insert(MqttError::MqttNotAuthorizedError, MqttNotAuthorizedError); | ||
|
|
There was a problem hiding this comment.
Maintain the same processing mode as QAbstractSocket.
src/mqtt/qmqtt_client_p.cpp
Outdated
| @@ -39,6 +39,7 @@ | |||
| #include <QSslConfiguration> | |||
| #include <QSslKey> | |||
| #endif // QT_NO_SSL | |||
| #include <QDebug> | |||
src/mqtt/qmqtt_client_p.cpp
Outdated
| if (ack==0) { | ||
| emit q->connected(); | ||
| } | ||
| emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError)); |
There was a problem hiding this comment.
switch-case loop would be a lot cheaper
There was a problem hiding this comment.
emits error() right after connected(). did you ever test your change-set?
There was a problem hiding this comment.
- Ask==0 is the most common case, using "if" is more efficient, and the error is relatively small.
- There is no problem with my program.
There was a problem hiding this comment.
According to me the if needs to be changed to something likes this:
if (ack==0)
emit q->connected();
else
emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError));
There was a problem hiding this comment.
@mwallnoefer You're right. We don't want to emit the error signal if there is no error.
There was a problem hiding this comment.
@KonstantinRitt A switch-case would be cheaper indeed. But I guess @yinbaoqiang used this approach, because it was already used for the socket errors. I would accept this pull request first and create the switch-case later.
src/mqtt/qmqtt_client_p.cpp
Outdated
| emit q->connected(); | ||
| } | ||
| emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError)); | ||
|
|
src/mqtt/qmqtt_client_p.cpp
Outdated
| @@ -440,8 +440,7 @@ void QMQTT::ClientPrivate::handleConnack(const quint8 ack) | |||
| if (ack==0) { | |||
| emit q->connected(); | |||
| } | |||
| emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError)); | |||
|
|
|||
| emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError)); | |||
There was a problem hiding this comment.
plz squash with previous commit
src/mqtt/qmqtt_client_p.cpp
Outdated
| @@ -39,7 +39,6 @@ | |||
| #include <QSslConfiguration> | |||
| #include <QSslKey> | |||
| #endif // QT_NO_SSL | |||
| #include <QDebug> | |||
There was a problem hiding this comment.
plz squash with previous commit
| @@ -67,6 +67,16 @@ enum ConnectionState | |||
| STATE_CONNECTED, | |||
| STATE_DISCONNECTED | |||
| }; | |||
| namespace MqttError { | |||
| enum MqttError { | |||
There was a problem hiding this comment.
This is an internal enum, and should not be part of the public interface. Move it to qmqtt_client_p.cpp where it is used.
The name of the first value of enum MqttError is incorrect (the value indicates that the connection is OK). It is not used as well, so remove it. This will also remove the need to the MqttError namespace.
src/mqtt/qmqtt_client_p.cpp
Outdated
| if (ack==0) { | ||
| emit q->connected(); | ||
| } | ||
| emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError)); |
There was a problem hiding this comment.
@mwallnoefer You're right. We don't want to emit the error signal if there is no error.
src/mqtt/qmqtt_client_p.cpp
Outdated
| if (ack==0) { | ||
| emit q->connected(); | ||
| } | ||
| emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError)); |
There was a problem hiding this comment.
@KonstantinRitt A switch-case would be cheaper indeed. But I guess @yinbaoqiang used this approach, because it was already used for the socket errors. I would accept this pull request first and create the switch-case later.
Cleanup of pull request #136
|
Closed in favour of pull request #138. |
add mqtt error type , Re-edit for pull #135 .
Solve the #133 problem.
Add enum ClientError element .
MqttUnacceptableProtocolVersionError=1<<16, MqttIdentifierRejectedError, MqttServerUnavailableError, MqttVadUserNameOrPasswordError, MqttNotAuthorizedErrorModify the handleConnack ack !=0 to issue an error message number.
`void QMQTT::ClientPrivate::handleConnack(const quint8 ack)
{
Q_Q(Client);
if (ack==0) {
emit q->connected();
}
emit q->error(_mqttErrorHash.value(MqttError::MqttError (ack), UnknownError));
}`