Skip to content
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

Fixing compatibility for usage with ESP32 core #9

Closed
wants to merge 1 commit into from

Conversation

aentinger
Copy link
Contributor

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

…t' 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
Copy link
Contributor

per1234 commented Aug 13, 2019

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
Copy link

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@aentinger
Copy link
Contributor Author

aentinger commented Sep 10, 2019

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

@aentinger aentinger closed this Sep 10, 2019
@aentinger aentinger deleted the compatibility-esp32-core branch September 10, 2019 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants