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.

Sign up for GitHub

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

Jump to bottom

Netif fix byteorder #1948

Merged
merged 1 commit into from
Nov 6, 2014
Merged

Netif fix byteorder #1948

merged 1 commit into from
Nov 6, 2014

Conversation

OlegHahm
Copy link
Member

@OlegHahm OlegHahm commented Nov 5, 2014

This PR proposes two possible solutions. Please choose one.

@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Nov 5, 2014
@OlegHahm OlegHahm added this to the Release NEXT MAJOR milestone Nov 5, 2014
@Kijewski
Copy link
Contributor

Kijewski commented Nov 5, 2014

I choose solution number three: use the types in byteorder.h.

@OlegHahm
Copy link
Member Author

OlegHahm commented Nov 5, 2014

Maybe I'm too tired, but I don't understand how I can swap the byte order by using a union.

thomaseichinger
thomaseichinger reviewed Nov 6, 2014
View reviewed changes
sys/net/link_layer/net_if/net_if.c
@@ -378,7 +379,8 @@ int net_if_send_packet_long(int if_id, net_if_eui64_t *target,
p.frame.fcf.frame_type = 1;
p.frame.fcf.frame_pend = 0;
p.frame.dest_pan_id = net_if_get_pan_id(if_id);
memcpy(p.frame.dest_addr, target, 8);
uint64_t target_h = NTOHLL(target->uint64);
Copy link
Member

Choose a reason for hiding this comment

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

for consistency you could use uint64_t target_h = byteorder_ntohll((network_uint64_t)target->uint64); here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will do this.

@thomaseichinger
Copy link
Member

Don't know if my comment above matches @Kijewski's proposal but might be worth considering. Anyway I'd take the second solution due to compactness.

@miri64
Copy link
Member

miri64 commented Nov 6, 2014

Is the EUI-64 really in host byte-order on all boards? Because I remember that this was not the case on all boards (including the old iot-lab_M3 from thirdparty_boards) I tested back then.

@Kijewski
Copy link
Contributor

Kijewski commented Nov 6, 2014

I think it would be best if ieee802154_frame_t and its friends would use network_uint64_t (network_uint16_t) for the addresses and other members were appropriate. This was the intention when byteorder.h was merged.

@OlegHahm
Copy link
Member Author

OlegHahm commented Nov 6, 2014

@authmillenon, I checked for at86rf231 and cc2420. For mc1322x I don't know, but should behave the same way. native is not affected anyway.

@OlegHahm
Copy link
Member Author

OlegHahm commented Nov 6, 2014

@Kijewski, I agree, but this would require a somewhat bigger change and I don't dare to introduce this now - one day before the IoT-Lab tutorial. Would you agree to merge the (cleaned up) second solution for now and prepare a new PR for replacing the right data types in the network stack later on? (Maybe even after the refactoring?)

Kijewski
Kijewski reviewed Nov 6, 2014
View reviewed changes
sys/net/link_layer/net_if/net_if.c
@@ -335,7 +335,8 @@ int net_if_send_packet(int if_id, uint16_t target, const void *payload,
p.frame.fcf.frame_pend = 0;

p.frame.dest_pan_id = net_if_get_pan_id(if_id);
memcpy(p.frame.dest_addr, &target, 2);
uint16_t target_h = byteorder_ntohs((network_uint16_t) target);
memcpy(p.frame.dest_addr, &target_h, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a memcpy needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because dest_addr is an array.

@OlegHahm
Copy link
Member Author

OlegHahm commented Nov 6, 2014

I've chosen to use NTOHS because these macros were already used in the rest of the file.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 6, 2014
@OlegHahm
Copy link
Member Author

OlegHahm commented Nov 6, 2014

Travis is fine. Who else (after squashing)?

@thomaseichinger
Copy link
Member

ACK, merge at will.

@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 6, 2014
@OlegHahm
Copy link
Member Author

OlegHahm commented Nov 6, 2014

Squashed - @Kijewski, can you live with this solution for now?

@Kijewski
Copy link
Contributor

Kijewski commented Nov 6, 2014

yes

OlegHahm added a commit that referenced this pull request Nov 6, 2014
@OlegHahm
Merge pull request #1948 from OlegHahm/netif_byteorder
8f93b5c 
Netif fix byteorder
@OlegHahm OlegHahm merged commit 8f93b5c into RIOT-OS:master Nov 6, 2014
@OlegHahm OlegHahm deleted the netif_byteorder branch November 6, 2014 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

Labels
Area: network Area: Networking Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2014.12
Development

Successfully merging this pull request may close these issues.
4 participants
@OlegHahm @Kijewski @thomaseichinger @miri64