Add mdns module #6175
Add mdns module #6175
Conversation
This allows for CircuitPython to resolve a .local domain and find other devices with MDNS services. First step for micropython#6174
There was a problem hiding this comment.
There was one bit which looked suspicious; aside from that some comments that don't require any direct action on your part.
I appreciate you picking up this work, since I dropped the ball. I didn't do any testing, but I look forward to using it once I remember what project I was working on that called for it.
| // Don't error if we're out of memory. Instead, truncate the tuple. | ||
| uint8_t added = 0; | ||
| while (next != NULL) { | ||
| mdns_remoteservice_obj_t *service = gc_alloc(sizeof(mdns_remoteservice_obj_t), true, false); |
There was a problem hiding this comment.
Instead of true use GC_ALLOC_FLAG_HAS_FINALISER. Not using a finali(s/z)er here would be an easy mistake to make in a fresh implementation of Server; is there a way to move the allocation step to a place where it would be shared by all ports?
For that matter, why is RemoteService in the common-hal? If it's just a container for some data found during a search, then why not make it a kind of NamedTuple or similar, allowing it to live in shared-module.
There was a problem hiding this comment.
The need for a finaliser is implementation specific in this case (therefore common-hal) because this implementation is a wrapper around the IDF allocated mdns_result_t. We could move it to shared-module and not need a finaliser but that'd require a copy of all the data into the MP heap even if it is never read.
There was a problem hiding this comment.
If this trade-off needs to be changed in the future, we can do it. My gut would have said that since the CircuitPython heap is generally larger than the IDF heap it might be better to preemptively move it, especially since then we can rely on the GC to free it after it's no longer possible to use it.
|
Looks like the doc build still needs some attention though. I couldn't get the github actions page to open (unicorn error) so I couldn't check into it further. |
|
@jepler Finally passed CI. Just need one more 👍 from you. |
This allows for CircuitPython to resolve a .local domain and find
other devices with MDNS services.
First step for #6174