Skip to content

sys/net/gnrc_pktbuf: deduplicate code#15839

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:gnrc_pktbuf_release_error
Jan 26, 2021
Merged

sys/net/gnrc_pktbuf: deduplicate code#15839
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:gnrc_pktbuf_release_error

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Jan 22, 2021

Contribution description

Implement gnrc_pktbuf_release_error() in gnrc_pktbuf once, rather than providing two implementations in gnrc_pktbuf_static and gnrc_pktbuf_malloc.

Testing procedure

  • build for master and this PR:
    • USEMODULE=gnrc_pktbuf_static make -C examples/gnrc_networking
    • USEMODULE=gnrc_pktbuf_malloc make -C examples/gnrc_networking
  • verify that the binary don't change compared to master

Issues/PRs references

suggested in review of #15694

@maribu maribu added Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Jan 22, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 22, 2021

The stats are not too impressive, because:

  • An additional internal header was added, with all the license, comment etc. overhead
  • Some variables previously marked as static now do export symbols, so longer variable names are used instead to prevent collisions. This resulted in lines becoming longer than the char limit
  • Split out functions / variables have been documented

So: The stats are lying, it is less code in this PR than in master ;-)

miri64
miri64 reviewed Jan 22, 2021
View reviewed changes
@maribu maribu force-pushed the gnrc_pktbuf_release_error branch from 275ad32 to 919c6e6 Compare January 22, 2021 19:27
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 22, 2021
@miri64 miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 23, 2021
miri64
miri64 reviewed Jan 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There is some stuff you missed (see Murdock). Also I would recommend to run the unittest tests-pktbuf on several boards (especially those with strict alignment) just to make sure nothing get's b0rken.

Not part of your PR, I know, but might I convince you to fix those vera++ warnings as well?

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 23, 2021
maribu
maribu commented Jan 23, 2021
View reviewed changes
maribu
maribu commented Jan 23, 2021
View reviewed changes
@maribu
sys/net/gnrc_pktbuf: deduplicate code
3970b66 
Implement gnrc_pktbuf_release_error() in gnrc_pktbuf once, rather than providing
two implementations in gnrc_pktbuf_static and gnrc_pktbuf_malloc
@maribu maribu force-pushed the gnrc_pktbuf_release_error branch from 6a4d69a to 3970b66 Compare January 26, 2021 09:50
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 26, 2021

make BOARD=nucleo-f767zi -C tests/unittests/ tests-pktbuf flash test

Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
READY
s
START
.............................................
OK (45 tests)

make BOARD=m1284p -C tests/unittests/ tests-pktbuf flash test

(m1284p is a custom ATmega1284P breakout board. It's like atmega1284p, but with a 12 MHz crystal. It's the only AVR with enough RAM to run it I have at hands.)

main(): This is RIOT! (Version: 2021.04-devel-176-g3970b-gnrc_pktbuf_release_error)
Help: Press s to start test, r to print it is ready
s
START
.............................................
OK (45 tests)

make BOARD=nucleo-f303re -C tests/unittests/ tests-pktbuf flash test

Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
elp: Press s to start test, r to print it is ready
Help: Press s to start test, r to print it is ready
READY
s
START
.............................................
OK (45 tests)

make BOARD=nucleo-f446re -C tests/unittests/ tests-pktbuf flash test

Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
U���Press s to start test, r to print it is ready
Help: Press s to start test, r to print it is ready
READY
s
START
.............................................
OK (45 tests)

make BOARD=arduino-due -C tests/unittests/ tests-pktbuf flash test

Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
main(): This is RIOT! (Version: 2021.04-devel-176-g3970b-gnrc_pktbuf_release_error)
Help: Press s to start test, r to print it is ready
READY
s
START
.............................................
OK (45 tests)

make BUILD_IN_DOCKER=1 BOARD=esp32-mh-et-live-minikit -C tests/unittests/ tests-pktbuf flash test

Connect to serial port /dev/ttyUSB0
Welcome to pyterm!
Type '/exit' to exit.
READY
s
START
.............................................
OK (45 tests)

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 26, 2021

All comments are addressed, the CI is happy, and I tested the shit out of it :-)

miri64
miri64 reviewed Jan 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Yepp. Just have a small remaining comment. You may squash that directly in.

@miri64 miri64 added CI: run tests If set, CI server will run tests on hardware for the labeled PR 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 Jan 26, 2021
miri64
miri64 approved these changes Jan 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK & Go!

@miri64 miri64 merged commit 8e47621 into RIOT-OS:master Jan 26, 2021
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jan 26, 2021

Thanks :-)

@maribu maribu deleted the gnrc_pktbuf_release_error branch January 26, 2021 14:41
@maribu maribu mentioned this pull request Jan 27, 2021
Merged
2 tasks
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

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 CI: run tests If set, CI server will run tests on hardware for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2021.04

Development

Successfully merging this pull request may close these issues.

3 participants

@maribu @miri64 @kaspar030