Skip to content

net: l2: ieee802154: pkt: fix cpp build failure#62314

Merged
carlescufi merged 1 commit intomainfrom
unknown repository
Sep 6, 2023
Merged

net: l2: ieee802154: pkt: fix cpp build failure#62314
carlescufi merged 1 commit intomainfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 5, 2023

ieee802154_pkt.h: Add cast to support CPP.

Fixes: #62282

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 5, 2023

The test failures seem to be unrelated to this change. I can reproduce them locally on main. Not sure what's going on. Mainline broken? See #62317

@jukkar
Copy link
Copy Markdown
Member

jukkar commented Sep 6, 2023

Fixed now in mainline, please re-submit.

CPP needs an additional cast.

Fixes: #62282

Signed-off-by: Florian Grandel <fgrandel@code-for-humans.de>
@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 6, 2023

@cfriedt ACK? :-)

Copy link
Copy Markdown
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Although AFAICT, 802.15.4 is the only consumer of this field. Even if the intent were to allow other L2s to override it, it isn't possible with the current struct. So maybe some future cleanup in the net code, but this works for now

@carlescufi carlescufi merged commit 718fe84 into zephyrproject-rtos:main Sep 6, 2023
@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 6, 2023

Although AFAICT, 802.15.4 is the only consumer of this field. Even if the intent were to allow other L2s to override it, it isn't possible with the current struct.

@cfriedt @rlubos Yes agreed - it was a conscious choice.

There is a comment at that field that gives the rationale: Robert and I tried to find a compromise that is not prematurely abstract but still architecturally way better than the previous solution. We now only depend on a single IEEE 802.15.4 specific field (and one conditional include) rather than globally exposing tens of IEEE 802.15.4 specific fields. Plus we can use memcpy to clone the whole well-encapsulated block rather than having to clone field by field which broke encapsulation even more and had to be updated with every new field (which is easy to forget).

It is trivial to change this field in the future to a union of conditionally included control block versions if other L2s want to have their own. Linux has a pre-allocated untyped array of 40 bytes as a control block but that's not acceptable in the embedded world IMO. We could have introduced a union right away but that's not nice with a single field. I discussed several versions of this with Robert and we agreed that that was a good middle ground for now.

@ghost ghost deleted the fix/62282-ieee802154-pkt-cpp branch September 7, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ieee802154: Build failes when including the file net/ieee802154_radio.h in a cplusplus file.

6 participants