Skip to content

slip: port to be used with netdev#7381

Merged
kYc0o merged 1 commit intoRIOT-OS:masterfrom
miri64:slipdev/api/slip-port-to-netdev
Oct 10, 2017
Merged

slip: port to be used with netdev#7381
kYc0o merged 1 commit intoRIOT-OS:masterfrom
miri64:slipdev/api/slip-port-to-netdev

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Jul 18, 2017 •
edited
Loading

The last link-layer, that is still purely GNRC is ported to netdev with this PR.

This PR is part of the network layer remodelling effort:
PR dependencies

@miri64 miri64 added Area: drivers Area: Device drivers Area: network Area: Networking labels Jul 18, 2017
@miri64 miri64 requested review from jnohlgard and kYc0o July 18, 2017 12:41
@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 Jul 18, 2017
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 18, 2017

Not tested yet, but have it on the agenda.

@miri64 miri64 force-pushed the slipdev/api/slip-port-to-netdev branch from 302a337 to f312f05 Compare July 18, 2017 13:43
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 19, 2017

Now tested with tests/slip between two samr21-xpro. Next: testing with tunslip6 and gnrc_networking.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 19, 2017

Next: testing with tunslip6 and gnrc_networking.

Done. Ready for review.

@miri64 miri64 force-pushed the slipdev/api/slip-port-to-netdev branch from ebf0679 to 4ea68ce Compare July 19, 2017 11:26
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 8, 2017
@miri64
Merge branch 'slipdev/api/slip-port-to-netdev' (RIOT-OS#7381) into HEAD
a06eb47
@miri64 miri64 mentioned this pull request Aug 8, 2017
Merged
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017
@miri64
Merge branch 'slipdev/api/slip-port-to-netdev' (RIOT-OS#7381) into RI…
d84bc88 
…OT-OS#7462
bergzand
bergzand reviewed Oct 7, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only a single remark here.

drivers/include/slipdev.h
cib_t pktfifo_idx; /**< CIB for slipdev_t::pktfifo */
uint16_t inbytes; /**< the number of bytes received of
* a currently incoming packet */
uint16_t inesc; /**< device previously received an escape
Copy link
Copy Markdown
Member

@bergzand bergzand Oct 7, 2017

Choose a reason for hiding this comment

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

I don't know if this has already been discussed in the past, but is there a reason to declare this as a uint16_t and not as an bool?

Copy link
Copy Markdown
Member Author

@miri64 miri64 Oct 8, 2017

Choose a reason for hiding this comment

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

Saves 4 bytes (which are in static memory => always will stay) on 32-bit platforms. And uint8_t doesn't help, because the struct would be aligned to the next 4-byte word anyway.

Copy link
Copy Markdown
Member

@bergzand bergzand Oct 10, 2017

Choose a reason for hiding this comment

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

Okay

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Oct 7, 2017

Is there (still) a reason to keep a per project slipdev_params.h instead of having a drivers/slipdev/include/slipdev_params.h identical to the other (radio) drivers?

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

Is there (still) a reason to keep a per project slipdev_params.h instead of having a drivers/slipdev/include/slipdev_params.h identical to the other (radio) drivers?

The UART may differ from board to board, so I would prefer to keep it in the application for now. Future plans are to adapt slipmux so the UART used for stdio can be used. If that happened we can move it to drivers/slipdev/include/slipdev_params.h

@miri64 miri64 force-pushed the slipdev/api/slip-port-to-netdev branch from eb3c040 to 654c08d Compare October 10, 2017 10:05
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

Rebased and adapted for current master.

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2017
@miri64
Merge branch 'slipdev/api/slip-port-to-netdev' (RIOT-OS#7381) into R…
326e060 
…IOT-OS#7462
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Oct 10, 2017 •
edited
Loading

The UART may differ from board to board, so I would prefer to keep it in the application for now. Future plans are to adapt slipmux so the UART used for stdio can be used. If that happened we can move it to drivers/slipdev/include/slipdev_params.h

Sounds good to me, no more comments here from me.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Oct 10, 2017

With the two last fixes (assert and compilation) it looks good to me, but only by reading I did not test it.

kYc0o
kYc0o approved these changes Oct 10, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

ACK. Tested and works as expected.

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Oct 10, 2017

Please squash and address Murdock's complaints.

@miri64 miri64 force-pushed the slipdev/api/slip-port-to-netdev branch from dd2324c to e42a69e Compare October 10, 2017 21:08
@miri64 miri64 force-pushed the slipdev/api/slip-port-to-netdev branch from e42a69e to b74ee88 Compare October 10, 2017 21:14
@kYc0o kYc0o merged commit a82930c into RIOT-OS:master Oct 10, 2017
@miri64 miri64 deleted the slipdev/api/slip-port-to-netdev branch October 10, 2017 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergzand bergzand bergzand left review comments

@kYc0o kYc0o kYc0o approved these changes

@jnohlgard jnohlgard Awaiting requested review from jnohlgard

Assignees

@kYc0o kYc0o

Labels

Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@miri64 @bergzand @cladmi @kYc0o