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

shell: introduce txtsnd pseudomodule #6397

Merged
merged 1 commit into from
Jan 18, 2017
Merged

shell: introduce txtsnd pseudomodule #6397

merged 1 commit into from
Jan 18, 2017

Conversation

OlegHahm
Copy link
Member

This commit allows to enable/disable the txtsnd shell command. The
command is used to send strings over L2 in GNRC. Until now the command
was automatically enabled if GNRC and shell_commands were present, which
may lead to confusion if no L2 packet handler is registered.

Fixes #5034.

@OlegHahm OlegHahm added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation GNRC labels Jan 17, 2017
@OlegHahm OlegHahm added this to the Release 2017.01 milestone Jan 17, 2017
@OlegHahm OlegHahm mentioned this pull request Jan 17, 2017
Closed
LudwigKnuepfer
LudwigKnuepfer approved these changes Jan 18, 2017
View reviewed changes
Copy link
Member

@LudwigKnuepfer LudwigKnuepfer left a comment

Choose a reason for hiding this comment

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

ACK for the concept, I don't have a good enough overview of the existing examples/tests to judge whether some place that should provide txtsnd has been overlooked.

miri64
miri64 requested changes Jan 18, 2017
View reviewed changes
examples/posix_sockets/Makefile
@@ -16,6 +16,7 @@ BOARD_INSUFFICIENT_MEMORY := airfy-beacon chronos msb-430 msb-430h nrf51dongle n
# NOTE: 6LoWPAN will be included if IEEE802.15.4 devices are present
USEMODULE += gnrc_netdev_default
USEMODULE += auto_init_gnrc_netif
USEMODULE += gnrc_txtsnd
Copy link
Member

Choose a reason for hiding this comment

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

This example doesn't need that.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the README mentions it. Should I update the README then?

Copy link
Member

Choose a reason for hiding this comment

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

Well because typing help at the moment includes that command appears (but that should be the case for any application that includes gnrc_netif), so yes: please update README.

Copy link
Member Author

Choose a reason for hiding this comment

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

True - I was a bit lazy and only used git grep without looking at the context. Fixed.

@miri64
Copy link
Member

miri64 commented Jan 18, 2017

Any test that registers GNRC_NETTYPE_NETIF to gnrc_pktdump requires this command.

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 18, 2017
@miri64
Copy link
Member

miri64 commented Jan 18, 2017

Any test that registers GNRC_NETTYPE_NETIF to gnrc_pktdump requires this command.

  1. Sorry I meant GNRC_NETTYPE_UNDEF (we are registering for the payload, not the header)
  2. Currently this includes only tests/driver_kw2xrf and tests/driver_xbee

Sidenote: tests/driver_at86rf2xx offers its own implementation of txtsnd which is purely netdev2 based. Maybe a netdev2_txtsnd pseudo-module could be provided for those cases.

@OlegHahm
Copy link
Member Author

I updated the mentioned tests. Regarding tests/driver_at86rf2xx I agree that here is room for de-duplication, but would leave this for a separate PR.

@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 Jan 18, 2017
@miri64
Copy link
Member

miri64 commented Jan 18, 2017

I agree that here is room for de-duplication, but would leave this for a separate PR.

👍

miri64
miri64 approved these changes Jan 18, 2017
View reviewed changes
Copy link
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.

👍

@OlegHahm
shell: introduce txtsnd pseudomodule
df7927d 
This commit allows to enable/disable the txtsnd shell command. The
command is used to send strings over L2 in GNRC. Until now the command
was automatically enabled if GNRC and shell_commands were present, which
may lead to confusion if no L2 packet handler is registered.
@OlegHahm OlegHahm force-pushed the txtsnd_pseudomodule branch from 951ee26 to df7927d Compare January 18, 2017 20:00
@OlegHahm
Copy link
Member Author

Squashed

@OlegHahm OlegHahm merged commit 8f71b4c into RIOT-OS:master Jan 18, 2017
@OlegHahm OlegHahm deleted the txtsnd_pseudomodule branch January 18, 2017 20:21
@miri64 miri64 mentioned this pull request Jan 19, 2017
Merged
@PeterKietzmann PeterKietzmann mentioned this pull request Jul 10, 2017
Merged
@miri64 miri64 added Platform: MSP Platform: This PR/issue effects MSP-based platforms Area: network Area: Networking CI: needs squashing labels Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LudwigKnuepfer LudwigKnuepfer LudwigKnuepfer approved these changes

@miri64 miri64 miri64 approved these changes

Assignees

@miri64 miri64

@LudwigKnuepfer LudwigKnuepfer

@kYc0o kYc0o

Labels
Area: network Area: Networking CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Platform: MSP Platform: This PR/issue effects MSP-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@OlegHahm @miri64 @LudwigKnuepfer @kYc0o