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

Remove ieee802154 from IphcRepr #1033

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Dominaezzz
Copy link

First part of #1031 .

The IphcRepr type was written in terms of ieee802154 addresses.
I've replaced the ieee802154 address for structures that closer match the RFC and can be used on other mediums like BLE.

After this, I need to do the same for sixlowpan/mod.rs and move the ieee802154 bits into the wire/ieee802154.rs file.

@Dominaezzz Dominaezzz marked this pull request as draft January 15, 2025 00:41
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 49.50690% with 256 lines in your changes missing coverage. Please review.

Project coverage is 80.26%. Comparing base (74bd1e5) to head (36d2d05).

Files with missing lines Patch % Lines
src/wire/sixlowpan/iphc.rs 39.39% 140 Missing ⚠️
src/wire/sixlowpan/mod.rs 43.47% 91 Missing ⚠️
src/wire/ieee802154.rs 63.76% 25 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1033      +/-   ##
==========================================
- Coverage   80.88%   80.26%   -0.62%     
==========================================
  Files          81       81              
  Lines       28452    28734     +282     
==========================================
+ Hits        23013    23064      +51     
- Misses       5439     5670     +231     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dominaezzz Dominaezzz marked this pull request as ready for review January 15, 2025 23:23
@Dominaezzz
Copy link
Author

Was hoping to get some feedback on the structure of things before investing time into improving coverage.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 26, 2025

Could you explain why is this needed to support 6lowpan over BLE?

IIUC address compression works the same way in both 6lo over BLE and 6lo over ieee802.15.4. The info you use to compress/decompress is an EUI64 that can be derived from either the ieee802.15.4 link address or the BLE link address. So I think it should be possible to pass in the EUI64, and have higher layers calculate it differently depending on whether the link is ieee802.15.4 or BLE? Then the code can stay in the same place, the refactor is not needed.

I'm not a 6lowpan expert though. Hopefully @thvdveld can weigh in.

(The reason I'm a bit concerned with the refactor is that "look at packet fields, write Rust enum, copy it around, match it to do thing" typically increases code size vs "look at packet fields, do thing")

@Dominaezzz
Copy link
Author

The info you use to compress/decompress is an EUI64 that can be derived from either the ieee802.15.4 link address or the BLE link address.

Yes, this is largely the case for non-contextual compression.
The problem with the existing IphcRepr is, it has ieee802.15.4 link addresses hard coded into it, which cannot be used by BLE. BLE uses mac addresses which are a different length (48 bits) to ieee802.15.4 link addresses (64 bits).
This PR just makes the type more agnostic.

So I think it should be possible to pass in the EUI64, and have higher layers calculate it differently depending on whether the link is ieee802.15.4 or BLE? Then the code can stay in the same place, the refactor is not needed.

Due to the things I've mentioned above, some kind of refactor will still be needed to remove ieee802.15.4 from IphcRepr to allow it to be used for BLE.
See this.

There's more info in the linked issue.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 26, 2025

My question is: why can't we keep the current code structure, just replace LlAddress with an EUI64? Higher layers derive this EUI64 from either the ieee802154 or BLE address.

This way wire::sixlowpan is agnostic from ieee802154 and we keep the current code structure which is simpler and uses less flash. No need for the enums you added.

@Dominaezzz
Copy link
Author

I unfortunately don't know enough about ieee802154 to give you a good answer. @thvdveld thinks an EUI64 isn't enough for ieee802154.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants