search

arduino-libraries / ArduinoMqttClient

ArduinoMqttClient Library for Arduino
GNU Lesser General Public License v2.1
192 stars 75 forks source link

Fixing compatibility for usage with ESP32 core #9

Closed aentinger closed 5 years ago

aentinger commented 5 years ago

ESP32 core class Client defines two additional pure virtual connect functions which are leading to a compilation error (class MqttClient becomes an abstract class and can not be instantiated, since it does not provide implementations for those pure virtual functions). This commit provides this implementation allowing the class MqttClient to be used with the ESP32 core

per1234 commented 5 years ago

My question is whether it makes sense for every Arduino networking library to have to be patched just because the ESP32 developers made a unilateral decision to break the standardized core API.

This issue has been raised repeatedly in the ESP32 core repository but they haven't taken action to fix it. It seems to me that if we give in to this then we are sending the message that any 3rd party developer can make arbitrary changes to the core API and everyone will just accommodate it. If breaking changes to the core API are actually needed, then that should be discussed with Arduino first.

Related:

...and so many more

ubidefeo commented 5 years ago

I'll side with @per1234 on this one. 3rd party cores developers/maintainers should comply with the basic API anywhere necessary. If there's a specific reason not to it should be brought up and discussed. I'm not sure how far this will go, though

aentinger commented 5 years ago

@per1234 and @ubidefeo - thank you very much for raising this point. I wasn't aware of those issues, in fact I have totally different reasons why I made this pull request which are better discussed internally. Let's leave the PR open for now until we can resolve this topic.

sandeepmistry commented 5 years ago

@lxrobotics has pointed out: https://github.com/espressif/arduino-esp32/pull/3191 so in a future ESP32 core release this will not be needed.

aentinger commented 5 years ago

That's just great. Since there is no further need for this pull request I will close it.