ItKindaWorks / ESPHelper

A library to make using WiFi & MQTT on the ESP8266 easy.
GNU General Public License v3.0
327 stars 67 forks source link

Functionality to specify a will upon connecting MQTT missing #13

Closed Sk4zz closed 6 years ago

Sk4zz commented 6 years ago

Nice package to get ArduinoOTA and MQTT working together. But some features are still missing. I miss the ability to specify a will for the MQTT client most at the moment.

Thank you for the great work!

jaromanda commented 6 years ago

Note that PubSubClient has implemented Last Will and Testament since prior to 2016 - hopefully this issue won't be dismissed as quickly as mine (for the same request) was yesterday

ItKindaWorks commented 6 years ago

Thank you for enjoying the library! Another commenter recently noted that they would also like last-will support.

It should be fairly easy to add into netInfo and then into the initialization code for ESPHelper. I really wish the pubSub lib had some other way to implement last will outside of the connect function... Oh well. I've put this feature on my list of additions but at the moment I don't have a whole lot of time to spend on this lib so it might take me a while to find the best way to add it in and test it.

Sk4zz commented 6 years ago

Thank you for the fast reply!

I would suggest to overload the constructor of the ESPHelper another time allowing for specifying a will. In addition I would include the option to specify the will in the netInfo object. Perhaps I find the time to include it on the weekend. The biggest hurdle for me is that I never used git before :-D

Best regards!

Sk4zz commented 6 years ago

I am new to GitHub and I have never used git before. I created a fork in my repository and a branch with added functionality for Last will of MQTT. I changed all the involved constructors and functions, overloaded some and added a setWill function. It compiles nicely but I have no ESP at hand right now to test the funcitonality. Please tell me if it works and how to make this branch "official"...

ItKindaWorks commented 6 years ago

Hi,

Thanks for jumping on the opportunity to add in the last will support. I'll try to find some time to go through it and merge it into the master version. The one thing I want to test out (and maybe you can try this) is just to make sure that the changes don't affect the compilation of any of the examples. With all of the additions that have been made over the past year I've done my best to always maintain backwards compatibility with previous versions (something not always attainable but I try...). If you could verify that all of the examples still compile without modification that would be awesome!

jaromanda commented 6 years ago

I also started adding LWT function when my identical request was closed. The way I made sure not to break existing functionality was to add LWT behind a #define. That way, current functionality is guaranteed to not break either due to code incompatibility, or due to increased sketch size.

ItKindaWorks commented 6 years ago

Sorry about closing your topic a bit prematurely - I think I was having a long day and between that and thinking I know everything there is to know about pubsubclient I just closed it without really thinking. Do you have your version up on github so I can take a look and see if I can get it integrated. I don't have a ton of time right now (oh to live on the schedule of an educator) but I can see what I can do to add it in as it seems like something people want.

Sk4zz commented 6 years ago

I can confirm that all your examples compile without errors, except from the relayControl.ino but there was a mistake in it and it didn't compile before. I fixed that mistake, now it compiles. Additional Memory usage is between 240 and 772 bytes of program memory and between 48 and 360 bytes in dynamic memory. I also tested it on my ESP8266 and the bad news is that it doesn't work :man_facepalming: It seems like the willMessageSet flag is not correctly set when verifying the config. When I use my setWill function before begin() it works though. I'll try to figure out what went wrong. I'd like to add an option to send a status message upon connect too. I'll report here on my progress, ok?

jaromanda commented 6 years ago

I haven't put it up on github, but as Sk4zz is doing such a good job, I'll pass 😉

ItKindaWorks commented 6 years ago

Ah yes, thank you for catching that mistake on relayControl.ino! I usually check to make sure that relayControlV2 works as assume they're they same in terms of compilation - just another reason to be more thorough in my testing.

Thats great that it compiles! It generally looks like it should work, I'm not sure why it is giving you a problem when you initialize through the constructor. Could you post the demo/test code that you've been working with so I can test things out too?

A status message to mqtt on connect? While that could probably be useful, I've been actively trying to cull features that would probably be better done by the user of the lib (this would be fairly trivial in a sketch to have a "hasConnected" flag that gets set when the status is "FULL_CONNECTION" and fire off a one time message). As the library has grown I've had to think about what the library should and should not be responsible for. I recently noted to another commenter that I'm actually thinking about removing or at least moving the heartbeat functionality out of the core of ESPHelper for this exact reason. For now let's just focus on the LW problem since that is part of MQTT. Maybe we can also start another topic for status messages to keep the conversation going

Sk4zz commented 6 years ago

I've got everything working now...

The problem was in the wrong usage of the netInfo constructor. My willMessage was treated as otaPass I think. This is fixed now, but I'll nee to look at the constructors again to choose the most realistic subsets of the parameters.

I updated my branch and included a new working example to illustrate the usage of last will for status messages.

You are of course totally right, the status message on connect shouldn't be in the library. That's why I included the new example, it contains one way of doing it - probably not the best way though. I would be grateful for tips on how to make it better.

Ah, and thank you jaromanda ;-)

ItKindaWorks commented 6 years ago

Great work man, thanks for putting in all of the effort! I'll take a look at what you have and hopefully it should be easy to integrate. Let me know when you get those last few loose ends tied up!

Sk4zz commented 6 years ago

I think, I have tied up all loose ends by now. The changes should be smooth to integrate. Thank you again for the nice package!

ItKindaWorks commented 6 years ago

Awesome! Can you possibly create a pull request on the branch that you want me to integrate. It makes things easier on my end to sort out when merging. Heres the github pull request page for reference https://help.github.com/articles/creating-a-pull-request/

Sk4zz commented 6 years ago

Done.

ItKindaWorks commented 6 years ago

Alright things should be all set to go for last will support! I just updated the version to 1.5.4 to reflect the new additions. Thanks guys!