mrdunk / esp8266_mdns

mDNS queries and responses on esp8266
MIT License
77 stars 16 forks source link

Updates callbacks to std::function allowing c++ classes method binding #9

Closed fcouceiro closed 7 years ago

fcouceiro commented 7 years ago

Description: Using std::functions for the callbacks one can std::bind instance methods from c++ classes. This change preserves previous function pointers functionality.

Scenario: Class A triggers a mDNS query. Class A has a onMDNSAnswer() method that should be called after receiving a mDNS answer. Using std::bind, one can pass onMDNSAnswer() as an instance method callback to one of MDns constructores.

Example:

// Constructor
ClassA::ClassA(){
  mdnsClient = mdns::MDns(NULL, NULL, std::bind(&ClassA::onMDNSAnswer, this, std::placeholders::_1));
}

// Instance callback
void ClassA::onMDNSAnswer(const mdns::Answer* answer){
}
fcouceiro commented 7 years ago

Yes. I'll create a private begin() method to do that and can call it from the two non-delegated constructors. Several libraries expect a begin() method to be called after instantiation. Do you want to do that as well? Or call the method from the constructor directly?

mrdunk commented 7 years ago

re: begin() method vs call the method from the constructor directly, I don't have a strong opinion on this. Either way sounds reasonable. I don't have much experience with Arduino so happy to let you decide what you think fits best with Arduino style.

Thanks!

fcouceiro commented 7 years ago

Done. I think users should explicitly call begin(). See if everything is ok for you.

mrdunk commented 7 years ago

Now i think this through... we'll need to update the code examples and documentation to include begin(); as part of the setup of each program example.

The alternative would be to include the begin(); in all the constructors which would somewhat defeat the point of having a separate begin() function... We could put it behind a #ifdef AUTO_UDP_START or something like that?

The advantage of this method would be there would be no change in functionality. If we set '#define AUTO_UDP_START` by default existing programs would work as before. Users who know what they are doing could disable it at compile time.

Sorry to make a simple thing more complicated that you first intended....

fcouceiro commented 7 years ago

You're right! I guess using a define like that would work, but seems a bit overkill and can potentially mislead someone who reads the code and find a begin() function. Given that people are already familiar with UDP being auto initialized (even though in loop()) I think we should rename begin() to something like startUdpMulticast(), set it as private and call it from the constructors.

What you think?

mrdunk commented 7 years ago

That works for me.

fcouceiro commented 7 years ago

There you go.

mrdunk commented 7 years ago

i've created a v1.1.7 release with this change in it. https://github.com/mrdunk/esp8266_mdns/releases The Arduino repositories should start picking it up in the next day or two.

Thanks for the help!

fcouceiro commented 7 years ago

Great! You're welcome!