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

sixlowpan: various fixes to sixlowpan_print() function #9585

Merged
merged 2 commits into from
Jul 24, 2018

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jul 17, 2018

Contribution description

While working with 6Lo more closely again for #8511, I noticed some bugs in the sixlowpan_print() function.

Issues/PRs references

None

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking GNRC Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch labels Jul 17, 2018
@miri64 miri64 added this to the Release 2018.07 milestone Jul 17, 2018
@miri64 miri64 requested a review from cladmi July 17, 2018 10:21
Destination address compression is all in the least significant nibble
of that byte in the IPHC header [[1]].

[1]: https://tools.ietf.org/html/rfc6282#section-3.1.1
We had bad experiences with those in the past when used with newlib-nano
;-)
printf("datagram size: %" PRIu16 "\n",
(uint16_t) (byteorder_ntohs(hdr->disp_size) & SIXLOWPAN_FRAG_SIZE_MASK));
printf("tag: 0x%" PRIu16 "\n", byteorder_ntohs(hdr->tag));
printf("datagram size: %u\n",
Copy link
Contributor

@kaspar030 kaspar030 Jul 17, 2018

Choose a reason for hiding this comment

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

I'm not sure dropping the inttypes defines is a good idea.
There were only problems with PRIu8, right?

Maybe instead of making our code not use ten year old C standard headers because of misbehaviour of newlib nano, we should try to fix or work around the problem. Maybe we can inject our own inttypes.h for newlib-nano builds...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure either, but the output formatting is broken in this function anyways (0x prefix for decimal numbers?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this issue referenced with inttypes.h macros ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 20, 2018
@miri64
Copy link
Member Author

miri64 commented Jul 20, 2018

Ping?

Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

@kaspar030

Maybe instead of making our code not use ten year old C standard headers because of misbehaviour of newlib nano, we should try to fix or work around the problem. Maybe we can inject our own inttypes.h for newlib-nano builds...

that's a good point 👍 and your proposed solution sounds also nice. We should tackle this issue separately from this PR, though. There are a lot of places that might need adjustment after such a workaround is in place.

ACK

@@ -201,15 +200,15 @@ void sixlowpan_print(uint8_t *data, size_t size)
puts("reserved");
break;

case 0x10:
case 0x01:
Copy link
Member

Choose a reason for hiding this comment

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

ui, nice catch..

@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 24, 2018
@cgundogan cgundogan merged commit e189d96 into RIOT-OS:master Jul 24, 2018
@cladmi
Copy link
Contributor

cladmi commented Jul 24, 2018

Backport provided in #9629

@miri64 miri64 deleted the sixlowpan/fix/print branch July 24, 2018 17:51
miri64 added a commit to miri64/RIOT that referenced this pull request Jul 30, 2018
PR RIOT-OS#9585 changed the output for the dump slightly, so now this test
is failing. This fixes that and should also fix some issues from the
2017.07 release tests.
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Jul 31, 2018
PR RIOT-OS#9585 changed the output for the dump slightly, so now this test
is failing. This fixes that and should also fix some issues from the
2017.07 release tests.
miri64 added a commit that referenced this pull request Jul 31, 2018
miri64 added a commit to miri64/RIOT that referenced this pull request Jul 31, 2018
PR RIOT-OS#9585 changed the output for the dump slightly, so now this test
is failing. This fixes that and should also fix some issues from the
2017.07 release tests.

(cherry picked from commit c289ccc)
miri64 added a commit that referenced this pull request Jul 31, 2018
…-print

tests: gnrc_sixlowpan: fix expect for #9585 [backport 2018.07]
danpetry pushed a commit to danpetry/RIOT that referenced this pull request Sep 5, 2018
PR RIOT-OS#9585 changed the output for the dump slightly, so now this test
is failing. This fixes that and should also fix some issues from the
2017.07 release tests.
llueder pushed a commit to llueder/RIOT that referenced this pull request Oct 21, 2018
PR RIOT-OS#9585 changed the output for the dump slightly, so now this test
is failing. This fixes that and should also fix some issues from the
2017.07 release tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants