- 
                Notifications
    
You must be signed in to change notification settings  - Fork 4
 
[PubSubClient3] Speedup publish large messages using buffer #59
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
Conversation
As discussed here: #29 (comment)
| 
           I think this can't be implemented like this. We can't set  pubsubclient3/src/PubSubClient.cpp Line 470 in 67eba23 
 But setting  pubsubclient3/src/PubSubClient.cpp Line 478 in 67eba23 
 MQTTPINGREQ.Maybe we can just flushBuffer() here, check the rc and only if it's 0 do the MQTTPINGREQ.
To your   | 
    
| 
           Well since   | 
    
| 
           I was thinking... maybe all   | 
    
| 
           This was my first thought too. But then more things need to be rewritten ...  | 
    
| 
           And yes, at least here pubsubclient3/src/PubSubClient.cpp Line 549 in 67eba23 
 we should use it as well to solve #36 ...  | 
    
…fter timeout if buffer still has data to send
| 
           @TD-er Did some implementations mentioned above. Looks quite promising. If you have time you may have a look over it.  | 
    
… be done in flushBuffer())
… adding a writeBuffer() function
| 
           @TD-er We could merge  pubsubclient3/src/PubSubClient.cpp Line 768 in 5b2c586 
 into pubsubclient3/src/PubSubClient.cpp Lines 644 to 646 in 5b2c586 
 and remove appendBuffer(uint8_t data). What do you think? Might be less clear ...
       | 
    
| 
           This PR fixes #29.  | 
    
| 
           This fixes #36  | 
    
| 
           @TD-er If you could take a look over it, I'd like to merge this. Tested it for a few weeks on a device of mine (but only uses   | 
    
| 
           @TD-er Is there any chance to finish this PR from your side? I would love to implement some other open issues but this might make extra work merging here ...  | 
    
| 
           I will have a look  | 
    
        
          
                src/PubSubClient.cpp
              
                Outdated
          
        
      | */ | ||
| size_t PubSubClient::writeBuffer(size_t pos, size_t size) { | ||
| size_t rc = 0; | ||
| if (size > 0 && pos + size <= this->bufferSize) { | 
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 know I mentioned it before and it is very likely to be correct C++ syntax, but can you mark either side of the && in braces?
if ((size > 0) && ((pos + size) <= this->bufferSize))makes it easier to read as a human, also because of color matching braces in the IDE.
Same for line 573 and probably others too.
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. Can do this.
| 
           OK, I loaded the files into my (ESPEasy) project and it builds just fine, so that's a plus ;) Also normal MQTT operations do seem to work just fine. Right now I don't have the setup here to dump large chunks, so that's a bit hard to test right now. Just as a cosmetic remark... There is no consistency in naming of member variables. I don't see any obvious mistakes, so LGTM  | 
    
          
 Yeah, that's what I want to do next ... see #60.  | 
    
| 
           Hm, yes. Testing with big data is a good point. Maybe I can create a unit test for that ...  | 
    
| if (!readByte(&digit)) return 0; | ||
| if (this->stream) { | ||
| if (isPublish && idx - *hdrLen - 2 > skip) { | ||
| if (isPublish && (idx - *hdrLen - 2 > skip)) { | 
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.
Also the sum before the compare...
| 
           Unit test runs green 
 With a simple code I just send 6k payload data ... 
 So everything look good for me as well. Merging that.  | 
    
As discussed here: #29 (comment)
Not yet tested, as I really need to get some sleep now...
My biggest concern right now is whether
writeNextMsgIdmight also need to callflushBuffer()first.But as I said, can't think clearly right now so if you can have a look @hmueller01