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

Minor release EspSoftwareSerial 8.2.0 requires ghostl as stand-alone lib dependency #8915

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented Apr 23, 2023

@mcspr I'm sorry, my fault.

@dok-net dok-net changed the title Bug fix EspSoftwareSerial 8.0.3:: Arduino library repo didn't pick up 8.0.2 proper Bug fix EspSoftwareSerial 8.0.3: Arduino library repo didn't pick up 8.0.2 proper Apr 23, 2023
@mcspr
Copy link
Collaborator

mcspr commented Apr 23, 2023

Just use braces to init?

#include <atomic>
struct Foo {
  std::atomic<bool> bar { false };
};

gcc8 stdlib should be ok with that
(~/[email protected]+2021r2-patch5/bin/xtensa-esp32-elf-g++ -c atomic.cpp works with the above)

@dok-net
Copy link
Contributor Author

dok-net commented Apr 23, 2023

@mcspr Too short, I don't have a clue what you are talking about. Anyway, since 8.0.3 has passed the Arduino library release checks and all now, is this a blocking issue you are referring me to? Again, please let me know what you are talking about, I'll gladly put in the sources, ready for the next release :-)

mcspr referenced this pull request in plerup/espsoftwareserial Apr 23, 2023
@dok-net dok-net marked this pull request as draft June 18, 2023 07:22
@dok-net dok-net changed the title Bug fix EspSoftwareSerial 8.0.3: Arduino library repo didn't pick up 8.0.2 proper Minor release EspSoftwareSerial 8.1.0 Jul 2, 2023
@dok-net
Copy link
Contributor Author

dok-net commented Jul 2, 2023

@mcspr This release 8.1.0 is ready for ESP32 IDF 5.1 and fixes a few other compilation problems as well. Please merge at your discretion.

@dok-net dok-net marked this pull request as ready for review July 2, 2023 09:31
@dok-net dok-net marked this pull request as draft July 23, 2023 15:41
@dok-net dok-net force-pushed the swserial branch 2 times, most recently from f2d4047 to bc25eb9 Compare January 4, 2024 22:28
@dok-net dok-net changed the title Minor release EspSoftwareSerial 8.1.0 Minor release EspSoftwareSerial 8.2.0 requires ghostl as stand-alone lib dependency Jan 5, 2024
@dok-net
Copy link
Contributor Author

dok-net commented Jan 5, 2024

Some careful consideration by the core team is required here: the new ghostl library apparently does get pulled in just fine by the Arduino library dependency framework, but this is breaking with the traditional model of the ESP8266 core that has all required libs as copies or git submodules.

@dok-net dok-net marked this pull request as ready for review January 5, 2024 09:38
@mcspr
Copy link
Collaborator

mcspr commented Jan 5, 2024

Arduino library dependency framework would pick things up during interactive session, true. Not the case here, library must be present in search path.

Note that if you'd do that, now you have 2 libs to sync to here :> And Core suddenly has an external dependency for no reason. Bundling is not an option?

@dok-net
Copy link
Contributor Author

dok-net commented Jan 6, 2024

"Bundling" like previously, or are you suggesting something I haven't thought of?

Anyway:

libraries/esp8266/examples/SerialStress/SerialStress.ino
libraries/ESP8266WiFi/examples/WiFiTelnetToSerial/WiFiTelnetToSerial.ino
libraries/lwIP_PPP/examples/PPPServer/PPPServer.ino

are the only place where the EspSoftwareSerial lib is used directly.
My focus has moved from the ESP8266 to the ESP32 family (some of them have just as few HW UARTs as the ESP8266), like most users seem to have as well. The inclusion as a submodule in ESP8266 core has been a source of pain for everyone using both ESP8266 and ESP32s and me far too long already.
I'm adding the ghostl library as a new submodule in this PR now, but I am not convinced. Perhaps do this and merge once now, then sometime soon remove BOTH submodules and either document or change the few places that use EspSoftwareSerial in the ESP8266 tree, for everyone's benefit.
I don't have much of a preference, except that I will not be able to spend much effort - plus everyone complaining about issues with multiple versions of the libs between ESP8266's included and any Arduino / platformio downloaded ones in ./libraries.

@dok-net
Copy link
Contributor Author

dok-net commented Jan 6, 2024

I've checked the aforementioned sketches. The all appear to use EspSoftwareSerial in order to have two UARTs available, one for logging. IIRC the second UART of ESP8266 is TX only, but perfectly suited to that task. Therefore, somebody could rewrite the sketches just fine and free the EspSoftwareSerial hard dependency. Everyone needing a real duplex SW UART can use the Arduino library manager way.

@mcspr
Copy link
Collaborator

mcspr commented Jan 18, 2024

I've checked the aforementioned sketches. The all appear to use EspSoftwareSerial in order to have two UARTs available, one for logging. IIRC the second UART of ESP8266 is TX only, but perfectly suited to that task. Therefore, somebody could rewrite the sketches just fine and free the EspSoftwareSerial hard dependency. Everyone needing a real duplex SW UART can use the Arduino library manager way.

Agreed, examples should prefer real uart here.

"Bundling" like previously, or are you suggesting something I haven't thought of?

Just put it inside of the lib folder as previously, but with an added namespace. Meaning

#include <circular_queue.h>

Becomes this

#include <ghostl/circular_queue.h>

(but, I think here it must also drop any mention of ghostl from library .json and .properties)

I don't have much of a preference, except that I will not be able to spend much effort - plus everyone complaining about issues with multiple versions of the libs between ESP8266's included and any Arduino / platformio downloaded ones in ./libraries.

True, but so is the case for WiFi libs or anything coming from the overly-simplified library manager. IDE / CLI that causes a lot of headache when using multiple development platforms. Specifically the fact that $SKETCHLIB/libraries is overriding Core lib(s) for some reason. PIO actually has it solved with project-private libs folders, but it is more of an Arduino-adjacent solution and an altogether different tool.

I'm still on the 'keep it here' side, so repo has a version of swserial that definitely works. Whether it is up-to-date is whole other question.
(I can always just cherry-pick the version to sync here if you don't want to :)

@dok-net
Copy link
Contributor Author

dok-net commented Jan 20, 2024

I don't understand, going backward to

#include <ghostl/circular_queue.h>

and including ghostl as a copy in EspSoftwareSerial is not an option now - just to state it categorically once more.
Especially that now somebody was kind enough to submit ghostl to the platformio library repository.

Is there anything I am missing with this PR, putting ghostl side-by-side in the submodules list? This works and finally solves one on the update headaches. If you're puzzled about the project ownership - I am the original author of ghostl, AND I am the acting maintainer of EspSoftwareSerial, too - so there is conflict of interest or potential hazard that one or the other would be handled differently with regards to ESP8266 Arduino core's needs.

Are considering merging this PR at all?

@mcspr
Copy link
Collaborator

mcspr commented Jan 20, 2024

Fixing CI might be a good start? Can't merge if 'Squash and merge' button does not work.

skip=$(skip_sketch "$sketch" "$sketchname" "$sketchdir" "$sketchdirname")

skip_sketch, followed by skip_ino must skip impossible-to-build sketches, which includes ghostl coroutine examples. Either by explicitly mentioning them, or creating SKETCH_DIR/.test.skip

To reiterate - the idea is to have '#include' path contain all of the required files in one step, just for the Core distribution. And all while inside of the single lib directory, possibly using a separate branch that have these already merged. If you don't think that is viable, ok, we'll ship both.
(btw aren't the even more problems b/c of something else depending on ghostl causing out-of-sync versions yet again?)

Namespacing of library includes would be helpful regardless? Seems like a commonplace practice for more-or-less complex libs. #include <ghostl.hpp> for everything, #include <ghostl/foo.h> for specific stuff. Otherwise this is yet another problem where header names can clash with each other.

@dok-net
Copy link
Contributor Author

dok-net commented Jan 20, 2024

Ah, CI, right.
Would you consider adding this to the default?

I'm currently using a platform.local.txt:

build.stdcpp_level=-std=gnu++20
compiler.cpp.extra_flags=-fcoroutines -std=gnu++20

@mcspr
Copy link
Collaborator

mcspr commented Jan 20, 2024

See #8916 and #8990. Main concerns are with new -std=... build defaults applied to every lib and sketch besides our Core files, (sometimes useless) new warnings, and also incomplete support and its unknown quality in gcc10 branch compared to gcc11 and gcc12. Maybe next version, not really feeling adventurous just yet

@dok-net
Copy link
Contributor Author

dok-net commented Jan 21, 2024

About translating c++ namespace into directory stucture ... good point... I'm working that out. Bear with me.

@mcspr
Copy link
Collaborator

mcspr commented Jan 21, 2024

OK to merge this?

mcspr added a commit to mcspr/esp8266-Arduino that referenced this pull request Feb 4, 2024
mcspr added a commit to mcspr/esp8266-Arduino that referenced this pull request Feb 4, 2024
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.

2 participants