-
Notifications
You must be signed in to change notification settings - Fork 11
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
Implement 2 buffers for receiving and sending #10
Conversation
Merge pull request knolleary#9 from MathewHDYT/master
…iting of received data when sending
@imbeacon Would be nice if this could be merged as well, and released as |
@MathewHDYT |
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 commit and the released version 2.10 has a problem making it impossible to connect to password protected brokers.
if(pass != NULL) { | ||
CHECK_STRING_LENGTH(length,pass) | ||
length = writeString(pass,this->buffer,length); | ||
length = writeString(pass,this->send_buffer,length); |
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.
unfortunately this breaks the password: If a password is available, it is appended to incorrect buffer and therefore not sent to server.
This makes the client never connect to password protected brokers.
Please release a new version. I will provide a PR tomorrow to fix this and some issues with keepalive handling.
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.
The password must be appended to the receive_buffer which is also used for sending username.
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.
@uschindler Already fixed in #12 Pull 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.
Thanks very much! This issue is serious, so this PR should be merged soon. It makes it impossible to use a password for the broker.
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.
and #11 fixes it, too.
When sending data in the user subscribed callbacks, of the
thingsboard-client-sdk
it overwrites the received payload and therefore garbles up the response for all future callbacks that also wanted to handle that data.To fix that issue I've seperated the buffer into a receive and send buffer, so that even if you send data while handling received data the response will not be overwritten. This also fixes the previous need to copy the payload temporarily in the callback of the
thingsboard-client-sdk
. Closes #8.Would be nice if this could be merged @imbeacon. So it can be released as
v2.10.0
.