embeddedartistry / arduino-printf

Add printf support to the Arduino SDK
MIT License
91 stars 15 forks source link

PlatformIO version of LibPrintf appears broken. #37

Closed NormanDunbar closed 1 year ago

NormanDunbar commented 2 years ago

Hello. I've used your installable library in the Arduino IDE and I love it. Thanks for this.

Unfortunately, I'm unable to use it "as is" in PlatformIO as the extras directory is missing. The library in question is at https://registry.platformio.org/libraries/embeddedartistry/LibPrintf.

I see it was last updated 4 months ago, so maybe that directory was somehow dropped from the installed source code? I've copied mine over from the Arduino version, so no major problems, but I thought I better let you know.

Cheers, Norm.

phillipjohnston commented 2 years ago

PlatformIO is the problem, there is a workaround here: https://github.com/embeddedartistry/arduino-printf/issues/17#issuecomment-733244442

phillipjohnston commented 2 years ago

Reopening because I might be able to refactor the way this is handled to just work around PlatformIO removing the extras folder.

phillipjohnston commented 2 years ago

(No promise on timeline)

NormanDunbar commented 2 years ago

PlatformIO is the problem, there is a workaround here: #17 (comment)

Ok, I see the problem from the other issue. Sorry I didn't find that when I was looking. My bad.

I'll use the workaround given there.

Thank you.

Cheers, Norm.

phillipjohnston commented 2 years ago

No need to be sorry!

ivankravets commented 2 years ago

Ivan from PlatformIO is here.

There is no problem with PlatformIO. We follow Arduino library.properties specification when exporting its libraries or building them. According to the docs, the extra directory is used for nonimportant things, such as media files, docs, etc. See

The content of the extras folder is totally ignored by the Arduino development software; you are free to put anything inside.

We don't pack an extra folder by default. In most cases, this folder consumes a few dozens megabytes (pictures, videos, etc), and these files are not important for project building.


This section explains the valid structure of the source code. Nevertheless, PlatformIO is very flexible. We can override all settings using library.json -> export field and control what to export.

@phillipjohnston, what is the best solution to solve this issue? Would you like to merge my PR with the library.json manifest or does it make sense to move source code from extra/assets folder to the src/utility?

P.S: Thanks for all your OSS contributions and amazing articles!

phillipjohnston commented 2 years ago

I would gladly accept a PR with the library.json manifest rather than continue to debate whose problem it really is :).

or does it make sense to move source code from extra/assets folder to the src/utility?

For a more technical explanation on why moving the files into src/utility would not work at this moment.

One underlying goal was structuring this library so that it could consume the upstream printf library wholesale, without modifications. This makes it easier to apply updates. So I have tried not to modify the upstream files, but clearly there is a need.

Second, the library's .cpp file includes the printf.c file directly in order to supply default compile-time configuration options (e.g., to make it suitable for AVR targets). Including the printf.c source file in the src tree causes the Arduino IDE to automatically compile the files a second time, generating multiple definition errors at link time. Moving to the extras folder gave me a pattern that worked to achieve the goals above (at least with the Arduino IDE, CMake, and Meson builds) and passes the Arduino library linter that is run to determine whether your library can be included in the library manager. (All my headaches are caused by the "helpful" features like auto-compiling source files or auto-deleting "unnecessary" folders to save space or configuration options 😅).

After this recent filing, I looked into refactoring the Arduino library code to use the recently provided configuration header support in printf.c to supply the default options, but its inclusion is dependent on another preprocessor definition instead of something like __has_include, so I cannot get around it without updating the upstream library. Ideally, I will work with the maintainer to get these changes rolled in (ideal scenario), otherwise I will have to deviate further on my own fork or in this library. One of those will happen, but not in the next few weeks, so again a stopgap would be welcome.

ivankravets commented 2 years ago

Thanks, @phillipjohnston . See https://github.com/embeddedartistry/arduino-printf/pull/38

Please also increment the version number in library.properties. I see you already tagged a new v1.2.11 but Arduino manifest still points to the v1.2.10. Next time you will need to update "version" field in both manifests: library.properties + library.json.

If that PR looks good for you, please merge it, then increment to v1.2.12 both manifest and make a new tag here https://github.com/embeddedartistry/arduino-printf/releases

PlatformIO will automatically pull the latest release and will do this in the future fetching library.json and looking for the release with the same tag as "version". The tag could be the same as the "version" value (this is how you use it now) or as Github recommends with the v prefix.

phillipjohnston commented 2 years ago

Noted, thanks!

NormanDunbar commented 2 years ago

Brilliant stuff. Thank you both.

Cheers, Norm.

ivankravets commented 2 years ago

@NormanDunbar, could add this line to your platformio.ini file?

[env:yourenv]
platform = ...
lib_deps = 
    embeddedartistry/LibPrintf@^1.2.13

Does it work now? If yes, please close this issue.

@phillipjohnston, thanks for merging and releasing a new version! 🙏

phillipjohnston commented 2 years ago

The credit is all yours, @ivankravets.