Add L2 PPP and PPPoE

[scaprile]

This PR:

• Splits the former PPP driver into two pieces of code
  • The AT command handling and serial port byte stuffing section is refactored to do stuffing/unstuffing on the fly, as bytes are sent and received; this is moved to a new driver: ATcmd, at atcmd.{c,h}- This driver also sends the HDLC idle (start/stop) flags. This can be easily configured out in the future, byte stuffing/unstuffing has a configuration switch to disable them now.
  • The actual PPP and HDLC section is refactored to work at the frame level (instead of byte level), and moved to an L2 implementation: L2_PPP, at l2_ppp.{c,h}. There, reusable portions are moved to internal functions and support for PPPoE is added.
• Removes the former ppp.{c,h} driver
• Adds two new l2types: MG_TCPIP_L2_PPP and MG_TCPIP_L2_PPPoE
• Modified the SIM800 example to use the ATcmd driver and configure the PPP l2type
• Adds an STM32F746 example connecting through PPPoE
• Slightly modifies the L2 internal API, and adds an extra poll() function call. This is because some L2 need to do things on their own, independently of higher level or received traffic. PPPoE is perhaps the simplest, having to initiate a session to a server before any PPP traffic gets transferred and before any IP datagrams can be interchanged.
  • PPP still works as a responsive peer, reacting to server requests; PPPoE clients must initiate a session, sort of like DHCP works, so that basic idea was copied here for that.
• Adds an extra state to the mif interface, to separate driver_ready from l2_ready. Any L2 function can then know if its physical link or underlying layer is ready or not.
  • This is used to start sending PPPoE PADI frames after the PHY negotiates with the switch and brings the link up; and to tear down an established session if the Ethernet link gets disconnected.
  • This, along with the poll() function, can be used for other future L2 implementations that require negotiating link protocols or discovering networks (Thread, for example)

NOTE: statics should move into a struct mg_l2_ppp_data, referenced through void * ifp->l2data

[scaprile]

Let's not merge this until 7.21 is out; --> draft

[robertc2000]

I think it would be more clear if we provide enums/macros for the values s_state variable takes: 0, 1, 2... and use those in the code instead of hardcoding. It is a bit unclear what those state values mean.

[scaprile]

Well, yeah, though PPPoE states are sequential so once you get the machine it is clear. The client advances from 0 to 2, and stays there... session state. Discovery, Offer, Session... that's it.
I'd probably add these later, doesn't hurt, let's see if CPQ likes the state machine... I could have avoided it but thought keeping it is more robust and doesn't hurt.

[gvanem]

This code:

  return &s_mapip;  // bogus
}

extern struct mg_l2addr s_mapip;

is still here. How is a compiler supposed to accept this w/o a s_mapip; further up?
Like:

extern struct mg_l2addr s_mapip;
 ...
  return &s_mapip;  // bogus
}

And BTW, this AT command stuff brings back memories to the 90-ies.
I'd whish there's a way to test this today. Good job anyway.

[scaprile]

Will check why there are no warnings, probably the optimizer removes that for us. It is for equivalence to other L2 (eth) where it is needed.

Regarding AT, to me it were the 80s, but you can find them in every cell modem today, and (not so) sadly on ST's latest Wi-Fi module... In fact, this comes from splitting a driver that provided PPP and AT in the same module.

[gvanem]

, probably the optimizer removes that for us.

A compiler must parse and validate the code before the optimizer does it's stuff AFAICS.

[scaprile]

Yes, I agree, I'm just speculating out of thin air on probable reasons to explain why we don't get warnings...

[robertc2000]

mongoose/src/l2.c

Line 115 in 9c48ac6

struct mg_l2addr s_mapip;

It's declared here, in l2.c. When mongoose.c is generated, the declaration be the first occurrence in the file, so I believe this explains why there are no warnings.

[scaprile]

Yes, you're right, only the unamalgamated sources would complain but those aren't built for PPP
Anyway, I don't want to add a new push as there are two other PRs currently based on this one, and we still have to pass CPQ scrutiny. I will move that line up just before merge time and that will remove raised eyebrows.

[gvanem]

only the unamalgamated sources would complain

Exactly what I do. Where is it stated that an unamalgamated build is not allowed?

And BTW, some Makefile contain # Make sure we can build from unamalgamated sources.
But this AFAICS depends on the shell sorting ../src/*.c into ../src/l2.c before ../src/l2_ppp.c.

[scaprile]

I didn't say it is not allowed, I said not built, meaning we don't build them for PPP; it is not in our test path; currently we build only for Eth L2, and even though that is in the code anyway (no macro to remove it, sadly), yes, shell sorting will rule. We have somehow tuned that to avoid clashes among different Linux versions and Mac, though generated mongoose.h started being different among us again, after whoknowswho has upgraded...
Will be fixed, just not now.

[cpq]

shall we rather add an extra state to ifp->status instead ?

[scaprile]

mmm... maybe... didn't go that way because changes were a lot more involved... though we could leave UP as it is and add LINKUP as a previous state...
The point is this:

• Some L2 need to know when they can do their job.
• Some L2 work over other L2

Here, the ETH driver sees link up, then PPPoE starts discovering and builds a session, then we go UP
As long as we have no more than two L2 (PPPoE, Thread), which should be enough, either approach would do. For more layers I guess we'd need a more complex API anyway... with internal L2 states in l2data perhaps, to keep it simple (l2data is introduced for VLANs)
@robertc2000 WDYT ?

[cpq]

The question is really this:
When I want to see interface current status.
Where should I look? ifp->status? OR, ifp->status + ifp->driver_up - which makes the state of the interface scattered across.
ifp->driver_up deserves to be separate if it is a flag-like. can if->status be MG_TCPIP_STATE_DOWN and ifp->driver_up be true at the same time?
how about ifp->status == MG_TCPIP_STATE_UP and ifp->driver_up is false ?

What is the difference between ifp->status == MG_TCPIP_STATE_UP and ifp->driver_up ? Are they two different states? If they are, we need to use ifp->status instead of introducing ifp->driver_up

[scaprile]

Good point.
ifp->state can be DOWN and ifp->driver_up be true, that means the physical link is up, reported by the driver, but the L2 hasn't done its job in order to bring ifp->state UP, so the IP stuff can do its job. E.g.: PPPoE has just seen link up and is establishing a session; probably the same with Thread and a lot of weird link protocols we might encounter in our path to world domination.
ifp->state can't be UP and ifp->driver_up be false, the latter would predate the former, L2 depends on the driver

I think that is consistent with an extra state, as you suggest.
In fact, I think it makes more sense even for more complex scenarios:

• driver --> LINKUP
• L2 --> UP

Should any weird stack of L2 arise, it will be dealt with inside l2data in the upper L2... as l2data was added later with VLANs, I didn't think of this and thought a boolean was less intrusive. Now I think an extra state before UP is better.
Do you agree ?

[cpq]

Yeah, sounds like an extra state to me too. Currently we have this

#define MG_TCPIP_STATE_DOWN 0   // Interface is down
#define MG_TCPIP_STATE_UP 1     // Interface is up
#define MG_TCPIP_STATE_REQ 2    // Interface is up, DHCP REQUESTING state
#define MG_TCPIP_STATE_IP 3     // Interface is up and has an IP assigned
#define MG_TCPIP_STATE_READY 4  // Interface has fully come up, ready to work

How about this - and rename existing states to make them obvious. Comments stripped for now

#define MG_TCPIP_STATE_LINK_DOWN
#define MG_TCPIP_STATE_LINK_UP
#define MG_TCPIP_STATE_LINK_READY
#define MG_TCPIP_STATE_IP_DHCP
#define MG_TCPIP_STATE_IP_GOT_ADDRESS
#define MG_TCPIP_STATE_IP_READY

[scaprile]

I was right, adding a state was quite much more intrusive, that's why I avoided it initially. The end result is of course cleaner, as agreed.
DONE
Name changes will be done on an upcoming PR, otherwise I feel there are lots of unrelated changes in net_builtin and it is confusing