Skip to content

Thread enhancements - #592

Merged
ivmarkov merged 3 commits into
masterfrom
thread-enhancements
Aug 11, 2025
Merged

Thread enhancements#592
ivmarkov merged 3 commits into
masterfrom
thread-enhancements

Conversation

@ivmarkov

@ivmarkov ivmarkov commented Aug 5, 2025

Copy link
Copy Markdown
Collaborator

Thread Enhancements

This work is the outcome of the ongoing effort to enable esp-idf-matter to operate over the Thread protocol as well.

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 📖

This PR is extending the existing Thread impl as follows:

  • Full support for the SRP API (= Thread-specific mDNS)
  • Ability to start/stop the Thread stack (the existing ThreadDriver::run method is removed). Note that stopping the Thread stack requires ESP IDF V5.5, or the not-yet released patch-levels V5.3.4, V5.4.3 and so on up to V5.1.X. Before this change into ESP-IDF itself, it was simply not possible to stop the Thread event loop in a clean manner, which was quite annoying
  • ThreadDriver::init / ThreadDriver::deinit are gone. The initialization is done simply on thread driver (or EspThread) construction, and the deinitialization - when the driver is dropped.

With the addition of the start / stop methods, and with the removal of the init / deinit methods, the API of ThreadDriver / EspThread becomes quite similar to WifiDriver / EspWifi which I think is a big advantage, as switching one for the other is much more seamless.

Testing

Testing was done with esp-idf-matter itself.

@ivmarkov
ivmarkov force-pushed the thread-enhancements branch 14 times, most recently from b67b85e to 6c0cf03 Compare August 7, 2025 09:44
@ivmarkov
ivmarkov marked this pull request as ready for review August 7, 2025 10:45
@ivmarkov
ivmarkov requested a review from Vollbrecht August 7, 2025 10:45
@ivmarkov
ivmarkov force-pushed the thread-enhancements branch from 6c0cf03 to add36d0 Compare August 10, 2025 17:34
@ivmarkov

Copy link
Copy Markdown
Collaborator Author

@Vollbrecht If you don't feel like reviewing this I can merge right away. It is anyway a bit specialized stuff.

@Vollbrecht

Copy link
Copy Markdown
Collaborator

I already was looking into it, but currently am traveling. I think overall i think it looks like a solid refactor, though as i hinted i am currently not in the country so my time is limeted.

One thing i am curious about, could you just briefly explain the gains of using the UnsafeCell<Box>. Before you needed two indirection, but this is all still a opauque pointer that you smuggle into the C callback context.

Does the UnsafeCell allow you for cheaper init when you got it from the C side or what place purpose does it have.

@Vollbrecht

Copy link
Copy Markdown
Collaborator

But overall if it works i think it looks fine by me ;D

Comment thread .cargo/config.toml Outdated
Comment thread src/netif.rs Outdated
Comment thread src/thread/srp.rs Outdated
@Vollbrecht

Copy link
Copy Markdown
Collaborator

So for the SRP state change we need something like a MultiWakerRegistration from embassy-sync to navigate around the problem you have with multiple async task? Is this a performance critical path or can we use some of the shelf async multi task solution for it?

@ivmarkov

Copy link
Copy Markdown
Collaborator Author

I already was looking into it, but currently am traveling. I think overall i think it looks like a solid refactor, though as i hinted i am currently not in the country so my time is limeted.

One thing i am curious about, could you just briefly explain the gains of using the UnsafeCell. Before you needed two indirection, but this is all still a opauque pointer that you smuggle into the C callback context.

Does the UnsafeCell allow you for cheaper init when you got it from the C side or what place purpose does it have.

Before:

  • We only needed to store two callbacks: the ipv6 RX and the scan callbacks

Now:

  • Since we have SRP enabled, we need to manage the SRP buffers. These need a stable address location for reasons too long to explain here (OT SRP API), so we need another Box for all the SRP stuff. Therefore it is just easer if we Box everything. The SRP impl is by the way copied from the baremetal openthread code and adjusted a bit for the purposes of the ESP-IDF-integrated OpenThread.

As for the UnsafeCell - while not strictly needed specifically for xtensa and riscv32imac I think - it is good if not mandatory to have it in there, as the combination of the UnsafeCell<Box> and the OtLock instance that must be used when calling into OT APIs under ESP-IDF essentially creates a Mutex. It is a hint for the compiler to disable aliasing optimizations

@ivmarkov
ivmarkov force-pushed the thread-enhancements branch from c44a7ca to fc69f03 Compare August 11, 2025 11:13
@ivmarkov

ivmarkov commented Aug 11, 2025

Copy link
Copy Markdown
Collaborator Author

So for the SRP state change we need something like a MultiWakerRegistration from embassy-sync to navigate around the problem you have with multiple async task? Is this a performance critical path or can we use some of the shelf async multi task solution for it?

No we don't need any of this.

Just like Wifi, so is Thread publishing events for its various state changes on the system event bus. So if the user would like to be notified on such changes, she can subscribe to the system event bus and listen to these events. They are even mapped to Rust typesafe enums since the previous version already. Or the user can just use polling as I do, which for most cases is good enough.

The commented out code was a carry-over from the bare-metal openthread impl, where obviously there is no system event bus, so there was a necessity to have other means to notify the user on state changes for baremetal.

@ivmarkov
ivmarkov merged commit ed819f5 into master Aug 11, 2025
20 checks passed
@ivmarkov
ivmarkov deleted the thread-enhancements branch July 20, 2026 16:59
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