bertmelis / espMqttClient

MQTT 3.1.1 client library for the Espressif devices ESP8266 and ESP32 on the Arduino framework.
https://www.emelis.net/espMqttClient/
MIT License
100 stars 21 forks source link

Update library.json: Point to ESPHome forks #142

Closed mathieucarbou closed 8 months ago

mathieucarbou commented 8 months ago

Switch to AsyncTCP fork from ESPHome which will support ipv6 and also allows to configure the async+http task stack size with CONFIG_ASYNC_TCP_STACK_SIZE

bertmelis commented 8 months ago

If we definitively switch, CI has to be updates as well.

mathieucarbou commented 8 months ago

I saw that yesterday yes. Missed this part. I will update the PR ;-)

mathieucarbou commented 8 months ago

I"ve updated the PR but one of the workflow (arduino ide build) is not passing and I really don't know why. The git url is OK, I've tried with different library names, etc.

You can look at the attempts in my fork here: https://github.com/mathieucarbou/bertmelis-espMqttClient/actions

bertmelis commented 8 months ago

The alternative libraries don't have a library.properties file. this might cause the installation to fail.

I haven't looked into the possibilities to fix this: https://github.com/arduino/compile-sketches?tab=readme-ov-file#libraries

If they can provide the library.properties file, it would be the easiest solution.

mathieucarbou commented 8 months ago

The alternative libraries don't have a library.properties file. this might cause the installation to fail.

I haven't looked into the possibilities to fix this: https://github.com/arduino/compile-sketches?tab=readme-ov-file#libraries

If they can provide the library.properties file, it would be the easiest solution.

Oh boy you're right! I did not notice the removal in the fork chain... It is there in the original repo from me-no-dev.

bertmelis commented 8 months ago

It might work if you just remove the name-key. But I suspect some trial-and-error here...

mathieucarbou commented 8 months ago

It might work if you just remove the name-key. But I suspect some trial-and-error here...

yeah, that's why there is so many attempts in this PR lol ;-) I did not succeed :-(

bertmelis commented 8 months ago

Just try and remove the name and we will see. I will squash merge anyway so the number of commits is not important at all.

mathieucarbou commented 8 months ago

That's what I just did (https://github.com/bertmelis/espMqttClient/compare/0acdc3a1df9b31d6d482be70e2960ac70b671f8d..f7827f8b596c1a64ad24ac9bd7eb6c272067d7c2) but it fails also: https://github.com/mathieucarbou/bertmelis-espMqttClient/actions/runs/7530490314

I never noticed (I am not using Arduino IDE but just platformio), but it means that any projects not having this library.properties file cannot be used in Arduino IDE ? There is no alternative ?

bertmelis commented 8 months ago

Unless the library will be made "Arduino IDE compatible", this isn't going to work.

As a workaround, you can download the alternative library and add it through your platformio.ini

mathieucarbou commented 8 months ago

Unless the library will be made "Arduino IDE compatible", this isn't going to work.

As a workaround, you can download the alternative library and add it through your platformio.ini

Better than that, what I did so far is forking the ESPHome repo (https://github.com/mathieucarbou/AsyncTCP-esphome), then I've issues a 2.1.1 release in my fork (based on their 2.1.1 tag) including the library.properties file.

So this way, I can cleanly unblock me.

I could even add my fork to the Arduino lib manager to have AsyncTCP-esphome officially deployed in the library manager.

mathieucarbou commented 8 months ago

@bertmelis : would you otherwise accept to remove the dependency in the library.json file in order to let the user choose its implementation ? This can easily be documented.

espMqtttClient works with several AsyncTCP impls:

There is no dependency declared in the library.properties. So I don't see why people using Arduino IDE would have this flexibility that platformio people don't have.

bertmelis commented 8 months ago

That's a possibility I'm willing to look into.

Another option is to keep the Arduino CI action use the me-no-dev version. It's just a test to see if compilation succeeds. I assume the API doesn't change.

It's only the Arduino IDE build that fails.

mathieucarbou commented 8 months ago

That's a possibility I'm willing to look into.

Another option is to keep the Arduino CI action use the me-no-dev version. It's just a test to see if compilation succeeds. I assume the API doesn't change.

It's only the Arduino IDE build that fails.

There are 2 issues:

1) the library.json forcing the dependency to AsyncTCP

=> for this one, library.json would need to be updated to clear the dependencies to make it on par with library.properties

2) the missing library.properties in ESPHome fork

=> for this one, yes we could keep the original AsyncTCP in Arduino CI

API are compatible yes. ESPHome forks allow better control over the stack size and adds IPv6 support.

bertmelis commented 8 months ago

Wouldn't this work: point the dependency in library.json to the esphome fork and leave dependencies in library.properties blank. CI can keep the AsyncTCP as is.

I'm not a fan of just leaving out dependencies in library.json. Platformio does a good job with dependency management. I'd like to keep making use of this.

mathieucarbou commented 8 months ago

Wouldn't this work: point the dependency in library.json to the esphome fork and leave dependencies in library.properties blank. CI can keep the AsyncTCP as is.

I'm not a fan of just leaving out dependencies in library.json. Platformio does a good job with dependency management. I'd like to keep making use of this.

Yes it would of course!

I will update the PR :-)

mathieucarbou commented 8 months ago

updated - build passed.

bertmelis commented 8 months ago

Sorry, with PR #146 , I screwed up this one. Now we have a merge conflict about the submodules. could you rebase?

mathieucarbou commented 8 months ago

ahah ;-) yes!

mathieucarbou commented 8 months ago

done!