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.

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

[Link layer] GNRC PPP #5470

Closed
wants to merge 7 commits into from
Closed

[Link layer] GNRC PPP #5470

wants to merge 7 commits into from

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented May 28, 2016

Hi.

I'm PR the implementation of GNRC PPP.

@OlegHahm OlegHahm added Area: network Area: Networking GNRC labels May 31, 2016
@miri64 miri64 self-assigned this May 31, 2016
@jia200x
Copy link
Member Author

jia200x commented Jul 18, 2016

Can this PR be in the next release? @OlegHahm @miri64

@OlegHahm
Copy link
Member

I think it's difficult, since we're already in feature freeze, it's more than +3000 lines and no-one has reviewed it so far.

@jia200x
Copy link
Member Author

jia200x commented Jul 18, 2016

Yes, true. I have time to make the netdev2 adaption then.
Cheers.

2016-07-18 11:12 GMT+02:00 Oleg Hahm notifications@github.com:

I think it's difficult, since we're already in feature freeze, it's more
than +3000 lines and no-one has reviewed it so far.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#5470 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABM8SB315lGVyhJkPK1CCs30HhDmBHqqks5qW0OZgaJpZM4IpHAY
.

@miri64
Copy link
Member

miri64 commented Aug 5, 2016

@jia200x I interpret your comment that this PR is still WIP. Please remove label if this isn't the case (anymore).

@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 5, 2016
#ifdef MODULE_GNRC_PPP
GNRC_NETTYPE_LCP, /**< Protocol is PPP LCP */
GNRC_NETTYPE_IPCP, /**< Protocol is PPP IPCP */
GNRC_NETTYPE_HDLC, /**< Protocol is HDLC */
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a type for each?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline that it stays this way.

@miri64
Copy link
Member

miri64 commented Aug 30, 2016

Lot's of code beatification needed + I'm not sure if this PR was touched since we last spoke.

@miri64
Copy link
Member

miri64 commented Sep 26, 2016

@jia200x ping?

@jia200x
Copy link
Member Author

jia200x commented Oct 4, 2016

Hi @miri64

Yes, the modification is still pending. As soon as I can, I will try to make it netdev2 capable.
Cheers.

@jia200x
Copy link
Member Author

jia200x commented Oct 7, 2016

I finished the pppdev->netdev2 adaption for sim900. I'm working in a different branch because I need 4 different PR to make it work.

I don't have the SIM card adapter for SIM900 atm, so it's not tested.

Now I'm working in the gnrc_pppdev_t -> gnrc_netdev2_t adoption. You can follow the development here.

@miri64
Copy link
Member

miri64 commented Dec 7, 2017

Mhhhhh... might it make sense to implement the different PPP layers as different layers in #7736?

@jia200x
Copy link
Member Author

jia200x commented Dec 15, 2017

@miri64 Makes sense, I will check it with more details :)

@miri64
Copy link
Member

miri64 commented Mar 15, 2018

@jia200x did you?

@jia200x
Copy link
Member Author

jia200x commented Mar 15, 2018

@miri64 not yet, I will try to find some time to get into this again.

@kaspar030 kaspar030 removed this from the Release 2018.04 milestone Apr 16, 2018
@jia200x
Copy link
Member Author

jia200x commented May 8, 2018

I really want to push this. It's not in my short term list ;)

@maxvankessel
Copy link
Contributor

@jia200x I'm trying to rebase the gnrc_ppp, can we discus some of your design choices?

@jia200x
Copy link
Member Author

jia200x commented May 8, 2018

@maxvankessel sure, this is 2 years old and there are several stuff that can be improved and adapted to RIOT changes

@maxvankessel
Copy link
Contributor

I'm not quite familiar the the gnrc stack. So correct me if I'm wrong.

  • Would you recommend rebasing ppp netdev2 to a netif?
  • How would you recommend to make the sublayers (lcp, dcp ipcp) netdev layered? (Couldn't find any examples in RIOT)
  • I've made a start but unsure if I'm on the correct path (maxvankessel/RIOT:update_gnrc_ppp)

@jia200x
Copy link
Member Author

jia200x commented May 8, 2018

hi @maxvankessel

  • the ppp_netdev2_t struct should be adapted to a struct that contains ppp device information (could be renamed to ppp_netdev_t). This information could be ACCM, etc. What also has to be adapted is the gnrc_netdev2 thread to the netif threads. E.g based on this example, it could be gnrc_netif_ppp.c.
  • There are several PPP layers (Link, Auth, Network), where Auth and Network can have several protocols (PAP, CHAP, IPCP, IPCPv6, etc). There is the idea of a layered approach for netdev, but in the case for PPP is always one protocol at a time (there can be several network layers operating at the same time though). On deeper thoughts, I would tend to leave them as MAC layers rather than netdev, since they just handle information of the layers machine state. Maybe we can discuss this deeper to find a better solution than the approach of this PR ;)
  • I will check the update_gnrc_ppp branch and try to help there.

We can have a voice chat or emails if you want to clarify these components.
Let me know ;)

@jia200x
Copy link
Member Author

jia200x commented May 8, 2018

@maxvankessel just checked your code and the pppos approach seems to be really appropiate.
If it helps, I can try to get the current code working with the latest RIOT and adapt it to this pppos device.

What do you think?

@maxvankessel
Copy link
Contributor

@jia200x That would be nice. I hope the HDLC layer within PPPoS works oké, was kinda hard to test without making some custom scripts to test it.

I'm still finding a way to "inherit" the PPPoS into a modem layer, and be able to switch from data mode to command mode, retrieve IP address, etc.

@maxvankessel
Copy link
Contributor

@jia200x I've rebased it to netif, still needs a lot of cleanup. Some routines can be made a lot more efficient.

I do think the chosen signaling method between layers through messages is not ideal.

Can you verify the working of the modules? I've tested in on native with an development kit from Qquectel (M95)

@jia200x
Copy link
Member Author

jia200x commented May 25, 2018

@maxvankessel sure! I'm busy until Tuesday, but I will give it a look then ;)

@jia200x
Copy link
Member Author

jia200x commented May 30, 2018

@maxvankessel I checked the update_gnrc_ppp branch. Have you tested it? Does it work?

Are you in for a remote meeting? I think it will be the fastest way to proceed.

@maxvankessel
Copy link
Contributor

I've tested the negotiation with my modem driver. And I got it to work, needed some minor changes because of the extra lcp header I did add. Still I didn't send any data, as you could see the netif send it still send ppp data instead of ip packages encapsulated in ppp frames.

I'm in contact with Quectel (for my job) and with a local provider to see if we can enable ipv6, already made a ipv6cp. Otherwise we can use SLiRP, I think it would support it.

The next step is making it more rigged, I think the messaging makes it unnecessary complex. I would like to build a different variant based on your code which doesn't need this messaging.

I'm in for a remote meeting. Can you make a proposal? Medium wise and date time.

@maxvankessel
Copy link
Contributor

I've already proposed HDLC (#9200) and PPPoS (#9246)

@jia200x
Copy link
Member Author

jia200x commented May 31, 2018

I've tested the negotiation with my modem driver. And I got it to work, needed some minor changes because of the extra lcp header I did add. Still I didn't send any data, as you could see the netif send it still send ppp data instead of ip packages encapsulated in ppp frames.

At least for IPv4, I provided the IP encapsulation for IPCP. For IPv6CP is slightly different, but could be used as a reference.

The next step is making it more rigged, I think the messaging makes it unnecessary complex. I would like to build a different variant based on your code which doesn't need this messaging.

Sure. Sounds like a good plan

I'm in for a remote meeting. Can you make a proposal? Medium wise and date time.

Does July 4th (next monday) at 11 am work for you? Via Jitsi at https://jitsi.tools.ietf.org/gnrc_ppp

@jia200x
Copy link
Member Author

jia200x commented May 31, 2018

Does July 4th (next monday) at 11 am work for you? Via Jitsi at https://jitsi.tools.ietf.org/gnrc_ppp

I meant June

@miri64
Copy link
Member

miri64 commented Jul 26, 2019

Are we ok with closing this? Appearently there are replacements now (which are also staled, but this is besides the point)

@jia200x
Copy link
Member Author

jia200x commented Jul 26, 2019

Are we ok with closing this? Appearently there are replacements now (which are also staled, but this is besides the point)

For PPP? I think there's some work in the adoption layers, but not in the stack.
But anyway, I'm OK to close it. I know where it is, so I could continue this work at some point :)

@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Jul 26, 2019
@miri64
Copy link
Member

miri64 commented Jul 26, 2019

Ok, then I closed as archived.

@miri64 miri64 closed this Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking State: archived State: The PR has been archived for possible future re-adaptation State: waiting for other PR State: The PR requires another PR to be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants