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

feature(async): ppp support using embassy-net-ppp & embassy-at-cmux #101

Open
wants to merge 11 commits into
base: feature/async
Choose a base branch
from

Conversation

MathiasKoch
Copy link
Member

@MathiasKoch MathiasKoch commented Feb 16, 2024

@tarfu FYI

This is still very early WIP, but perhaps you have some ideas you want to chip in.

Concerns uncovered so far from my end, that I would like to see addressed by us at some point:

The OperationState::DataEstablished is not used in PPP mode at all, as we do our own dial-up using ATD*99#1

I think it might make sense to feature-gate the last state behind flags that actually makes use of the internal TCP/IP stack? ublox-sockets, ublox-mqtt etc. family of feature flags?

The initialization/resource creation is a bit cumbersome/bloated

Currently, the creation of all the resources during program initialization feels a bit verbose and brittle.
I think we should be able to wrap this much nicer together somehow.
Eventually, I would love to see a situation where we would just get back a set of (Device, Runner, Control), where:

  • The Runner is owning and propagating the full combination of cmux, ppp and the existing ublox runner through an internal select/join (maybe even the uart ingeress?)
  • The Control is the same struct as when using the internal TCP/IP stack (network state, operators etc.)
  • The Device is the struct you would pass to embassy-net::Stack

It makes use of BlackbirdHQ/atat#203

@MathiasKoch MathiasKoch changed the base branch from master to feature/async February 16, 2024 07:08
@MathiasKoch MathiasKoch mentioned this pull request Feb 16, 2024
@tarfu
Copy link
Contributor

tarfu commented Feb 16, 2024

Hey it looks nice will build an example for my setup as well. Maybe feature gating both the data_established and the internal network stack as well as the PPP stuff, so you can decide what you want and reduce the size to a minimum?

@tarfu
Copy link
Contributor

tarfu commented Feb 16, 2024

OK, after looking a bit closer, it is nice that it runs but really needs some moving of parts cleaning and putting all the wiring and implementation into a module. For skipping the at_init we could feature gate it for non PPP, but I'm not sure if it really is a dealbreaker to have it. A lot of the stuff should not harm the PPP session and are set in your program as well.
Exclusive feature gating of PPP and internal stack should be fine. I cannot think of a reason why you would want to have the ability to do both in one application, or can you see a reason for that?

But really cool POC!

@MathiasKoch
Copy link
Member Author

Absolutely, this is very much just a PoC!
I just wanted to gauge where stuff would not be compatible with PPP mode.

So far my finding are, as you mentioned:

  • The Initialized step
  • The DataEstablished step
  • The resource creation and general packaging

I think an either or approach to ublox-sockets vs ppp would be fine, I also can't see any scenario where the same binary should support both.
The only concern is that features should preferably be additive, but that is often not the case in embedded anyway, and is already not with the module features.

… batteries included new functions that sets up ATAT and all related resources
@MathiasKoch
Copy link
Member Author

@tarfu

Take a look at my latest commit changes. I think I have managed to clean it up quite a lot for both the PPP mode and the regular ublox mode.

I have made changes such that the fn new and fn new_ppp will setup all related resources as well, including the full ATAT client and ingress, CMUX runner and PPP channels.
I will still move some modules around, and I think the state machine in the runner has room for improvement, when considering both "normal" mode and ppp mode.

Overall, I am very happy with the result of this so far.

Stuff on my to-do:

  • Allow setting the baud rate nicely
  • Make sure error-handling works correctly in PPP mode, including mux and ppp setup after module restarts

@tarfu
Copy link
Contributor

tarfu commented Feb 21, 2024

It looks a lot nicer!

How do you feel about the PRs it is depending on, would you think they are ready to try to be merged with a bit of work like renaming and changing descriptions?

For the features, I would go with two features: ppp internal-network-stack
If you have none activated, you can only send SMS and stuff like that and then choose one or the other.
If you select both, you could guard like it is done with the log and defmt features a lot:

#[cfg(all(feature = "ppp", feature = "internal-network-stack"))]
compile_error!("You may not enable both `ppp` and `internal-network-stack` features.");

The state machine definitely has room for improvement, but just feature gating the stuff not needed should work without issue.

What is your baud rate an issue? It is always set up in the HAL layer, and I don't think there is a way to change it for a driver.
So setting it and then relying on the autodiscovery is probably all you can do.

@tarfu
Copy link
Contributor

tarfu commented Mar 1, 2024

Nice additions and refactorings!
Makes the codebase quite a bit more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants