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

Fix crash when enabling napt #545

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Fix crash when enabling napt #545

wants to merge 30 commits into from

Conversation

indexds
Copy link
Contributor

@indexds indexds commented Jan 2, 2025

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo fmt command to ensure that all changed code is formatted correctly.
  • I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
  • My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Description

something something tcpip context else i crash.

I added some documentation as well. Really a paste from the wiki. There might be a way to do this cleaner but I could not for the life of me figure it out. The enable function takes two arguments so I can't really pass them directly I have to do this whole spaghetti thing. Let me know if you find an easier way i'll modify it.

Testing

It's not crashing.

@ivmarkov
Copy link
Collaborator

Can you paste a link to the documentation where you have found that this is the correct workaround?
I'm pretty sure the tcpip functions are deprecated since quite some time. Also, the enable_napt thing was working up until 4.4.8 or 5.0...

@indexds
Copy link
Contributor Author

indexds commented Jan 11, 2025

Can you paste a link to the documentation where you have found that this is the correct workaround? I'm pretty sure the tcpip functions are deprecated since quite some time. Also, the enable_napt thing was working up until 4.4.8 or 5.0...

I can't find anything on the doc except this, but it doesn't speak about deprecation for this function at all. I could replicate the crash if you want the logs, it fails an assert with "function must be executed inside a tcpip context".

https://docs.espressif.com/projects/esp-idf/en/stable/esp32c5/api-reference/network/esp_netif_programming.html#_CPPv414esp_netif_nextP11esp_netif_t

@indexds
Copy link
Contributor Author

indexds commented Jan 11, 2025

Which is mega weird now that I think about it because i'm looking at the code and i can't find this assert.

@ivmarkov
Copy link
Collaborator

... and you pasted a link to another function?

@indexds
Copy link
Contributor Author

indexds commented Jan 11, 2025

I know, it's the documentation of that function i'm reading,

This API doesn't lock the list, nor the TCPIP context, as this it's usually required to get atomic access between iteration steps rather that within a single iteration. Therefore it is recommended to iterate over the interfaces inside esp_netif_tcpip_exec()

This API is deprecated. Please use esp_netif_next_unsafe() directly if all the system interfaces are under your control and you can safely iterate over them. Otherwise, iterate over interfaces using esp_netif_tcpip_exec(), or use esp_netif_find_if() to search in the list of netifs with defined predicate.

It specifically says to use the tcpip exec function, so I don't think that function is deprecated, but that's the only piece of documentation I could find on that function, the tcpip_exec function itself is not marked as deprecated.

@indexds
Copy link
Contributor Author

indexds commented Jan 11, 2025

The crash I had was something like this:

assert failed: something /IDF/components/lwip/dontrememberwhere (Required to lock TCPIP core functionality!)

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.

2 participants