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

[BUG] LWIP_PBUF_FROM_CUSTOM_POOLS and PBUF_POOL_SIZE don't seem to be set up in a reasonable way #29211

Closed
bzbarsky-apple opened this issue Sep 13, 2023 · 3 comments · Fixed by #29408
Labels
bug Something isn't working needs triage

Comments

@bzbarsky-apple
Copy link
Contributor

bzbarsky-apple commented Sep 13, 2023

Reproduction steps

LWIP_PBUF_FROM_CUSTOM_POOLS and PBUF_POOL_SIZE are used by some of our config headers (as in, its value is used in ReliableMessageProtocolConfig.h), but where are they supposed to be defined by consumers? Some things seem to have them in lwipopts.h, but our config headers don't include lwipopts.h, right?

How is this supposed to work?

Bug prevalence

Always

GitHub hash of the SDK that was being used

b02c984

Platform

other

Platform Version(s)

No response

Anything else?

No response

@bzbarsky-apple bzbarsky-apple added bug Something isn't working needs triage labels Sep 13, 2023
@bzbarsky-apple bzbarsky-apple changed the title [BUG] LWIP_PBUF_FROM_CUSTOM_POOLS doesn't seem to be set up in a reasonable way [BUG] LWIP_PBUF_FROM_CUSTOM_POOLS and PBUF_POOL_SIZE don't seem to be set up in a reasonable way Sep 13, 2023
@rojer
Copy link
Contributor

rojer commented Sep 21, 2023

PBUF_POOL_SIZE is one of LwIP's public options hence it's ok to rely on it being defined. LwIP user code can explicitly #include <lwipopts.h>, so that seems like a valid solution (when CHIP_SYSTEM_CONFIG_USE_LWIP is set, of course).

@bzbarsky-apple
Copy link
Contributor Author

Interestingly, we have other headers using LWIP_PBUF_FROM_CUSTOM_POOLS without obviously including lwipopts.h. But maybe those are just broken?

For now I am going to condition the use of the LwIP things on CHIP_SYSTEM_CONFIG_USE_LWIP, and then we'll see what happens when we actually try to turn on -Wundef on the LwIP-using platforms.

@rojer
Copy link
Contributor

rojer commented Sep 22, 2023

lwipopts could be transitively picked up via any other lwip include somewhere in the tree. #include "lwip/opt.h" is the first thing any lwip include does, opt.h in turn includes lwipopts.h to pick up user settings.
so actually, small correction: the right thing to do is to #include "lwip/opt.h", to make sure defaults are correctly applied.

@mergify mergify bot closed this as completed in #29408 Sep 22, 2023
mergify bot pushed a commit that referenced this issue Sep 22, 2023
…9408)

* Don't use LwIP defines if CHIP_SYSTEM_CONFIG_USE_LWIP is not set.

Fixes #29211

* Address review comment.
HunsupJung pushed a commit to HunsupJung/connectedhomeip that referenced this issue Oct 23, 2023
…oject-chip#29408)

* Don't use LwIP defines if CHIP_SYSTEM_CONFIG_USE_LWIP is not set.

Fixes project-chip#29211

* Address review comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants