-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
esp32/network_ppp: Restructure to match extmod/network_ppp_lwip. #17138
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
esp32/network_ppp: Restructure to match extmod/network_ppp_lwip. #17138
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17138 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 169 169
Lines 21897 21897
=======================================
Hits 21579 21579
Misses 318 318 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bc2be92
to
4434ade
Compare
Code size report:
|
4434ade
to
cea5f2c
Compare
Yes, I think that's a good idea. Eventually would be good to have these two implementations share as much code as possible, but for now the PR here is a very good step in that direction. |
db139c8
to
8e60f20
Compare
The ESP32 PPP implementation predates the generic implementation in extmod. The new extmod implementation has a few advantages such as a better deinitialisation procedure (the ESP32 implemementation would not clean up properly and cause crashes if recreated) and using the UART IRQ functionality instead of running a task to read data from the UART. This change restructures the ESP implementation to be much closer to the new extmod version, while also bringing a few tiny improvements from the ESP32 version to the extmod version. The diff between extmod/network_ppp_lwip.c and ports/esp32/network_ppp.c is now a small set of easy to review ESP32 port-specific changes. Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>
That's a really neat improvement. At the very least it means one less FreeRTOS task, which is great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this, but reviewing the code it looks really good. I particularly like that:
- it's much more maintainable moving forward, now that it more closely matches the extmod version
- API now matches the docs
- is backwards compatible (retains old keyword argument names, security constants, etc)
- uses the UART IRQ instead of a separate polling task
Point (1) is particularly important, because PPP is quite hard to get right and modems can be very finicky. So it's great now that the code running on esp32 boards is also (indirectly) testing the extmod code, and vice versa.
As for testing this PR, the CI tests that this builds, and I trust your testing with actual PPP hardware.
Thank you very much!
8e60f20
to
3b1e22c
Compare
Summary
The ESP32 PPP implementation predates the generic implementation in
extmod
. The newextmod
implementation has a few advantages such as a better deinit procedure (the ESP32 implementation would not clean up properly and cause crashes if recreated) and using the UART IRQ functionality instead of running a task to read data from the UART.This change restructures the ESP implementation to be much closer to the new
extmod
version, while also bringing a few tiny improvements from the ESP32 version to theextmod
version. The diff betweenextmod/network_ppp_lwip.c
andports/esp32/network_ppp.c
is now a small set of easy to review ESP32 port-specific changes.Testing
I've been running these changes for months on a few custom boards (original ESP32 and ESP32S3) with three different modem chips to connect to the Internet via PPP. Those firmware versions do contain a bunch of other changes as well (for example #17135, which I highly recommend also gets considered if this PR gets merged since it fixes a broken behaviour in the UART that can now get triggered by this updated PPP version) so it's not a completely clean test. I'm fairly confident in the changes, and the approach makes sense, but I'd be happy to see other people giving it a go as well and letting me know if they spot anything.
Note for reviewers: Instead of reviewing the changes to the ESP32 version directly, it might be easier to review the differences to the
extmod
version first (they should make sense on their own) and then compare the diff between theextmod
and ESP32 versions:git diff --no-index extmod/network_ppp_lwip.c ports/esp32/network_ppp.c
Trade-offs and Alternatives
I didn't fully replace the ESP32 version with the
extmod
version since there are distinct differences, and the ESP32 port does not use the lwIP version shipped by us but the modified version shipped as part of the ESP-IDF.I've also aimed to keep the ESP32 version backwards compatible, thus it implements a few extra arguments and settings compared to the
extmod
version. Removing these deprecated arguments and porting the missing features over to theextmod
version was out of scope for this PR, to keep it a bit easier to review.We might want to add a message for future contributors to be aware that these two files are now very similar and that many changes could probably be applied to both.