Skip to content

sixlowpan: various fixes to sixlowpan_print() function#9585

Merged
cgundogan merged 2 commits intoRIOT-OS:masterfrom
miri64:sixlowpan/fix/print
Jul 24, 2018
Merged

sixlowpan: various fixes to sixlowpan_print() function#9585
cgundogan merged 2 commits intoRIOT-OS:masterfrom
miri64:sixlowpan/fix/print

Conversation

@miri64
Copy link
Copy Markdown
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
miri64 added 2 commits July 17, 2018 12:22
@miri64
sixlowpan: fix decoding of DAM in sixlowpan_print()
ecfff3e 
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
@miri64
sixlowpan: use generic format strings instead of inttypes.h macros
c6d4605 
We had bad experiences with those in the past when used with newlib-nano
;-)
@miri64 miri64 force-pushed the sixlowpan/fix/print branch from 9f81667 to c6d4605 Compare July 17, 2018 10:22
@miri64 miri64 requested a review from cgundogan July 17, 2018 10:22
kaspar030
kaspar030 reviewed Jul 17, 2018
View reviewed changes
sys/net/network_layer/sixlowpan/sixlowpan_print.c
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
Copy Markdown
Contributor

@kaspar030 kaspar030 Jul 17, 2018 •
edited
Loading

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
Copy Markdown
Member Author

@miri64 miri64 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 either, but the output formatting is broken in this function anyways (0x prefix for decimal numbers?)

Copy link
Copy Markdown
Contributor

@cladmi cladmi Jul 17, 2018

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
Copy Markdown
Member Author

@miri64 miri64 Jul 17, 2018

Choose a reason for hiding this comment

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

#1891.

@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
Copy Markdown
Member Author

miri64 commented Jul 20, 2018

Ping?

cgundogan
cgundogan approved these changes Jul 24, 2018
View reviewed changes
Copy link
Copy Markdown
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

sys/net/network_layer/sixlowpan/sixlowpan_print.c
break;

case 0x10:
case 0x01:
Copy link
Copy Markdown
Member

@cgundogan cgundogan Jul 24, 2018

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 cladmi mentioned this pull request Jul 24, 2018
Merged
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jul 24, 2018

Backport provided in #9629

@miri64 miri64 deleted the sixlowpan/fix/print branch July 24, 2018 17:51
@miri64 miri64 mentioned this pull request Jul 30, 2018
Closed
46 tasks
miri64 added a commit to miri64/RIOT that referenced this pull request Jul 30, 2018
@miri64
tests: gnrc_sixlowpan: fix expect for RIOT-OS#9585
c289ccc 
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 miri64 mentioned this pull request Jul 30, 2018
Merged
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Jul 31, 2018
@miri64 @cladmi
tests: gnrc_sixlowpan: fix expect for RIOT-OS#9585
419c0d1 
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
Merge pull request #9649 from miri64/tests/fix/gnrc_sixlowpan-print
a15a1b6 
tests: gnrc_sixlowpan: fix expect for #9585
miri64 added a commit to miri64/RIOT that referenced this pull request Jul 31, 2018
@miri64
tests: gnrc_sixlowpan: fix expect for RIOT-OS#9585
482b7eb 
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 miri64 mentioned this pull request Jul 31, 2018
Merged
miri64 added a commit that referenced this pull request Jul 31, 2018
@miri64
Merge pull request #9652 from miri64/2018.07/tests/fix/gnrc_sixlowpan…
efd9470 
…-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
@miri64
tests: gnrc_sixlowpan: fix expect for RIOT-OS#9585
0b3dae8 
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
@miri64 @llueder
tests: gnrc_sixlowpan: fix expect for RIOT-OS#9585
b5aa074 
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

Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@cgundogan cgundogan cgundogan approved these changes

@cladmi cladmi Awaiting requested review from cladmi

Assignees

No one assigned

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

Milestone

Release 2018.07

Development

Successfully merging this pull request may close these issues.

4 participants

@miri64 @cladmi @cgundogan @kaspar030