Skip to content

examples/gnrc_networking: move udp command to shell commands#16598

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
benpicco:sys/shell/udp
Dec 13, 2021
Merged

examples/gnrc_networking: move udp command to shell commands#16598
benpicco merged 1 commit intoRIOT-OS:masterfrom
benpicco:sys/shell/udp

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Jun 30, 2021

Contribution description

The udp command is a valuable debugging tool that is also useful outside of the gnrc_networking example.

To enable easy sending of udp messages in other applications during development, move the udp command to the shell module and introduce the gnrc_udp_cmd pseudo-module to enable it.

Testing procedure

The udp command is still present in the gnrc_networking example.

But it's now also possible to add

USEMODULE += gnrc_udp_cmd

to e.g. examples/gnrc_border_router and have a simple test for one-way communication available there.

Issues/PRs references

@github-actions github-actions bot added Area: build system Area: Build system Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System labels Jun 30, 2021
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jun 30, 2021
@janosbrodbeck
Copy link
Copy Markdown
Member

janosbrodbeck commented Jun 30, 2021

Which I personally also use a lot by now: I have extended the command with the optional parameter "-s" (size) to directly send a fixed number of bytes. This is then always the same character, but is great for quickly testing a specific packet size via shell.

Having something like that later would be handy (possibly as a subsequent PR).

@benpicco benpicco requested review from fjmolinas and jia200x July 6, 2021 08:06
fjmolinas
fjmolinas reviewed Jul 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

How different is tests/gnrc_udp/udp.c from this? Can it be re-used there as well?

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jul 6, 2021 •
edited
Loading

Indeed, it's the same code - but that one comes with it's own server thread instead of pktdump

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jul 6, 2021

I think you will find a lot of applications that use a variant of this code. I always wanted to consolidate them somehow (maybe even make a version that is both compatible with gnrc_netapi and sock_udp). The only problem I see with this PR is that the example now effectively does not contain any GNRC code anymore, which makes this example somewhat not a real GNRC example anymore. But I think, given that this particular example generated a lot of clones, we can live with that.

@benpicco
examples/gnrc_networking: move udp command to shell commands
da7d377 
The `udp` command is a valuable debugging tool that is also useful
outside of the gnrc_networking example.

To enable easy sending of udp messages in other applications during
development, move the `udp` command to the shell module and introduce
the `gnrc_udp_cmd` pseudo-module to enable it.
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jul 21, 2021

rebased after #16634 was merged

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Nov 18, 2021

I think you will find a lot of applications that use a variant of this code. I always wanted to consolidate them somehow (maybe even make a version that is both compatible with gnrc_netapi and sock_udp). The only problem I see with this PR is that the example now effectively does not contain any GNRC code anymore, which makes this example somewhat not a real GNRC example anymore. But I think, given that this particular example generated a lot of clones, we can live with that.

Is that an ACK or a will not NACK?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 18, 2021

A won't NACK.

@fjmolinas fjmolinas mentioned this pull request Dec 13, 2021
Merged
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Dec 13, 2021

Lets get this one in, @benpicco can you provide some test output?

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Dec 13, 2021

Sure. e.g. when adding

USEMODULE += gnrc_udp_cmd

to the border router application:

main(): This is RIOT! (Version: 2022.01-devel-1218-gb5962-sys/shell/udp)
RIOT border router example application
All up, running the shell now
> udp server start 1234
Success: started UDP server on port 1234
> PKTDUMP: data received:
~~ SNIP  0 - size:   4 byte, type: NETTYPE_UNDEF (0)
00000000  74  65  73  74
~~ SNIP  1 - size:   8 byte, type: NETTYPE_UDP (4)
   src-port:  1234  dst-port:  1234
   length: 12  cksum: 0x6ee9
~~ SNIP  2 - size:  40 byte, type: NETTYPE_IPV6 (2)
traffic class: 0x00 (ECN: 0x0, DSCP: 0x00)
flow label: 0x00000
length: 12  next header: 17  hop limit: 64
source address: fe80::782c:83d6:bda2:e844
destination address: ff02::1
~~ SNIP  3 - size:  18 byte, type: NETTYPE_NETIF (-1)
if_pid: 8  rssi: 0  lqi: 255
flags: BROADCAST 
src_l2addr: 7A:2C:83:D6:BD:A2:E8:44
dst_l2addr: FF:FF
~~ PKT    -  4 snips, total size:  70 byte

fjmolinas
fjmolinas approved these changes Dec 13, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK, test output provided this is a good cleanup. Regarding @miri64 comment maybe the original code could be referenced in the README and or other good GNRC example code.

@benpicco benpicco merged commit cfaa167 into RIOT-OS:master Dec 13, 2021
@benpicco benpicco deleted the sys/shell/udp branch December 13, 2021 14:03
@fjmolinas fjmolinas mentioned this pull request Dec 13, 2021
Merged
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
@kfessel kfessel mentioned this pull request Feb 15, 2022
Merged
@fjmolinas fjmolinas mentioned this pull request Feb 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@jia200x jia200x Awaiting requested review from jia200x

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2022.01

Development

Successfully merging this pull request may close these issues.

4 participants

@benpicco @janosbrodbeck @miri64 @fjmolinas