Skip to content

Fixes to facilitate building directly on ESP-IDF#147

Open
kbx81 wants to merge 1 commit intomikalhart:masterfrom
kbx81:20250720-build-on-idf
Open

Fixes to facilitate building directly on ESP-IDF#147
kbx81 wants to merge 1 commit intomikalhart:masterfrom
kbx81:20250720-build-on-idf

Conversation

@kbx81
Copy link
Copy Markdown

@kbx81 kbx81 commented Jul 20, 2025

This PR implements patches to permit building ESPHome configurations with this library when using ESP-IDF (instead of Arduino) as the framework. This results in smaller binaries, which is particularly important due to the notable size increase associated with Arduino 3.x. Here's the results from our build tests for the original ESP32:

  • Arduino:

    RAM:   [=         ]   6.4% (used 20900 bytes from 327680 bytes)
    Flash: [==        ]  19.7% (used 361164 bytes from 1835008 bytes)
    
  • IDF:

    RAM:   [          ]   3.6% (used 11780 bytes from 327680 bytes)
    Flash: [=         ]  14.7% (used 269864 bytes from 1835008 bytes)
    

Some of these changes were in #69 but it appears that they were not merged correctly and/or additional changes were made after merging them, at least based on what I see in c093432

I've created additional (ESPHome) tests to verify these changes work when built with ESPHome configurations using both Arduino and IDF; see esphome/esphome#9728

@svdrummer
Copy link
Copy Markdown

As this library isnt really supported much anymore, would it be better having your changes as a different tinyGPS library? If it aint broke, lets not fix it./break it. TINY GPS has been working fine for a ling time, and in reality, very few would use the changes you may want, unless you are happy fixing any errors. I have seen this happen with other libraries. Make a new version. TinyGPSPP-new or whatever

@kbx81
Copy link
Copy Markdown
Author

kbx81 commented Jul 21, 2025

Sorry @svdrummer, I work in open source full-time and would much rather contribute back to the source than "take the money and run." There are already many, many forks of this project all attempting to fix the same thing and I couldn't find one that worked. ESPHome was already using this fork and we need a tiny bit more from it; so, it was "broke", I did fix it and I'd much rather contribute my work back to the source than maintain my own version.

As it's now 2025, GPS is not exactly new and isn't changing much; I couldn't find a GPS library that's been updated more recently than 2-3 years ago. Since the rest of the world is clearly advancing and wanting new things from old projects, what exactly is wrong with updating them a little bit to fit in?

@svdrummer
Copy link
Copy Markdown

svdrummer commented Jul 21, 2025

AdaFruit has many recent updates. For ESPHome, other than time, what would you want a GPS functions for.? other than time? If you just want time, I can write something for you in C++. I have made changes to a few GPS library's in the past 18 months, as there have been changes to the NMEA0183 format, and NEO is slowly changing to its own HEX format. Then you have deliberate error injections to contend with, other hardware/software decoding.

@kbx81
Copy link
Copy Markdown
Author

kbx81 commented Jul 21, 2025

ESPHome currently supports reporting latitude, longitude, speed, course, altitude, number of visible satellites and HDOP. Yes, the most common use is for time but we also have a number of users who have mobile applications and do indeed use/consume more than just time. If I just wanted to grab time from input sentences, yes, I also know how to implement this myself, but I see no reason to reinvent the wheel yet again.

The changes in this PR:

  • Do not alter the core functionality of this library in any way, so they do not break or otherwise modify what this library does.
  • Are only invoked when a non-Arduino platform is being built.
  • Permit PlatformIO to use the library when a non-Arduino platform is in use.
  • Are consistent with changes the owner has already expressed are to be included in the current beta (and, presumably, future release) based on the README and related beta commit here; when I tested this, it did not work for a non-arduino platform/build, so I took the time to fix it and hence we have this PR.

I must also add that I see no reason why this library, given what it does, should be specifically tied to Arduino -- I have to argue that breaking that specific dependency is indeed a good thing.

@svdrummer
Copy link
Copy Markdown

Fair enough. Valid points. Well thought out. You seem to know your stuff. A lot of people just want changes because they don't have the skill to add a bit of code, which ultimately slows down and makes the library bloated. Arduino is way behind as a wrapper for Espressif's compiler.

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