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

Compile error if CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM set to 1 #29282

Closed
wy-hh opened this issue Sep 15, 2023 · 4 comments · Fixed by #29325
Closed

Compile error if CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM set to 1 #29282

wy-hh opened this issue Sep 15, 2023 · 4 comments · Fixed by #29325

Comments

@wy-hh
Copy link
Contributor

wy-hh commented Sep 15, 2023

After PR #29232 merged, I get compile error , if I define CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM to 1 for our platform.

Here is my found, if there is something wrong, please let me know.

Defining CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM to 1 makes CHIP_SYSTEM_PACKETBUFFER_FROM_LWIP_POOL to 0.
Then CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX, not used for LwIP-based platform, is required in class PacketBuffer as below.

#if CHIP_SYSTEM_PACKETBUFFER_FROM_LWIP_POOL
    static constexpr uint16_t kMaxSizeWithoutReserve = LWIP_MEM_ALIGN_SIZE(PBUF_POOL_BUFSIZE);
#else
    static constexpr uint16_t kMaxSizeWithoutReserve = CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX;
#endif
@wy-hh wy-hh changed the title #29232 makes compile error if CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM set to 1 Compile error if CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM set to 1 Sep 15, 2023
@bzbarsky-apple
Copy link
Contributor

Is kMaxSizeWithoutReserve not used with lwip? It sure seems to be used....

What is the right setting of kMaxSizeWithoutReserve when doing CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM?

@bzbarsky-apple
Copy link
Contributor

@kpschoedel can you please take a look?

@kpschoedel
Copy link
Contributor

Did you previously use #define CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_TYPE PBUF_RAM?
I think then, because of the error that #29208 called out, CHIP_SYSTEM_PACKETBUFFER_FROM_LWIP_POOL was (incorrectly) set to 1, and this was never caught because there's no in-tree build using this configuration.

We would preserve the previous behaviour here by changing that line

#if CHIP_SYSTEM_PACKETBUFFER_FROM_LWIP_POOL

to

#if CHIP_SYSTEM_CONFIG_USE_LWIP

and I don't immediately see any other unexpected consequences of #29208.

@wy-hh
Copy link
Contributor Author

wy-hh commented Sep 16, 2023

@kpschoedel Matter currently doesn't fully support PBUF_RAM with original LWIP code, so there is no platform enabled in public Matter repo and no auto build detects it.
We use #define CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_TYPE PBUF_RAM to only pass pbuf_type paramter to pbuf_alloc in PacketBufferHandle::New under our repo for some PBUF_RAM experimental developments.

I think changing to #if CHIP_SYSTEM_CONFIG_USE_LWIP works for our platform.

kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Sep 18, 2023
Missed a case of CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM, as
there are no in-tree builds using this configuration. (This used
CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_TYPE before project-chip#29208.)

Fixes project-chip#29282
@mergify mergify bot closed this as completed in #29325 Sep 18, 2023
mergify bot pushed a commit that referenced this issue Sep 18, 2023
Missed a case of CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM, as
there are no in-tree builds using this configuration. (This used
CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_TYPE before #29208.)

Fixes #29282
HunsupJung pushed a commit to HunsupJung/connectedhomeip that referenced this issue Oct 23, 2023
Missed a case of CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_RAM, as
there are no in-tree builds using this configuration. (This used
CHIP_SYSTEM_CONFIG_PACKETBUFFER_LWIP_PBUF_TYPE before project-chip#29208.)

Fixes project-chip#29282
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 a pull request may close this issue.

3 participants