Skip to content

ieee802154: migrate netdev_ieee802154_dst_filter to a common ieee802154#16576

Merged
MrKevinWeiss merged 4 commits intoRIOT-OS:masterfrom
jia200x:pr/netdev/migrate_dst_filter
Jun 29, 2021
Merged

ieee802154: migrate netdev_ieee802154_dst_filter to a common ieee802154#16576
MrKevinWeiss merged 4 commits intoRIOT-OS:masterfrom
jia200x:pr/netdev/migrate_dst_filter

Conversation

@jia200x
Copy link
Member

@jia200x jia200x commented Jun 21, 2021

Contribution description

This PR migrates netdev_ieee802154_dst_filter to ieee802154_dst_filter.
This not only makes it not dependent to netdev, but will help with Kconfig modelling since now netdev_ieee802154 can be represented as a pure netdev layer. I will add a follow up PR that shows this in action.

netdev_ieee802154_dst_filter is marked as deprecated and will be removed after 2022.01.

Testing procedure

Check that examples/gnrc_networking compiles for e.g nrf52840dk (with USEMODULE=nrf802154_netdev_legacy`) and confirm that pinging still works.

Issues/PRs references

None so far

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jun 21, 2021
jia200x
jia200x commented Jun 21, 2021
View reviewed changes
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 21, 2021
benpicco
benpicco reviewed Jun 21, 2021
View reviewed changes
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Jun 22, 2021
@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jun 22, 2021

I pushed the changes and added unit tests, therefore can no longer ack... Hi @benpicco 👋

@jia200x jia200x force-pushed the pr/netdev/migrate_dst_filter branch from 0490335 to b7c3e1a Compare June 22, 2021 15:54
@jia200x
Copy link
Member Author

jia200x commented Jun 22, 2021

squashed

@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jun 22, 2021

Seems like some -Werror=strict-aliasing issue on the esp8266 I thought I fixed that but maybe you can take another look @jia200x

@jia200x jia200x force-pushed the pr/netdev/migrate_dst_filter branch from b7c3e1a to 9425f56 Compare June 23, 2021 08:55
@jia200x
Copy link
Member Author

jia200x commented Jun 23, 2021

The test is fine. I just changed the short address from uint16_t to network_uint16_t.

@MrKevinWeiss MrKevinWeiss added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Jun 23, 2021
@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jun 23, 2021

Please squash, murdock is happy now.

benpicco
benpicco reviewed Jun 23, 2021
View reviewed changes
sys/include/net/ieee802154.h
Comment on lines +415 to +416
int ieee802154_dst_filter(const uint8_t *mhr, uint16_t pan,
network_uint16_t short_addr, const eui64_t *ext_addr);
Copy link
Contributor

@benpicco benpicco Jun 23, 2021

Choose a reason for hiding this comment

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

Just to clarify: pan is uint16_t but short_addr is network_uint16_t?

Copy link
Member Author

@jia200x jia200x Jun 23, 2021

Choose a reason for hiding this comment

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

yes :(
Unfortunately this is the way how it's presented in many ieee802154_xxx functions.

I would actually migrate everything to little endian values. We could the use all ieee802154_get_xxx operations without any kind of memcpy / reverse. But we would need to adapt many modules.

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss Jun 23, 2021

Choose a reason for hiding this comment

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

Lets keep it simple for now, the reversing issue is out of scope of this PR.

@jia200x
Copy link
Member Author

jia200x commented Jun 23, 2021

done!

@jia200x jia200x force-pushed the pr/netdev/migrate_dst_filter branch from 9425f56 to 48ef2ec Compare June 23, 2021 14:44
@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jun 24, 2021

I just saw #16576, maybe we can get 2 birds with one stone?

@MrKevinWeiss MrKevinWeiss mentioned this pull request Jun 24, 2021
Closed
@jia200x
Copy link
Member Author

jia200x commented Jun 24, 2021

I just saw #16576, maybe we can get 2 birds with one stone?

I think it's #11191

Although they touch the same file, the mechanism has changed quite a lot since the SubMAC was introduced. The packet in practice is parsed before it gets to the frame filter. So, I think we should keep them separate.

@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jun 24, 2021

copy pasta 🤦

@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jun 24, 2021

Good good.

@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Jun 28, 2021

ping @benpicco

benpicco
benpicco approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Communication using short addresses still works between samr21-xpro and nrf52840dk

@MrKevinWeiss MrKevinWeiss merged commit 25c871e into RIOT-OS:master Jun 29, 2021
@MrKevinWeiss MrKevinWeiss modified the milestone: Release 2021.07 Jul 13, 2021
@leandrolanzieri leandrolanzieri mentioned this pull request Jan 13, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.

Projects

None yet

Milestone

Release 2021.07

Development

Successfully merging this pull request may close these issues.

3 participants

@jia200x @MrKevinWeiss @benpicco