Don't compile the installIPv6RABlocker if unused

[RedVyper]

The function installIPv6RABlocker() installs a raw-ICMPv6 callback that blackholes unsolicited IPv6 router advertisements, working around the LwIP bug where an RA overwrites the IPv4 DNS servers. That workaround is only needed on ESP-IDF v4 and older: the call site in wled.cpp has been gated on ESP_IDF_VERSION_MAJOR < 5 for a while, but the function's definition (network.cpp) and declaration (fcn_declare.h) were not.

So on ESP-IDF v5 targets the function was still compiled and emitted into the binary despite having no callers. This adds the matching guards so the three sites agree.

No behavioural change on any target: on IDF v4 the blocker is compiled and installed exactly as before; on IDF v5 it was never called, and now simply isn't built.

Summary by CodeRabbit

• Bug Fixes
  • Improved IPv6 networking compatibility on ESP32 devices using ESP-IDF 5 and newer.
  • Removed an obsolete IPv6 workaround where it is no longer required.
  • Preserved the workaround for supported older ESP-IDF versions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ddd5580d-5242-4894-86b0-98d751eb79a8

📥 Commits

Reviewing files that changed from the base of the PR and between 700c0a4 and 369d2cf.

📒 Files selected for processing (2)

• wled00/fcn_declare.h
• wled00/network.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

Walkthrough

Changes

IPv6 RA blocker compatibility

Layer / File(s)	Summary
Apply platform and ESP-IDF guards wled00/fcn_declare.h, wled00/network.cpp	installIPv6RABlocker() is declared and defined only for ESP32 builds with IPv6 enabled and ESP-IDF versions below 5.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 369d2

The PR only prevents an unused IPv6 router-advertisement blocker from being built on ESP-IDF 5+ while preserving existing ESP-IDF 4 behavior, so no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: willmmiles, softhack007, netmindz

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: preventing compilation of installIPv6RABlocker() when it is unused. It is concise and related to the pull request objectives, although it does not speci…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title clearly identifies the main change: preventing compilation of installIPv6RABlocker() when it is unused. It is concise and related to the pull request objectives, although it does not specify the ESP-IDF version condition.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

Warning

⚠️ This pull request shows signs of AI-generated slop (redundant_comments). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

[RedVyper]

Isn't it ironic that the AI that checks the code says that a commit contains AI slop just because I have the same comment repeated for clarity next to the same pre-processor if clause?

[willmmiles]

I strongly disagree, and IMO this is the worst kind of slop: pointless extra code that adds only noise and clutter but makes no difference to the final output. These unnecessary #ifdefs were deliberately omitted because the linker will exclude the unreferenced functions automatically. It is not necessary or useful to #ifdef out every function everywhere that doesn't happen to be called on some specific platform(s). #ifdef should be reserved only for places where it is necessary for the build.

[RedVyper]

I strongly disagree,

Fair enough.

and IMO this is the worst kind of slop

Not that great going that direct on someone contributing.

To answer on the merit, I don't think your objection holds up for the following reasons.

How can a person know that some of those ifdefs were deliberately omitted if there is no comment and no commit message saying so? You know it because you wrote the original piece of code but sometimes you need to take into account other people's fresh point of view and let them know in advance the rationale behind a piece of code.

On the linker doing it for us: -Wl,--gc-sections isn't ours to rely on. It comes from framework-arduinoespressif32/tools/platformio-build-esp32.py, a package we pin but don't control, and nothing in our build asserts it's enabled. And it can't catch a mistake so if that call ever gets moved or un-gated, the linker will quietly put the blocker back in the image, where a guarded definition would break the build and tell someone. Writing the constraint where the compiler can check it isn't noise.

Moreover, I'd like you consider the bigger picture, which is the final goal of getting rid completely at this piece of code entirely. After all, this is just a workaround needed because the project doesn't support IPv6 yet but the underlying LwIP does it. Implementing IPv6 properly will make this piece of code not necessary anymore.

[softhack007]

I think @willmmiles is right, both gc-sections and -flto will remove the function from the binary in case it's not referenced. There is no need to shield everything with #ifdef.

[RedVyper]

@softhack007 I am sure that's the case, still I think that being sure that something happens because the pre-processor gets the right and clear hint is better than hoping for the best. I personally think that's better but since that function is supposed to be removed at some point in time, at least when I'm finished with the IPv6 implementation for ESP32, I won't insist on the matter as much as my believes push me.

[softhack007]

@RedVyper I am sure - if it would not be the case, our firmware binaries would be much larger.

[willmmiles]

Not that great going that direct on someone contributing.

You're right, and I'm sorry. I posted in anger, and that's not fair to you or any other contributors. It doesn't help you learn or improve our code. I'll make sure to cool off before posting in the future.

To answer on the merit, I don't think your objection holds up for the following reasons.

How can a person know that some of those ifdefs were deliberately omitted if there is no comment and no commit message saying so? You know it because you wrote the original piece of code but sometimes you need to take into account other people's fresh point of view and let them know in advance the rationale behind a piece of code.

Best practices in C/C++ code are to avoid using the preprocessor unless it's absolutely necessary. It's always a loaded footgun; while there are plenty of tricks that can't be achieved any other way, it should treated as a method of last resort. We already have enough such noise making it difficult to follow code - in a case like this, less is more.

Regarding documentation, as a general rule, I don't think it's necessary to comment every time a best practice is followed.

On the linker doing it for us: -Wl,--gc-sections isn't ours to rely on. It comes from framework-arduinoespressif32/tools/platformio-build-esp32.py, a package we pin but don't control, and nothing in our build asserts it's enabled. And it can't catch a mistake so if that call ever gets moved or un-gated, the linker will quietly put the blocker back in the image, where a guarded definition would break the build and tell someone. Writing the constraint where the compiler can check it isn't noise.

Respectfully, --gc-sections has been standard practice in embedded environments for more than 15 years, and is common pretty much everywhere today. As @softhack007 mentioned, basically nothing in this system would work without it. It's reasonable to expect that our build tooling should be discarding unreferenced functions, be it our own work, or functions from libraries we inherit. We certainly do not expect the entire C++ standard library to be included in the binary unless it's used! This approach is more or less foundational to the Arduino ecosystem, where including a header and addressing its global Object is what instantiates various subsystems in one's project.

Moreover, I'd like you consider the bigger picture, which is the final goal of getting rid completely at this piece of code entirely. After all, this is just a workaround needed because the project doesn't support IPv6 yet but the underlying LwIP does it. Implementing IPv6 properly will make this piece of code not necessary anymore.

Sadly, I fear we are stuck with it until we remove IDF v4 support. The IPv6 "support" in the IDF v4 platform is essentially unworkable due to bugs like this one -- the two stacks poorly share a number of global variables resulting in much confusion if either protocol is broken or misconfigured in the local network environment. This workaround is a case in point: we need it because, even when "disabled", IPv6 packets are still overwriting global variables used by the IPv4 stack. I wish I could say that was an isolated bug, but to my read of the code, that sort of issue was typical of the quality of the integration in that release.

I believe that enabling IPv6 in IDF v4 will result in more issues caused by platform bugs than successful use cases.

I've no objections to enabling IPv6 with the newer IDF platform releases, though -- hopefully we won't find any other showstopping bugs. As you're pursuing that end, I'd encourage you to review the factoring of the various network management functions to avoid that feature also turning into an #ifdef soup -- personally I'd much rather consider reorganizing the code than another #ifdefed block in every network function.

[RedVyper]

You're right, and I'm sorry. I posted in anger, and that's not fair to you or any other contributors. It doesn't help you learn or improve our code. I'll make sure to cool off before posting in the future.

Thanks for having noticed that. My goal is to give my contribute to the project and make it better. Nothing else.

Best practices in C/C++ code are to avoid using the preprocessor unless it's absolutely necessary. It's always a loaded footgun; while there are plenty of tricks that can't be achieved any other way, it should treated as a method of last resort. We already have enough such noise making it difficult to follow code - in a case like this, less is more.

I can understand the idea that "if it's not there you cannot have it break something" but I'm still convinced that keeping control over how the code is pre-processed and compiled is essential. Relying to other software that one doesn't control is kinda weird to me. I understand that you can say that you kinda have control over that by passing it the various parameters but still it feels wrong to me.

Regarding documentation, as a general rule, I don't think it's necessary to comment every time a best practice is followed.

Yet you noticed that without proper comments I didn't know what your intentions were when you wrote that part of the code. This is why I am a big fan of "keep the code as concise as possible while being allowed to be very detailed on the comments". They don't waste space in the build, after all.

Respectfully, --gc-sections has been standard practice in embedded environments for more than 15 years, and is common pretty much everywhere today. As @softhack007 mentioned, basically nothing in this system would work without it. It's reasonable to expect that our build tooling should be discarding unreferenced functions, be it our own work, or functions from libraries we inherit. We certainly do not expect the entire C++ standard library to be included in the binary unless it's used! This approach is more or less foundational to the Arduino ecosystem, where including a header and addressing its global Object is what instantiates various subsystems in one's project.

As I said before I hope that something works the way it's intended but if I can check myself that it cannot go wrong, then I prefer to do so.

Please note that some of my comments up to here are strong opinions but yet opinions. It's totally normal that you have your own. If I have to insist on something I'd go with proper comments to improve legibility to people that are reading the code for the first time and need context.

Sadly, I fear we are stuck with it until we remove IDF v4 support. The IPv6 "support" in the IDF v4 platform is essentially unworkable due to bugs like this one -- the two stacks poorly share a number of global variables resulting in much confusion if either protocol is broken or misconfigured in the local network environment. This workaround is a case in point: we need it because, even when "disabled", IPv6 packets are still overwriting global variables used by the IPv4 stack. I wish I could say that was an isolated bug, but to my read of the code, that sort of issue was typical of the quality of the integration in that release.
I believe that enabling IPv6 in IDF v4 will result in more issues caused by platform bugs than successful use cases.

My goal for now is IPv6 support only for IDF v5.

I've no objections to enabling IPv6 with the newer IDF platform releases, though -- hopefully we won't find any other showstopping bugs. As you're pursuing that end, I'd encourage you to review the factoring of the various network management functions to avoid that feature also turning into an #ifdef soup -- personally I'd much rather consider reorganizing the code than another #ifdefed block in every network function.

I'm trying to work by little steps, so that my PRs are small and easily reviewable. At a certain point there will be the chance to refactor the network section, perhaps. But for now I want to be as surgical as possible.

[willmmiles]

If I have to insist on something I'd go with proper comments to improve legibility to people that are reading the code for the first time and need context.

I don't really think that's a meaningful thing to do in this context. If I added a comment every time I omitted an unnecessary line of code, at the limit that's an infinite amount of comments. How am I to know what line you're thinking of that I didn't write?

I appreciate that you've got a different opinion on #ifdef usage. As a maintainer of this project, I'm passing along that our opinion here is to avoid #ifdefs when they do not change the output in our supported build system. It's OK that you prefer a different style, but that's how we do things here.

I've no objections to enabling IPv6 with the newer IDF platform releases, though -- hopefully we won't find any other showstopping bugs. As you're pursuing that end, I'd encourage you to review the factoring of the various network management functions to avoid that feature also turning into an #ifdef soup -- personally I'd much rather consider reorganizing the code than another #ifdefed block in every network function.

I'm trying to work by little steps, so that my PRs are small and easily reviewable. At a certain point there will be the chance to refactor the network section, perhaps. But for now I want to be as surgical as possible.

I gathered that, and that's why I brought it up -- I'm concerned the "surgical" approach will result in messy code and a larger maintenance burden (#ifdef soup). It's a big feature, and I'd much rather review a thoughtful framework than fifty little code injections around the project. (In open source it's an all-too-common failure mode for a contributor to start a can-this-be-done spike, get something working, and then PR it in that nascent form, leaving it up to the maintainers to clean it up later.) I'd like to encourage you to think bigger. :)

[softhack007]

Back on-topic: what makes this PR a bit more questionable is that the function to install the blocker is removed, however the blocker itself blockRouterAdvertisements() still gets compiled.

As the PR is basically a no-op from firmware binary perspective, and compilation guards are already in place to prevent compilation when the core does not define LWIP_IPV6 - I'd say let's close this PR (not merged) and move on.

[RedVyper]

I appreciate that you've got a different opinion on #ifdef usage. As a maintainer of this project, I'm passing along that our opinion here is to avoid #ifdefs when they do not change the output in our supported build system. It's OK that you prefer a different style, but that's how we do things here.

While I keep disagreeing with your opinion, I value it and your role so I will adapt to what you think it's best for the project.

I gathered that, and that's why I brought it up -- I'm concerned the "surgical" approach will result in messy code and a larger maintenance burden (#ifdef soup). It's a big feature, and I'd much rather review a thoughtful framework than fifty little code injections around the project. (In open source it's an all-too-common failure mode for a contributor to start a can-this-be-done spike, get something working, and then PR it in that nascent form, leaving it up to the maintainers to clean it up later.) I'd like to encourage you to think bigger. :)

There's a reason why the other PR is a draft and I won't remove the draft state until I get the web UI on par with IPv4. But I need your opinion on the state of the PR as it is now because it's easier for me to adapt the base of that PR if you give me your opinion on the small bits that I submitted until now. I already have two new features ready that I'd like to adapt to your vision.

As the PR is basically a no-op from firmware binary perspective, and compilation guards are already in place to prevent compilation when the core does not define LWIP_IPV6 - I'd say let's close this PR (not merged) and move on.

I basically have the same comment that I gave @willmmiles about this. And I also have the same answer. I accept your vision on this and we can move no problem. After all, this function will become obsolete when the other PR will be in a mergeable state. But to get there I need his opinion on the code that's there right now.