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

gnrc: link-type flag #3697

Merged
merged 3 commits into from
Sep 1, 2015
Merged

gnrc: link-type flag #3697

merged 3 commits into from
Sep 1, 2015

Conversation

OlegHahm
Copy link
Member

Alternative to #3632.

Introduces a netopt type to indicate a wired link. To save code-lines for the default case (aka wireless tranceivers) a non-wired interface can just return -ENOTSUP. A corresponding flag is set on an IPv6 interface.

@OlegHahm OlegHahm added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Aug 23, 2015
@miri64
Copy link
Member

miri64 commented Aug 23, 2015

gnrc_netdev_eth needs to return this as true, right?

miri64
miri64 reviewed Aug 23, 2015
View reviewed changes
sys/shell/commands/sc_netif.c

printf("Link type: %s ", (entry->flags & GNRC_IPV6_NETIF_FLAGS_IS_WIRED) ?
"wired" : "wireless");
linebreak= true;
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in an extra line? Though it is a flag internally, the way you print it does not show it as a flag.

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 have no opinion on this. What do I need to do? Apparently the linebreak = true isn't enough.

Copy link
Member

Choose a reason for hiding this comment

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

Just put it above https://github.com/RIOT-OS/RIOT/pull/3697/files#diff-ad63ecb056f596bac854115d8ccb46ebR300 e.g.

Copy link
Member

Choose a reason for hiding this comment

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

This one still needs fixing, though

@OlegHahm
Copy link
Member Author

gnrc_netdev_eth needs to return this as true, right?

Anything else apart from https://github.com/RIOT-OS/RIOT/pull/3697/files#diff-c385624ce7895442f48edbb1bc19963fR276 ?

@miri64
Copy link
Member

miri64 commented Aug 23, 2015

Ah, overlooked that ^^

@OlegHahm OlegHahm mentioned this pull request Aug 23, 2015
Closed
miri64
miri64 reviewed Aug 25, 2015
View reviewed changes
sys/include/net/netopt.h
*
* @note Setting this option will always return -EONOTSUP.
*/
NETOPT_IS_WIRED,
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the netopt_enable_t type here too? That -ENOTSUP is returned for set (or when the interface is not wired) is given if you document this option only for "getting". Look at the other flags to get an idea about how they are documented/use netopt_enable_t

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 don't know what I should change here.

Copy link
Member

Choose a reason for hiding this comment

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

    NETOPT_IS_WIRED,            /**< get if interface is wired as type
                                 *   netopt_enable_t. */

Copy link
Member Author

Choose a reason for hiding this comment

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

For the record: IIRC we concluded that no change is necessary. Correct me, if I'm wrong. To me the current version seems the most descriptive one.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Later PRs can update the other option types.

@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 Aug 31, 2015
@OlegHahm
Copy link
Member Author

addressed comment

@miri64
Copy link
Member

miri64 commented Aug 31, 2015

ACK, please squash.

@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 Aug 31, 2015
@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 1, 2015
@OlegHahm
Copy link
Member Author

OlegHahm commented Sep 1, 2015

squashed and waiting for Travis

@miri64
Copy link
Member

miri64 commented Sep 1, 2015

Unittests need adaptation.

@OlegHahm
Copy link
Member Author

OlegHahm commented Sep 1, 2015

adapted and squashed immediately

@miri64
Copy link
Member

miri64 commented Sep 1, 2015

Re-ACK

OlegHahm added a commit that referenced this pull request Sep 1, 2015
@OlegHahm
Merge pull request #3697 from OlegHahm/netopt_wireless_ro
eac7f3e 
gnrc: link-type flag
@OlegHahm OlegHahm merged commit eac7f3e into RIOT-OS:master Sep 1, 2015
@OlegHahm OlegHahm deleted the netopt_wireless_ro branch September 1, 2015 09:54
@kaspar030
Copy link
Contributor

@OlegHahm Somehow this PR breaks ping6 (didn't try more networking) on trifecta using cc110x.

@OlegHahm
Copy link
Member Author

OlegHahm commented Sep 2, 2015

Weird, I certainly tested on iotlab-m3 and native last week. Will investigate.

@kaspar030
Copy link
Contributor

Got it. Setting 0 to flags overrides setting other flags from above.

@kaspar030
Copy link
Contributor

(have a fix ready, wait)

@kaspar030 kaspar030 mentioned this pull request Sep 2, 2015
Merged
@kaspar030
Copy link
Contributor

Dammit, 2 hrs of bisecting.

@OlegHahm
Copy link
Member Author

OlegHahm commented Sep 2, 2015

sorry

@kaspar030
Copy link
Contributor

no problemo ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

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 Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@OlegHahm @miri64 @kaspar030